dfu_util: bugfix for crash

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

dfu_util: bugfix for crash

Satz Klauer
Hi,

I found a small bug in dfu_util, file main.c function find_dfu_if() line 86:

There a function libusb_get_config_descriptor(dev, cfg_idx, &cfg); is
called without checking the return code. In case it fails cfg is
invalid and will lead to an crash some lines later.

Something like

rc=libusb_get_config_descriptor(dev, cfg_idx, &cfg);
if (rc<0) return rc;
/* in some cases, noticably FreeBSD if uid != 0,
 * the configuration descriptors are empty */

would fix that problem.

Elmi

_______________________________________________
devel mailing list
[hidden email]
https://lists.openmoko.org/mailman/listinfo/devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCH dfu-util] main: Check return value on all libusb calls

Tormod Volden
From: Tormod Volden <[hidden email]>

Thanks to Uwe Bonnes and Satz Klauer for pointing it out.

Also adjusted a couple of related diagnostic messages.

Signed-off-by: Tormod Volden <[hidden email]>
---

Uwe once actually sent me a patch for what Satz pointed out,
but it needed to be refreshed for the current code.

Cheers,
Tormod

 src/main.c |   42 ++++++++++++++++++++++++++++--------------
 1 files changed, 28 insertions(+), 14 deletions(-)

diff --git a/src/main.c b/src/main.c
index 4c96f88..eebce1e 100644
--- a/src/main.c
+++ b/src/main.c
@@ -76,10 +76,14 @@ static int find_dfu_if(libusb_device *dev,
  int rc;
 
  memset(dfu_if, 0, sizeof(*dfu_if));
- libusb_get_device_descriptor(dev, &desc);
+ rc = libusb_get_device_descriptor(dev, &desc);
+ if (rc)
+ return rc;
  for (cfg_idx = 0; cfg_idx < desc.bNumConfigurations;
      cfg_idx++) {
- libusb_get_config_descriptor(dev, cfg_idx, &cfg);
+ rc = libusb_get_config_descriptor(dev, cfg_idx, &cfg);
+ if (rc)
+ return rc;
  /* in some cases, noticably FreeBSD if uid != 0,
  * the configuration descriptors are empty */
  if (!cfg)
@@ -204,7 +208,8 @@ static int get_alt_name(struct dfu_if *dfu_if, unsigned char *name)
  ret = -1;
  if (alt_name_str_idx) {
  if (!dfu_if->dev_handle)
- libusb_open(dfu_if->dev, &dfu_if->dev_handle);
+ if (libusb_open(dfu_if->dev, &dfu_if->dev_handle))
+ dfu_if->dev_handle = NULL;
  if (dfu_if->dev_handle)
  ret = libusb_get_string_descriptor_ascii(
  dfu_if->dev_handle, alt_name_str_idx,
@@ -299,7 +304,8 @@ static int iterate_dfu_devices(libusb_context *ctx, struct dfu_if *dif,
     (libusb_get_bus_number(dev) != dif->bus ||
      libusb_get_device_address(dev) != dif->devnum))
  continue;
- libusb_get_device_descriptor(dev, &desc);
+ if (libusb_get_device_descriptor(dev, &desc))
+ continue;
  if (dif && (dif->flags & DFU_IFF_VENDOR) &&
     desc.idVendor != dif->vendor)
  continue;
@@ -437,12 +443,17 @@ static int usb_get_any_descriptor(struct libusb_device_handle *dev_handle,
   uint8_t desc_index,
   unsigned char *resbuf, int res_len)
 {
- struct libusb_device *dev = libusb_get_device(dev_handle);
+ struct libusb_device *dev;
  struct libusb_config_descriptor *config;
  int ret;
  uint16_t conflen;
  unsigned char *cbuf;
 
+ dev = libusb_get_device(dev_handle);
+ if (!dev) {
+ fprintf(stderr, "Error: Broken device handle\n");
+ return -1;
+ }
  /* Get the total length of the configuration descriptors */
  ret = libusb_get_active_config_descriptor(dev, &config);
  if (ret == LIBUSB_ERROR_NOT_FOUND) {
@@ -767,10 +778,10 @@ int main(int argc, char **argv)
 
  /* We have exactly one device. Its libusb_device is now in dif->dev */
 
- printf("Opening DFU USB device... ");
- libusb_open(dif->dev, &dif->dev_handle);
- if (!dif->dev_handle) {
- fprintf(stderr, "Cannot open device \n");
+ printf("Opening DFU capable USB device... ");
+ ret = libusb_open(dif->dev, &dif->dev_handle);
+ if (ret || !dif->dev_handle) {
+ fprintf(stderr, "Cannot open device\n");
  exit(1);
  }
 
@@ -903,7 +914,7 @@ int main(int argc, char **argv)
  }
  if (!ret) {
  fprintf(stderr,
-    "Can't resolve path after RESET?\n");
+    "Cannot resolve path after RESET?\n");
  exit(1);
  }
  }
@@ -921,9 +932,9 @@ int main(int argc, char **argv)
  if (!get_first_dfu_device(ctx, dif))
  exit(3);
 
- printf("Opening USB Device...\n");
- libusb_open(dif->dev, &dif->dev_handle);
- if (!dif->dev_handle) {
+ printf("Opening DFU USB Device...\n");
+ ret = libusb_open(dif->dev, &dif->dev_handle);
+ if (ret || !dif->dev_handle) {
  fprintf(stderr, "Cannot open device\n");
  exit(1);
  }
@@ -1093,7 +1104,10 @@ status_again:
  }
  /* DFU specification */
  struct libusb_device_descriptor desc;
- libusb_get_device_descriptor(dif->dev, &desc);
+ if (libusb_get_device_descriptor(dif->dev, &desc)) {
+ fprintf(stderr, "Error: Failed to get device descriptor\n");
+ exit(1);
+ }
  if (transfer_size < desc.bMaxPacketSize0) {
  transfer_size = desc.bMaxPacketSize0;
  printf("Adjusted transfer size to %i\n", transfer_size);
--
1.7.0.4


_______________________________________________
devel mailing list
[hidden email]
https://lists.openmoko.org/mailman/listinfo/devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH dfu-util] main: Check return value on all libusb calls

Stefan Schmidt-2
Hello.

On Wed, 2011-11-16 at 23:43, Tormod Volden wrote:

> From: Tormod Volden <[hidden email]>
>
> Thanks to Uwe Bonnes and Satz Klauer for pointing it out.
>
> Also adjusted a couple of related diagnostic messages.
>
> Signed-off-by: Tormod Volden <[hidden email]>
> ---
>
> Uwe once actually sent me a patch for what Satz pointed out,
> but it needed to be refreshed for the current code.

Thanks, applied.

As a side note we now have a basic continous integration setup for
dfu-util. At least some build testing on freeBSD 64 and 32 Bit Linux
via jenkins:
http://jenkins.osmocom.org/jenkins/job/dfu-util/

Together with my 64 Bit Linux system is gives at least a fast heads up
when something gets broken.

SCM gets polled every five minutes. So Worst time waiting after a push
is 5 minutes. As example this patch was job #6:
http://jenkins.osmocom.org/jenkins/job/dfu-util/6/

regards
Stefan Schmidt

_______________________________________________
devel mailing list
[hidden email]
https://lists.openmoko.org/mailman/listinfo/devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: dfu_util: bugfix for crash

Stefan Schmidt-2
In reply to this post by Satz Klauer
Hello.

On Wed, 2011-11-16 at 18:45, Satz Klauer wrote:
>
> I found a small bug in dfu_util, file main.c function find_dfu_if() line 86:

Did you hit this problem somewhere?

> There a function libusb_get_config_descriptor(dev, cfg_idx, &cfg); is
> called without checking the return code. In case it fails cfg is
> invalid and will lead to an crash some lines later.
>
> Something like
>
> rc=libusb_get_config_descriptor(dev, cfg_idx, &cfg);
> if (rc<0) return rc;
> /* in some cases, noticably FreeBSD if uid != 0,
>  * the configuration descriptors are empty */
>
> would fix that problem.

Thanks for bringing this to our attention. Tormod prepared a patch for
it and I applied it. Does this covers your concers?

http://cgit.openezx.org/dfu-util/commit/?id=60bb907b9b30f74bbd9ddafbba6064e1fbbd52ad

New and shiny jenkins autobuild results can now be foundhere btw:
http://jenkins.osmocom.org/jenkins/job/dfu-util/

regards
Stefan Schmidt

_______________________________________________
devel mailing list
[hidden email]
https://lists.openmoko.org/mailman/listinfo/devel
Loading...