grub-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: segfault in grub_device_iterate on 64bit platforms


From: Dan Merillat
Subject: Re: segfault in grub_device_iterate on 64bit platforms
Date: Tue, 19 Jan 2010 22:24:57 -0500
User-agent: Mozilla-Thunderbird 2.0.0.22 (X11/20090707)

Colin Watson wrote:
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:

I thought the Kayne West reference would have indicated that it was mostly tongue in cheek. The question was more serious than critical though - is there code out there that uses that for linked lists? I've never seen that particular idiom, but it wouldn't be the first time something that just happens to work has been passed on as gospel.

      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];
  };

Previously, the code looked like this:
  struct part_ent
  {
    struct part_ent *next;
    char *name;
  } *ents;

...

Two pointers, with ->name referencing the return from asprintf:

p = grub_malloc(sizeof(p->next));
...
p->name=grub_asprintf ("%s,%s", disk->name, partition_name);

The bug was that you only were allocating the first pointer and not the char *. which happened to work on x86 due to malloc padding everything to 8 bytes.

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.

Actually, it was a flat-out bug on every processor before, hence my criticism. The current code has a very subtle bug on some unusual systems with alignment larger than the pointer size. - Worse, it'd only show up for certain length strings!

> I've committed your patches now.

If it looks like:
    p = grub_malloc (sizeof (*p) + grub_strlen (disk->name) + 1 +
                   grub_strlen (partition_name) + 1);

then it should be correct.





reply via email to

[Prev in Thread] Current Thread [Next in Thread]