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 [cjwat...@ubuntu.com]
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel
