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