Re: segfault in grub_device_iterate on 64bit platforms

[Colin Watson]

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:
>
> 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;

(Nit: stick to the prevailing whitespace style.)

> 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.

probe_raid_level is called in two places: one on dev->disk from
grub_device_open, and one on each member list->disk from
dev->disk->dev->memberlist (dev->disk).  grub_device_open guarantees
either !dev or dev->disk, so we must be looking at the latter, and the
membership function pointer here is always grub_lvm_memberlist.

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.

In the meantime, my gut feel is that grub_lvm_memberlist is reporting
the member list accurately, and since grub-probe is the thing that cares
whether the disk member is non-NULL, it should also test that.  It
doesn't much matter whether we do this in probe or in probe_raid_level.

Which is all a laborious way of saying that I think your patch is OK,
although I'm going to add a comment to explain why.  Thanks - I'll apply
this and your other patch in just a moment.

-- 
Colin Watson                                       address@hidden