mcuee/libusb-win32

Crash in reads with short timeout -- bug in _usb_cancel_io()

mcuee opened this issue · 1 comments

mcuee commented

https://sourceforge.net/p/libusb-win32/mailman/message/36291183/

When calling e.g. usb_interrupt_read() that times out it will call:

  1. usb_interrupt_read
  2. _usb_transfer_sync
  3. usb_reap_async
  4. _usb_reap_async
  5. _usb_cancel_io

This is where it goes south. To cancel the I/O operation it calls:

_usb_abort_ep() to send the LIBUSB_IOCTL_ABORT_ENDPOINT to the driver
synchroniously. Even though the LIBUSB_IOCTL_ABORT_ENDPOINT is sent
synchroniously to the driver, the driver just forwards the request to
"usbd" further down the IRP chain as a "reset endpoint" request, which
has asynchrinous elements to it. So all-in-all you cannot regard the
request as cancelled when returning from _usb_abort_ep().

To fix this it was introduced that using WaitForSingleObject() would be
used to wait for the cancel to go through.

The first revision that had this was:

6eaec78aed8e34e1aaf2e835b7

It was later reformatted into being a part of _usb_cancel_io().

However, WaitForSingleObject() is not called correctly. If you want to
be sure that the cancel has been executed, you must call
WaitForSingleObject() with a timeout that is large enough to ensure
that the request is processed. In many cases on a loaded system, this
ends tragically.

mcuee commented

Potential fix

Let's check the return code.  What about this:
static int _usb_cancel_io(usb_context_t *context)
{
     int ret;
     ret = _usb_abort_ep(context->dev, context->req.endpoint.endpoint);
     if (ret == 0)
     {
         WaitForSingleObject(context->ol.hEvent, 1000);
     }
     else
     {
         WaitForSingleObject(context->ol.hEvent, 0);
     }
     return ret;
}

Use 1000 ms I think.  That should always be plenty and it will keep it 
from dead locking.

Regards,
Travis