From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with archive (Exim 4.43) id 1MHgQb-0001tG-RQ for mharc-grub-devel@gnu.org; Fri, 19 Jun 2009 11:54:57 -0400 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1MHgQa-0001sr-Li for grub-devel@gnu.org; Fri, 19 Jun 2009 11:54:56 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1MHgQW-0001qv-QC for grub-devel@gnu.org; Fri, 19 Jun 2009 11:54:56 -0400 Received: from [199.232.76.173] (port=44389 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1MHgQW-0001qe-J0 for grub-devel@gnu.org; Fri, 19 Jun 2009 11:54:52 -0400 Received: from mail-bw0-f216.google.com ([209.85.218.216]:42766) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1MHgQV-0000PD-Pb for grub-devel@gnu.org; Fri, 19 Jun 2009 11:54:52 -0400 Received: by bwz12 with SMTP id 12so401450bwz.42 for ; Fri, 19 Jun 2009 08:54:50 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:mime-version:received:in-reply-to:references :date:message-id:subject:from:to:content-type; bh=GWdmZw1zoA2Dk7nJJ1TIeBbmqY5rWvGaFcoSh8mq/Ns=; b=P4UW4lTiQS7WRJROC3EBAOoQtvg05qVREkpvu2jVvM7dipLRLmUkVac1d6IQHppTPH Nr9WgKU33s0UfA7CsJFfpS+RAYm6bFywn9jXdgaou/JPujOUeuW5xQTM9EJdieFeqLp6 KnUV6GeskYTuCLk/ZGMkEA6MFvUWU6da/ao0c= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :content-type; b=VGjNz1HKk60aSq1F7c8+65Di8Tsfr9+rkggGW360aFPNe+01gDPOzJlLD9tsTquojf FvaJpeLY2pq0e10idqfPRYZ82EO5ffoXjUq7E6hAz5gmODh+z1856M9LxsBtYeH7Hsyd 2KSJUD940dOdsJ1RQ0CGZAo13OQmpAjcMw/EA= MIME-Version: 1.0 Received: by 10.204.102.14 with SMTP id e14mr2681558bko.183.1245426890584; Fri, 19 Jun 2009 08:54:50 -0700 (PDT) In-Reply-To: <1245424852.3431.30.camel@mj> References: <1245424852.3431.30.camel@mj> Date: Fri, 19 Jun 2009 17:54:50 +0200 Message-ID: From: "Vladimir 'phcoder' Serbinenko" To: The development of GRUB 2 Content-Type: multipart/alternative; boundary=00163662e5c668343a046cb58b08 X-detected-operating-system: by monty-python.gnu.org: GNU/Linux 2.6 (newer, 2) Subject: Re: [PATCH] fix SigSegV and hang with grub-emu-usb X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.5 Precedence: list Reply-To: The development of GRUB 2 List-Id: The development of GRUB 2 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 19 Jun 2009 15:54:56 -0000 --00163662e5c668343a046cb58b08 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit On Fri, Jun 19, 2009 at 5:20 PM, Pavel Roskin wrote: > On Fri, 2009-06-19 at 16:58 +0200, Vladimir 'phcoder' Serbinenko wrote: > > Hello when testing grub-emu with USB support I stumbled across several > problems > > 1) compile time warning of undefined grub_usb_libinit > > 2) When launched under normal user it crashed > > 3) When launched as superuser it hanged on ls > > Here is the fix. Formatting omitted for readability > > It's not clear which changes are responsible for fixing which problems. > Please post separate patches if you want a meaningful review. > 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 @@ -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; @@ -306,7 +307,6 @@ grub_scsi_open (const char *name, grub_disk_t disk) return GRUB_ERR_NONE; } - } grub_free (scsi); Counterpart of first hunk diff --git a/disk/usbms.c b/disk/usbms.c index 3c7ebaf..f67f918 100644 --- 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 @@ -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. diff --git a/include/grub/usb.h b/include/grub/usb.h index 8dd3b6e..d6d9a3e 100644 --- a/include/grub/usb.h +++ b/include/grub/usb.h @@ -204,4 +204,9 @@ grub_usb_get_config_interface (struct grub_usb_desc_config *config) return interf; } +#ifdef GRUB_UTIL +grub_err_t grub_libusb_init (void); +grub_err_t grub_libusb_fini (void); +#endif + #endif /* GRUB_USB_H */ diff --git a/util/grub-emu.c b/util/grub-emu.c index c133dbe..2621d18 100644 --- a/util/grub-emu.c +++ b/util/grub-emu.c @@ -39,6 +39,10 @@ #include +#if HAVE_USB_H +#include +#endif + /* Used for going back to the main function. */ jmp_buf main_env; @@ -223,6 +227,10 @@ main (int argc, char *argv[]) if (setjmp (main_env) == 0) grub_main (); +#if HAVE_USB_H + grub_libusb_fini (); +#endif + grub_fini_all (); grub_machine_fini (); Previous hunks just fixed warnings diff --git a/util/usb.c b/util/usb.c index e1d8c71..4ca1c10 100644 --- 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 > > 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 > > -- > Regards, > Pavel Roskin > > > _______________________________________________ > Grub-devel mailing list > Grub-devel@gnu.org > http://lists.gnu.org/mailman/listinfo/grub-devel > -- Regards Vladimir 'phcoder' Serbinenko --00163662e5c668343a046cb58b08 Content-Type: text/html; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable

On Fri, Jun 19, 2009 at 5:20 PM, Pavel R= oskin <proski@gnu.or= g> wrote:
On Fri, 2009-06-19 at 16:58 +0200, Vladimir 'phcoder&= #39; Serbinenko wrote:
> Hello when testing grub-emu with USB support I stumbled across =A0seve= ral problems
> 1) compile time warning of undefined grub_usb_libinit
> 2) When launched under normal user it crashed
> 3) When launched as superuser it hanged on ls
> Here is the fix. Formatting omitted for readability

It's not clear which changes are responsible for fixing which pro= blems.
Please post separate patches if you want a meaningful review.
I thought it was clear. Here is an explanation hunk by hu= nk:
diff --git a/disk/scsi.c b/disk/scsi.c
index 046dcb8..312d58a 100= 644
--- a/disk/scsi.c
+++ b/disk/scsi.c
@@ -246,8 +246,9 @@ grub_s= csi_open (const char *name, grub_disk_t disk)
=A0
=A0=A0 for (p =3D grub_scsi_dev_list; p; p =3D p->next)
=A0=A0= =A0=A0 {
-=A0=A0=A0=A0=A0 if (! p->open (name, scsi))
-=A0=A0 =A0{=
+=A0=A0=A0=A0=A0 if (p->open (name, scsi))
+=A0=A0 =A0continue;+
=A0=A0=A0 =A0=A0 disk->id =3D (unsigned long) "scsi"; /= * XXX */
=A0=A0=A0 =A0=A0 disk->data =3D scsi;
=A0=A0=A0 =A0=A0 scsi->dev = =3D p;

This is purely stilistic to avoid unnecessarily long if
@@ -266,7 +267,7 @@ grub_scsi_open (const char *name, grub_disk_t disk)=A0=A0=A0 =A0=A0=A0=A0 {
=A0=A0=A0 =A0=A0=A0=A0=A0=A0 grub_free (scsi)= ;
=A0=A0=A0 =A0=A0=A0=A0=A0=A0 grub_dprintf ("scsi", "inquiry = failed\n");
-=A0=A0 =A0=A0=A0=A0=A0=A0 return grub_errno;
+=A0= =A0 =A0=A0 return err;
=A0=A0=A0 =A0=A0=A0=A0 }
=A0
=A0=A0=A0 =A0= =A0 grub_dprintf ("scsi", "inquiry: devtype=3D0x%02x removab= le=3D%d\n",
Error wasn't propagated which caused double closing which resulted in s= igsegv. With another hunk (adding grub-error) this hunk wouldn't be nec= essary but I consider construction
err =3D ....;
if (err)
=A0 retu= rn err;
more logical than
err =3D ....;
if (err)
=A0 return grub_errno;

@@ -306,7 +307,6 @@ grub_scsi_open (const char *name, grub_disk_t disk)=
=A0
=A0=A0=A0 =A0=A0 return GRUB_ERR_NONE;
=A0=A0=A0 =A0}
-=A0= =A0=A0 }
=A0
=A0=A0 grub_free (scsi);
=A0Counterpart of first hunk=
diff --git a/disk/usbms.c b/disk/usbms.c
index 3c7ebaf..f67f918 100644
--- a/disk/usbms.c
+++ b/disk/usbms.c@@ -226,7 +226,7 @@ grub_usbms_transfer (struct grub_scsi *scsi, grub_siz= e_t cmdsize, char *cmd,
=A0
=A0 retry:
=A0=A0 if (retrycnt =3D=3D = 0)
-=A0=A0=A0 return err;
+=A0=A0=A0 return grub_error (GRUB_ERR_IO, "USB Mass Storage stalled&q= uot;);
=A0
=A0=A0 /* Setup the request.=A0 */
=A0=A0 grub_memset (= &cbw, 0, sizeof (cbw));

when retry numbers failed returned error= was ERR_NONE even if nothing was read
@@ -246,6 +246,7 @@ grub_usbms_transfer (struct grub_scsi *scsi, grub_size_= t cmdsize, char *cmd,
=A0=A0=A0=A0=A0=A0 if (err =3D=3D GRUB_USB_ERR_STA= LL)
=A0=A0=A0 =A0{
=A0=A0=A0 =A0=A0 grub_usb_clear_halt (dev->dev,= dev->out->endp_addr);
+=A0=A0 =A0=A0 retrycnt--;
=A0=A0=A0 =A0=A0 goto retry;
=A0=A0=A0 =A0= }
=A0=A0=A0=A0=A0=A0 return grub_error (GRUB_ERR_IO, "USB Mass Stor= age request failed");;
retrycnt wasn't decreased which caused g= rub2 to retry infinitely hence a hang.
diff --git a/include/grub/usb.h b/include/grub/usb.h
index 8dd3b6e..d6d9= a3e 100644
--- a/include/grub/usb.h
+++ b/include/grub/usb.h
@@ -2= 04,4 +204,9 @@ grub_usb_get_config_interface (struct grub_usb_desc_config *= config)
=A0=A0 return interf;
=A0}
=A0
+#ifdef GRUB_UTIL
+grub_err_t gr= ub_libusb_init (void);
+grub_err_t grub_libusb_fini (void);
+#endif+
=A0#endif /* GRUB_USB_H */
diff --git a/util/grub-emu.c b/util/gr= ub-emu.c
index c133dbe..2621d18 100644
--- a/util/grub-emu.c
+++ b/util/grub-e= mu.c
@@ -39,6 +39,10 @@
=A0
=A0#include <grub_emu_init.h>=A0
+#if HAVE_USB_H
+#include <grub/usb.h>
+#endif
+
= =A0/* Used for going back to the main function.=A0 */
=A0jmp_buf main_env;
=A0
@@ -223,6 +227,10 @@ main (int argc, char *a= rgv[])
=A0=A0 if (setjmp (main_env) =3D=3D 0)
=A0=A0=A0=A0 grub_main = ();
=A0
+#if HAVE_USB_H
+=A0 grub_libusb_fini ();
+#endif
+<= br>=A0=A0 grub_fini_all ();
=A0
=A0=A0 grub_machine_fini ();
Previous hunks just fixed warningsdiff --git a/util/usb.c b/util/usb.c
index e1d8c71..4ca1c10 100644
= --- a/util/usb.c
+++ b/util/usb.c
@@ -51,6 +51,7 @@ grub_libusb_devic= es (void)
=A0=A0=A0=A0=A0=A0 for (usbdev =3D bus->devices; usbdev; usbdev =3D usbd= ev->next)
=A0=A0=A0 =A0{
=A0=A0=A0 =A0=A0 struct usb_device_descri= ptor *desc =3D &usbdev->descriptor;
+=A0=A0 =A0=A0 grub_err_t err= ;
=A0
=A0=A0=A0 =A0=A0 if (! desc->bcdUSB)
=A0=A0=A0 =A0=A0=A0=A0 continue;
@@ -62,7 +63,9 @@ grub_libusb_devices (= void)
=A0=A0=A0 =A0=A0 dev->data =3D usbdev;
=A0
=A0=A0=A0 =A0= =A0 /* Fill in all descriptors.=A0 */
-=A0=A0 =A0=A0 grub_usb_device_ini= tialize (dev);
+=A0=A0 =A0=A0 err =3D grub_usb_device_initialize (dev);<= br> +=A0=A0 =A0=A0 if (err)
+=A0=A0 =A0=A0=A0=A0 continue;
=A0
=A0=A0= =A0 =A0=A0 /* Register the device.=A0 */
=A0=A0=A0 =A0=A0 grub_usb_devs[= last++] =3D dev;
When device couldn'r be initialized (e.g. because o= f privilege problem) it was still added to list. Subsequent access created = sigsegv

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 (vo= id);
void grub_lvm_init (void);
void grub_lvm_fini (void);
#endif<= br> file: include/grub/disk.h

--
Regards,
Pavel Roskin


_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/grub-devel



--
Regards
Vladimir = 9;phcoder' Serbinenko
--00163662e5c668343a046cb58b08--