Re: [PATCH for 2.06] video/fbfill: use unsigned integers for width/height

[Daniel Kiper]

On Fri, Apr 02, 2021 at 02:22:04AM +1100, Daniel Axtens wrote:
> Since commit 7ce3259f67ac ("video/fb/fbfill: Fix potential integer
> overflow"), clang builds of grub-emu have failed with messages like:
>
> /usr/bin/ld: libgrubmods.a(libgrubmods_a-fbfill.o): in function 
> `grub_video_fbfill_direct24':
> fbfill.c:(.text+0x28e): undefined reference to `__muloti4'
>
> This appears to be due to a weird quirk in how clang compiles
>
>   grub_mul (dst->mode_info->bytes_per_pixel, width, &rowskip)
>
> which is grub_mul (unsigned int, int, &grub_size_t).
>
> It looks like clang somewhere promotes everything to 128-bit maths
> before ultimately reducing down to 64 bit for grub_size_t. I think

Excerpt from [1]: These built-in functions promote the first two
operands into infinite precision signed type and perform addition on
those promoted operands. The result is then cast to the type the third
pointer argument points to and stored there.

Of course s/addition/multiplication/...

> this is because width is signed, and indeed converting width to an
> unsigned int makes the problem go away.
>
> This conversion also makes more sense generally:
>  - the caller of all the fbfill_directN functions is
>    grub_video_fb_fill_dispatch and it takes width and height as
>    unsigned ints already,
>  - it doesn't make sense to fill a negative width or height.
>
> Convert the width and height arguments and associated loop counters
> to unsigned ints.
>
> Fixes: 7ce3259f67ac ("video/fb/fbfill: Fix potential integer overflow")
> Signed-off-by: Daniel Axtens <dja@axtens.net>

Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

Daniel

[1] https://gcc.gnu.org/onlinedocs/gcc/Integer-Overflow-Builtins.html