Re: [Qemu-devel] [PATCH 2/3] Allow larger return values from get_image_size()

[David Gibson]

On Mon, Feb 27, 2012 at 09:21:25AM +0100, Markus Armbruster wrote:
> David Gibson <address@hidden> writes:
> 
> > Currently get_image_size(), used to find the size of files, returns an int.
> > But for modern systems, int may be only 32-bit and we can have files
> > larger than that.
> >
> > This patch, therefore, changes the return type of get_image_size() to off_t
> > (the same as the return type from lseek() itself).  It also audits all the
> > callers of get_image_size() to make sure they process the new unsigned
> > return type correctly.
> >
> > This leaves load_image_targphys() with a limited return type, but one thing
> > at a time (that function has far more callers to be audited, so it will
> > take longer to fix).
> 
> I'm afraid this replaces the single, well-known integer overflow in
> get_image_size()'s conversion of lseek() value to int by many unknown
> overflows in get_image_size()'s users.  One example below.  Didn't look
> for more.
> 
> If you need a wider get_image_size(), please make sure its users are
> prepared for it!

Actually, I have no such need at all, but when I fixed another bug in
loader.c, someone whinged about me not changing get_image_size(), so
here it is.

> Is the any use for image sizes exceeding size_t?  Arent such images
> impossible to load?

Well, possibly not.

> 
> [...]
> > diff --git a/hw/pc.c b/hw/pc.c
> > index b9f4bc7..cb41955 100644
> > --- a/hw/pc.c
> > +++ b/hw/pc.c
> > @@ -672,7 +672,8 @@ static void load_linux(void *fw_cfg,
> >                         target_phys_addr_t max_ram_size)
> >  {
> >      uint16_t protocol;
> > -    int setup_size, kernel_size, initrd_size = 0, cmdline_size;
> > +    int setup_size, kernel_size, cmdline_size;
> > +    off_t initrd_size = 0;
> >      uint32_t initrd_max;
> >      uint8_t header[8192], *setup, *kernel, *initrd_data;
> >      target_phys_addr_t real_addr, prot_addr, cmdline_addr, initrd_addr = 0;
> > @@ -795,7 +796,7 @@ static void load_linux(void *fw_cfg,
> >     }
> >  
> >     initrd_size = get_image_size(initrd_filename);
> > -        if (initrd_size < 0) {
> > +        if (initrd_size == -1) {
> 
> Needless churn.

No, it's not.  Now that initrd_size is unsigned initrd_size < 0 would
return false always (and give a "comparison is always false due to
limited range of data type" warning).

> 
> >              fprintf(stderr, "qemu: error reading initrd %s\n",
> >                      initrd_filename);
> >              exit(1);
>            }
> 
>            initrd_addr = (initrd_max-initrd_size) & ~4095;
> 
>            initrd_data = g_malloc(initrd_size);
> 
> Integer overflow in conversion from off_t initrd_size to the argument
> type size_t[*].

Hm, true.

Ok, well, I give up.  Someone who actually needs it can fix it.

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson