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

[Vladimir 'phcoder' Serbinenko]

committed

On Fri, Jun 19, 2009 at 8:44 PM, Vladimir 'phcoder'
Serbinenko<address@hidden> wrote:
>
>
> On Fri, Jun 19, 2009 at 6:52 PM, Pavel Roskin <address@hidden> wrote:
>>
>> 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.
>
> I don't understand what do you mean. grub_error () which don't come from
> previous function
>>
>> 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.
>
> Wel it didn't fixed the logic completely just one case when it was wrong.
> Sorry that patch was low-quality. My goal was to enable everything by
> default and the bugs in long-unmaintained libusb code weren't something I
> wanted to spend time on.
>>
>> > @@ -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;
>
> It was clearing grub_errno
>>
>>
>> >         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.
>
> It seems it's unnecessary. I removed them and it didn't generate any
> warnings. Now I followed your recommendation and they build system with my
> previous fixes picked it right
>>
>> --
>> Regards,
>> Pavel Roskin
>>
>>
>> _______________________________________________
>> Grub-devel mailing list
>> address@hidden
>> http://lists.gnu.org/mailman/listinfo/grub-devel
>
>
>
> --
> Regards
> Vladimir 'phcoder' Serbinenko
>
> Personal git repository: http://repo.or.cz/w/grub2/phcoder.git
>



-- 
Regards
Vladimir 'phcoder' Serbinenko

Personal git repository: http://repo.or.cz/w/grub2/phcoder.git