Yo Grub2, I'm really happy for you and I'mma let you finish but...
allocating sizeof(p->next) instead of sizeof(*p) is the worst idiom of all time. OF ALL TIME.

</Kayne West>

Diff against debian's 1.98-experimental-20100111, to which I submit the following patches:

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.

On 64bit, the structure is 16 bytes long, and trashes whatever the next allocation is, which leads to nasty problems and random crashes later.

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

And here's a freebie to avoid a null pointer deref that -lefence pointed out:

diff --git a/util/grub-probe.c b/util/grub-probe.c
index 4ba8a98..acb0887 100644
--- a/util/grub-probe.c
+++ b/util/grub-probe.c
@@ -94,6 +94,8 @@ probe_partmap (grub_disk_t disk)
 static int
 probe_raid_level (grub_disk_t disk)
 {
+  if (!disk)
+         return -1;
   if (disk->dev->id != GRUB_DISK_DEVICE_RAID_ID)
     return -1;


Someone may want to look into why probe_raid_level can get passed an invalid disk pointer, but the combo of these two patches makes it work properly. I don't know if it's possible to have a disk pointer without a dev pointer, but that's a question for people with some deeper knowledge.

This may fix some of the other bug reports - I see stacktraces crossing
grub_device_iterate which tells me that this is a potential corruption in all of them.



--
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org

Reply via email to