Re: segfault in grub_device_iterate on 64bit platforms

[Dan Merillat]

Colin Watson wrote:
> On Tue, Jan 19, 2010 at 08:23:44PM -0500, Dan Merillat wrote:
> > And here's a freebie to avoid a null pointer deref that -lefence pointed
> > out:

> (Nit: stick to the prevailing whitespace style.)

:)  Glad that's the worst problem with my submission.

> grub_lvm_memberlist just constructs its return value from lv->vg->pvs,
> so we'll need to look at how that's constructed.  The only time pv->disk
> is assigned a non-NULL value is in grub_lvm_scan_device:
>
>   /* Match the device we are currently reading from with the right
>      PV. */
>   if (vg->pvs)
>     for (pv = vg->pvs; pv; pv = pv->next)
>       {
>         if (! grub_memcmp (pv->id, pv_id, GRUB_LVM_ID_STRLEN))
>           {
>             /* This could happen to LVM on RAID, pv->disk points to the
>                raid device, we shouldn't change it.  */
>             if (! pv->disk)
>               pv->disk = grub_disk_open (name);
>             break;
>           }
>       }
>
> So it's possible for pv->disk to be NULL if that PV doesn't have an LVM
> signature (as I interpret this).  That's either an error condition or an
> indication that we aren't managing to understand LVM well enough.  If
> you can reproduce this it would be useful if you could clarify which.

Ok, here goes - grub_lvm_scan_device is called once ("md0"), it finds
the LVM signature, searches the vg list to see if we've seen this vg
before, creates a new vg, pv list (disk = NULL) etc.

Then it attaches the disk with this name to the appropriate pv entry in
that code snippet you posted.

So the only way for a PV->disk to be attached is if it is called by name
in grub_lvm_scan_device().

I'm running the following setup:
/dev/md0 pv0 contains lv_root
/dev/sdb2 pv1 contains lv_test1, lv_test2, etc.

invoke as:
# grub-probe -t abstraction /

Grub discovers / is on /dev/md0, probes md0 for LVM information, never
looks at other disks, segfaults on a null pointer dereference because
pv1 disk = NULL.

So either you can ignore it via my patch, or add a loop to get disks by
ID and add their information to the pvs.

Does my analysis make sense?