Re: segfault in grub_device_iterate on 64bit platforms

[Colin Watson]

On Tue, Jan 19, 2010 at 08:23:44PM -0500, Dan Merillat wrote:
> diff --git a/kern/device.c b/kern/device.c
> index d4de496..3727ddc 100644
> --- a/kern/device.c
> +++ b/kern/device.c
> @@ -139,7 +139,7 @@ grub_device_iterate (int (*hook) (const char *name))
>        if (! partition_name)
>       return 1;
>
> -      p = grub_malloc (sizeof (p->next));
> +      p = grub_malloc (sizeof (*p));
>        if (!p)
>       {
>         grub_free (partition_name);
>
>
>
> Seriously though, where did someone see sizeof(p->next) and think it was
> a valid idiom?  It allocates a pointer, not a structure.  I've never
> seen it used before, and a quick rgrep shows it to be an anomaly in the
> grub source as well.  It happens to work on 32bit processors for tiny
> structures because the default alignment for malloc is 8 bytes - and
> struct part_ent is two pointers.  You end up overflowing into the
> padding and not trashing anything.

I think you're a bit overly critical here, although the patch is still
sound.  Here's the code from current trunk:

      p = grub_malloc (sizeof (p->next) + grub_strlen (disk->name) + 1 +
                       grub_strlen (partition_name) + 1);

sizeof (p->next) is perfectly reasonable on the face of it because the
structure being allocated is:

  struct part_ent
  {
    struct part_ent *next;
    char name[0];
  };

To allocate a new instance of this structure, it does look at a quick
glance as though we need the size of the next pointer, plus however much
is going to be needed for name; so I can easily understand why it was
written this way and I think you're going a bit overboard in your
criticism.

The reason that it breaks is, of course, because of padding within the
struct needed to ensure alignment.  Easily forgotten.

> Electric Fence.  Learn it, love it, live it.

Valgrind this decade, surely? ;-)


I've committed your patches now.  A ChangeLog entry would speed up the
process for next time. :-)

-- 
Colin Watson                                       address@hidden