libbacktrace patch RFC: check size passed to backtrace_get_view

libbacktrace patch RFC: check size passed to backtrace_get_view

[Ian Lance Taylor]

[-- Attachment #1: Type: text/plain, Size: 520 bytes --]

I agree that checking the size passed to backtrace_get_view seems like
the most reliable approach to avoid problems with large files on
32-bit systems.  How does this patch look?

Ian


2019-01-18  Ian Lance Taylor  <iant@golang.org>

PR libbacktrace/88890
* mmapio.c (backtrace_get_view): Change size parameter to
uint64_t.  Check that value fits in size_t.
* read.c (backtrace_get_view): Likewise.
* internal.h (backtrace_get_view): Update declaration.
* elf.c (elf_add): Pass shstrhdr->sh_size to backtrace_get_view.

[-- Attachment #2: patch.txt --]
[-- Type: text/plain, Size: 2553 bytes --]

Index: elf.c
===================================================================
--- elf.c	(revision 268078)
+++ elf.c	(working copy)
@@ -2813,7 +2813,7 @@ elf_add (struct backtrace_state *state,
   shstr_size = shstrhdr->sh_size;
   shstr_off = shstrhdr->sh_offset;
 
-  if (!backtrace_get_view (state, descriptor, shstr_off, shstr_size,
+  if (!backtrace_get_view (state, descriptor, shstr_off, shstrhdr->sh_size,
 			   error_callback, data, &names_view))
     goto fail;
   names_view_valid = 1;
Index: internal.h
===================================================================
--- internal.h	(revision 268078)
+++ internal.h	(working copy)
@@ -179,7 +179,7 @@ struct backtrace_view
 /* Create a view of SIZE bytes from DESCRIPTOR at OFFSET.  Store the
    result in *VIEW.  Returns 1 on success, 0 on error.  */
 extern int backtrace_get_view (struct backtrace_state *state, int descriptor,
-			       off_t offset, size_t size,
+			       off_t offset, uint64_t size,
 			       backtrace_error_callback error_callback,
 			       void *data, struct backtrace_view *view);
 
Index: mmapio.c
===================================================================
--- mmapio.c	(revision 268078)
+++ mmapio.c	(working copy)
@@ -51,7 +51,7 @@ POSSIBILITY OF SUCH DAMAGE.  */
 
 int
 backtrace_get_view (struct backtrace_state *state ATTRIBUTE_UNUSED,
-		    int descriptor, off_t offset, size_t size,
+		    int descriptor, off_t offset, uint64_t size,
 		    backtrace_error_callback error_callback,
 		    void *data, struct backtrace_view *view)
 {
@@ -60,6 +60,12 @@ backtrace_get_view (struct backtrace_sta
   off_t pageoff;
   void *map;
 
+  if ((uint64_t) (size_t) size != size)
+    {
+      error_callback (data, "file size too large", 0);
+      return 0;
+    }
+
   pagesize = getpagesize ();
   inpage = offset % pagesize;
   pageoff = offset - inpage;
Index: read.c
===================================================================
--- read.c	(revision 268078)
+++ read.c	(working copy)
@@ -46,12 +46,18 @@ POSSIBILITY OF SUCH DAMAGE.  */
 
 int
 backtrace_get_view (struct backtrace_state *state, int descriptor,
-		    off_t offset, size_t size,
+		    off_t offset, uint64_t size,
 		    backtrace_error_callback error_callback,
 		    void *data, struct backtrace_view *view)
 {
   ssize_t got;
 
+  if ((uint64_t) (size_t) size != size)
+    {
+      error_callback (data, "file size too large", 0);
+      return 0;
+    }
+
   if (lseek (descriptor, offset, SEEK_SET) < 0)
     {
       error_callback (data, "lseek", errno);

Re: libbacktrace patch RFC: check size passed to backtrace_get_view

[Tom de Vries]

On 18-01-19 16:40, Ian Lance Taylor wrote:
>  int
>  backtrace_get_view (struct backtrace_state *state ATTRIBUTE_UNUSED,
> -		    int descriptor, off_t offset, size_t size,
> +		    int descriptor, off_t offset, uint64_t size,
>  		    backtrace_error_callback error_callback,
>  		    void *data, struct backtrace_view *view)
>  {
> @@ -60,6 +60,12 @@ backtrace_get_view (struct backtrace_sta
>    off_t pageoff;
>    void *map;
>  
> +  if ((uint64_t) (size_t) size != size)
> +    {
> +      error_callback (data, "file size too large", 0);
> +      return 0;
> +    }
> +

Agreed, this will fix the PR.

There's a cornercase I'm not sure is worth bothering about, but given
that this is an RFC: in the case of 32-bit systems with 32-bit
filesystem, there will be a range of numbers that fit in size_t, but are
too large for off_t (both 32-bit but size_t unsigned and off_t signed),
so in that case, the file size is too large, but we're not detecting
that here. Though I think that should be handled in the subsequent mmap
(or, in the case of read.c, in the subsequent read(), though I'm
guessing the earlier backtrace_alloc > 2GB will already fail).

Thanks,
- Tom

Re: libbacktrace patch RFC: check size passed to backtrace_get_view

[Ian Lance Taylor]

On Fri, Jan 18, 2019 at 8:18 AM Tom de Vries <tdevries@suse.de> wrote:
>
> On 18-01-19 16:40, Ian Lance Taylor wrote:
> >  int
> >  backtrace_get_view (struct backtrace_state *state ATTRIBUTE_UNUSED,
> > -                 int descriptor, off_t offset, size_t size,
> > +                 int descriptor, off_t offset, uint64_t size,
> >                   backtrace_error_callback error_callback,
> >                   void *data, struct backtrace_view *view)
> >  {
> > @@ -60,6 +60,12 @@ backtrace_get_view (struct backtrace_sta
> >    off_t pageoff;
> >    void *map;
> >
> > +  if ((uint64_t) (size_t) size != size)
> > +    {
> > +      error_callback (data, "file size too large", 0);
> > +      return 0;
> > +    }
> > +
>
> Agreed, this will fix the PR.

Thanks.  Committed to mainline.

> There's a cornercase I'm not sure is worth bothering about, but given
> that this is an RFC: in the case of 32-bit systems with 32-bit
> filesystem, there will be a range of numbers that fit in size_t, but are
> too large for off_t (both 32-bit but size_t unsigned and off_t signed),
> so in that case, the file size is too large, but we're not detecting
> that here. Though I think that should be handled in the subsequent mmap
> (or, in the case of read.c, in the subsequent read(), though I'm
> guessing the earlier backtrace_alloc > 2GB will already fail).

Yeah, I'm not worried about that case.  A system with a signed 32-bit
off_t can't really support files larger than 2G anyhow, since for
larger files the struct stat st_size field will be negative.

Ian