Re: [PATCH] fix SigSegV and hang with grub-emu-usb

[Pavel Roskin <proski@gnu.org>]

On Fri, 2009-06-19 at 17:54 +0200, Vladimir 'phcoder' Serbinenko wrote:

> I thought it was clear. Here is an explanation hunk by hunk:
> diff --git a/disk/scsi.c b/disk/scsi.c
> index 046dcb8..312d58a 100644
> --- a/disk/scsi.c
> +++ b/disk/scsi.c
> @@ -246,8 +246,9 @@ grub_scsi_open (const char *name, grub_disk_t
> disk)
>  
>    for (p = grub_scsi_dev_list; p; p = p->next)
>      {
> -      if (! p->open (name, scsi))
> -    {
> +      if (p->open (name, scsi))
> +    continue;
> +
>        disk->id = (unsigned long) "scsi"; /* XXX */
>        disk->data = scsi;
>        scsi->dev = p;
> 
> This is purely stilistic to avoid unnecessarily long if

It can be committed without review.

> @@ -266,7 +267,7 @@ grub_scsi_open (const char *name, grub_disk_t
> disk)
>          {
>            grub_free (scsi);
>            grub_dprintf ("scsi", "inquiry failed\n");
> -          return grub_errno;
> +      return err;
>          }
>  
>        grub_dprintf ("scsi", "inquiry: devtype=0x%02x removable=%d\n",
> Error wasn't propagated which caused double closing which resulted in
> sigsegv. With another hunk (adding grub-error) this hunk wouldn't be
> necessary but I consider construction
> err = ....;
> if (err)
>   return err;
> more logical than
> err = ....;
> if (err)
>   return grub_errno;

OK, I understand you tried USB mass storage devices.

I believe the paramount here is consistency.  There are several places
in the code where grub_errno is returned.  In one place, grub_error() is
returned.  It's important to fix all places at once.

Also, please check other .open functions in other disk drivers.  In
disk/fs_uuid.c, grub_error() is used.  The same is in disk/host.c.

I see the standard is grub_error().  Let's do it for SCSI as well.

> --- a/disk/usbms.c
> +++ b/disk/usbms.c
> @@ -226,7 +226,7 @@ grub_usbms_transfer (struct grub_scsi *scsi,
> grub_size_t cmdsize, char *cmd,
>  
>   retry:
>    if (retrycnt == 0)
> -    return err;
> +    return grub_error (GRUB_ERR_IO, "USB Mass Storage stalled");
>  
>    /* Setup the request.  */
>    grub_memset (&cbw, 0, sizeof (cbw));
> 
> when retry numbers failed returned error was ERR_NONE even if nothing
> was read

Something is wrong with the logic in that function.  retrycnt is only
changed in one place, and if it hits zero, we don't go to the retry
label.  I think the condition can be removed.  I don't see how your
change could have fixed anything in the behavior.

> @@ -246,6 +246,7 @@ grub_usbms_transfer (struct grub_scsi *scsi,
> grub_size_t cmdsize, char *cmd,
>        if (err == GRUB_USB_ERR_STALL)
>      {
>        grub_usb_clear_halt (dev->dev, dev->out->endp_addr);
> +      retrycnt--;
>        goto retry;
>      }
>        return grub_error (GRUB_ERR_IO, "USB Mass Storage request
> failed");;
> retrycnt wasn't decreased which caused grub2 to retry infinitely hence
> a hang. 

There are many instances of "goto retry" where you don't decrement
retrycnt.  Then let's decrement retrycnt in the beginning.

Generally, when making a change, please have a look if it needs to be
done elsewhere.

> --- a/util/usb.c
> +++ b/util/usb.c
> @@ -51,6 +51,7 @@ grub_libusb_devices (void)
>        for (usbdev = bus->devices; usbdev; usbdev = usbdev->next)
>      {
>        struct usb_device_descriptor *desc = &usbdev->descriptor;
> +      grub_err_t err;
>  
>        if (! desc->bcdUSB)
>          continue;
> @@ -62,7 +63,9 @@ grub_libusb_devices (void)
>        dev->data = usbdev;
>  
>        /* Fill in all descriptors.  */
> -      grub_usb_device_initialize (dev);
> +      err = grub_usb_device_initialize (dev);
> +      if (err)
> +        continue;
>  
>        /* Register the device.  */
>        grub_usb_devs[last++] = dev;
> When device couldn'r be initialized (e.g. because of privilege
> problem) it was still added to list. Subsequent access created sigsegv

Fine with me.

>         Regarding the compile warning fix, I would try to make
>         grub_libusb_init() and grub_libusb_fini() appear in
>         grub_emu_init.h
>         rather than declare them elsewhere.
> I was inspired by previous example of disk subsystems:
> #ifdef GRUB_UTIL
> void grub_raid_init (void);
> void grub_raid_fini (void);
> void grub_lvm_init (void);
> void grub_lvm_fini (void);
> #endif
> file: include/grub/disk.h 

I would check why it was needed.

-- 
Regards,
Pavel Roskin