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

[Michael Chang]

On Tue, Mar 22, 2022 at 10:19:26PM +0100, Daniel Kiper wrote:
> 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?

Yes, no problem.

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

Yes the shuffling was due to the same reasoning provided in patch 3.
I'll include it to flesh out in the commit message.

> 
> > +
> 
> Please drop this empty line addition.

OK.

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

OK.

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

OK.

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

Looks good to me. I will do in next version,

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

OK.

> 
> Why did you decide to use "asm() version" of this macro [1] instead of more
> C-ish one [2]?

The C-ish one doesn't work as it is acting as fallback for compilers
other than gcc without the need for such hack.

> Anyway, I would mention the idea comes from the Linux
> kernel RELOC_HIDE() macro.

I will add a comment to referencing it.

Thanks a lot for review,
Michael

> 
> 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
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel