David Gibson <da...@gibson.dropbear.id.au> writes:
> On Mon, Feb 27, 2012 at 09:21:25AM +0100, Markus Armbruster wrote:
>> David Gibson <da...@gibson.dropbear.id.au> 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!
