Re: disk/mdraid1x_linux.c:181:15: warning: array subscript ...

[Thomas Schmitt]

Hi,

Paul Menzel wrote:
>   181 |      (char *) &sb.dev_roles[grub_le_to_cpu32 (sb.dev_number)]
>    98 |   grub_uint16_t dev_roles[]; /* Role in array, or 0xffff for a
>   127 |       struct grub_raid_super_1x sb;
> [...]
> Normally, it should be fixed by using `grub_uint16_t[]` instead of
> `grub_uint16_t[0]`, but I haven’t found out where yet.

I rather propose to consider this untested and not properly styled
change:

--- grub-core/disk/mdraid1x_linux.c     2018-09-05 11:41:09.690721520 +0200
+++ grub-core/disk/mdraid1x_linux.c.ts  2020-03-20 13:57:21.159675792 +0100
@@ -178,8 +178,9 @@ grub_mdraid_detect (grub_disk_t disk,
        return NULL;

       if (grub_disk_read (disk, sector,
-                         (char *) &sb.dev_roles[grub_le_to_cpu32 
(sb.dev_number)]
-                         - (char *) &sb,
+                         ((char *) &sb.dev_roles - (char *) sb)
+                         + grub_le_to_cpu32 (sb.dev_number) * 
sizeof(grub_uint16_t),
                          sizeof (role), &role))
        return NULL;

------------------------------------------------------------------------
Reasoning:

I see
  grub_uint16_t dev_roles[0];
in
  
http://git.savannah.gnu.org/cgit/grub.git/tree/grub-core/disk/mdraid1x_linux.c#n98
It's a gcc extension.
  https://www.gnu.org/software/gnu-c-manual/gnu-c-manual.html#Declaring-Arrays
  "As a GNU extension, the number of elements can be as small as zero.
   Zero-length arrays are useful as the last element of a structure which
   is really a header for a variable-length object"

So isn't the problem rather about the allocation of the struct which
hosts .dev_roles ?
Currently there is in mdraid1x_linux.c:

  struct grub_raid_super_1x
  {
    ...
    grub_uint16_t dev_roles[0];   /* Role in array, or 0xffff for a spare, or 
0xfffe for faulty.  */
  };

  ...

  static struct grub_diskfilter_vg *
  grub_mdraid_detect (grub_disk_t disk, ...
    ...
      struct grub_raid_super_1x sb;

The allocation as local variable does not appear to provide this extra
memory storage, which shall host the array members of dev_roles.
I fail to see how sb could get enlarged later. The stack neighbors of sb
do not look like they would provide their storage for an array.

Now why didn't this fail earlier ?
That's because the bad array index use is not for memory access but for
pointer arithmetics:

 
http://git.savannah.gnu.org/cgit/grub.git/tree/grub-core/disk/mdraid1x_linux.c#n180

 if (grub_disk_read (disk, sector,
          (char *) &sb.dev_roles[grub_le_to_cpu32 (sb.dev_number)]
           - (char *) &sb,
          sizeof (role), &role))

The code wants a number which shall be used as parameter grub_off_t offset
of grub_disk_read() (in grub-core/kern/disk.c).

I think that the following expression produces the same number without
virtual access to a virtual array member:

         (char *) &sb.dev_roles - (char *) sb
         + grub_le_to_cpu32 (sb.dev_number) * sizeof(grub_uint16_t)


Have a nice day :)

Thomas