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

[Markus Armbruster]

David Gibson <address@hidden> writes:

> 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.

Heh.

>> 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).

off_t is signed:
http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/sys_types.h.html

>> >              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.

Pity.  Thanks anyway!