Re: [PATCH 2/3] Fix -Werror=array-bounds array subscript 0 is outside array bounds

[Daniel Kiper]

On Thu, Mar 17, 2022 at 02:43:41PM +0800, Michael Chang via Grub-devel wrote:
> The grub is failing to build with gcc-12 in many places like this:
>
> In function 'init_cbfsdisk',
>     inlined from 'grub_mod_init' at ../../grub-core/fs/cbfs.c:391:3:
> ../../grub-core/fs/cbfs.c:345:7: error: array subscript 0 is outside array 
> bounds of 'grub_uint32_t[0]' {aka 'unsigned int[]'} [-Werror=array-bounds]
>   345 |   ptr = *(grub_uint32_t *) 0xfffffffc;
>       |   ~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> This is caused by gcc regression in 11/12 [1]. In a nut shell, the
> warning is about detected invalid accesses at non-zero offsets to null

May I ask you to be more consistent and use NULL instead of null everywhere?

> pointers. Since hardwired constant address is treated as NULL plus an
> offset in the same underlying code, the warning is therefore triggered.
>
> Instead of inserting #pragma all over the places where literal pointers
> are accessed to avoid diagnosing array-bounds, we can try to borrow the
> idea from linux kernel that the absolute_pointer macro [2][3] is used to
> disconnect a pointer using literal address from it's original object,
> hence gcc won't be able to make assumptions on the boundary while doing
> pointer arithmetic. With that we can greatly reduce the code we have to
> cover up by making initial literal pointer assignment to use the new
> wrapper but not having to track everywhere literal pointers are
> accessed. This also makes code looks cleaner.
>
> [1]
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99578
> [2]
> https://elixir.bootlin.com/linux/v5.16.14/source/include/linux/compiler.h#L180
> [3]
> https://elixir.bootlin.com/linux/v5.16.14/source/include/linux/compiler-gcc.h#L31
>
> Signed-off-by: Michael Chang <mchang@suse.com>

[...]

> diff --git a/grub-core/commands/i386/pc/drivemap.c 
> b/grub-core/commands/i386/pc/drivemap.c
> index 3fb22dc460..5e13f82eb5 100644
> --- a/grub-core/commands/i386/pc/drivemap.c
> +++ b/grub-core/commands/i386/pc/drivemap.c
> @@ -31,9 +31,6 @@
>
>  GRUB_MOD_LICENSE ("GPLv3+");
>
> -/* Real mode IVT slot (seg:off far pointer) for interrupt 0x13.  */
> -static grub_uint32_t *const int13slot = (grub_uint32_t *) (4 * 0x13);
> -
>  /* Remember to update enum opt_idxs accordingly.  */
>  static const struct grub_arg_option options[] = {
>    /* TRANSLATORS: In this file "mapping" refers to a change GRUB makes so if
> @@ -280,6 +277,9 @@ install_int13_handler (int noret __attribute__ ((unused)))
>    grub_uint8_t *handler_base = 0;
>    /* Address of the map within the deployed bundle.  */
>    int13map_node_t *handler_map;
> +  /* Real mode IVT slot (seg:off far pointer) for interrupt 0x13.  */
> +  grub_uint32_t *int13slot = (grub_uint32_t *) grub_absolute_pointer (4 * 
> 0x13);

This shuffling looks strange and should be explained in the commit message.
I understood what is going here when I took a look at patch #3.

> +

Please drop this empty line addition.

>    int i;
>    int entries = 0;
> @@ -354,6 +354,9 @@ install_int13_handler (int noret __attribute__ ((unused)))
>  static grub_err_t
>  uninstall_int13_handler (void)
>  {
> +  /* Real mode IVT slot (seg:off far pointer) for interrupt 0x13.  */
> +  grub_uint32_t *int13slot = (grub_uint32_t *) grub_absolute_pointer (4 * 
> 0x13);

WRT shuffling, ditto.

> +
>    if (! grub_drivemap_oldhandler)
>      return GRUB_ERR_NONE;

[...]

> diff --git a/grub-core/term/i386/pc/console.c 
> b/grub-core/term/i386/pc/console.c
> index d44937c305..9403390f1f 100644
> --- a/grub-core/term/i386/pc/console.c
> +++ b/grub-core/term/i386/pc/console.c
> @@ -238,12 +238,11 @@ grub_console_getkey (struct grub_term_input *term 
> __attribute__ ((unused)))
>    return (regs.eax & 0xff) + (('a' - 1) | GRUB_TERM_CTRL);
>  }
>
> -static const struct grub_machine_bios_data_area *bios_data_area =
> -  (struct grub_machine_bios_data_area *) 
> GRUB_MEMORY_MACHINE_BIOS_DATA_AREA_ADDR;
> -
>  static int
>  grub_console_getkeystatus (struct grub_term_input *term __attribute__ 
> ((unused)))
>  {
> +  const struct grub_machine_bios_data_area *bios_data_area =
> +  (struct grub_machine_bios_data_area *) grub_absolute_pointer 
> (GRUB_MEMORY_MACHINE_BIOS_DATA_AREA_ADDR);

Ditto and below...

>    /* conveniently GRUB keystatus is modelled after BIOS one.  */
>    return bios_data_area->keyboard_flag_lower & ~0x80;
>  }

[...]

> diff --git a/include/grub/compiler.h b/include/grub/compiler.h
> index 8f3be3ae70..e159f0e292 100644
> --- a/include/grub/compiler.h
> +++ b/include/grub/compiler.h
> @@ -56,4 +56,15 @@
>  #  define CLANG_PREREQ(maj,min) 0
>  #endif
>
> +#if defined(__GNUC__)
> +#  define grub_absolute_pointer(val)                                 \
> +({                                                                   \
> +     unsigned long __ptr;                                            \

I think grub_addr_t would be more appropriate here. But this requires
include/grub/types.h. So, maybe we should move this macro there.

> +     __asm__ ("" : "=r"(__ptr) : "0"((void *)(val)));                \

s/__asm__/asm/

Why did you decide to use "asm() version" of this macro [1] instead of more
C-ish one [2]? Anyway, I would mention the idea comes from the Linux
kernel RELOC_HIDE() macro.

Daniel

[1] 
https://elixir.bootlin.com/linux/v5.16.14/source/include/linux/compiler-gcc.h#L31
[2] 
https://elixir.bootlin.com/linux/v5.16.14/source/include/linux/compiler.h#L180