Crash in reads with short timeout - race.

Hi libusb,

If anyone could review and eventually apply the fix below it would be
great, as i am probably not the only only getting this rare crash.

I still use the old libusb-win32 (still works great by the way) on some
of my older projects, and i have had an annoying crash bug which i
never got fixed.

I decided to use some time finding the "easter egg" that was causing
this. After lots of debugging, it turned out that there is a race in
the DLL.

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:

https://github.com/mcuee/libusb-win32/commit/6eaec78aed8e34e1aaf2e835b7
01497d352bc3e6#diff-b5691da501465b646e19ae693a116e5e

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.

A simple statement can crash, e.g.:

-- snip --

void test(void)
{
  uint8_t data[20];
  usb_interrupt_read(handle, ep_int_in, data, 20, 1);
}

-- snip --

The above code crashes in some cases, as the read is in some rare cases
executed *after* leaving usb_interrupt_read() and test(), which then
corrupts the local stack as "data" is not valid any more.

To fix this, simply use INFINITE as timeout, as you can *not* return
from _usb_cancel_io until the operation has been truely cancelled. It
is also described on MSDN for WaitForSingleObject() that the only was
to be sure is to call it with INFINITE.

I have tested the fix on lots of setups, and i think the fix is OK, and
does not seem to create other new side-effects.

It was also partially fixable by cancelling all I/O to the driver using
CancelIo(). However, this will cancel all I/O and will ruin other
running endpoints (async readers and such), so that will not work.
Newer version of windows have CancelIoEx() which can cancel a specific
I/O operation, but this is only available in 2008/win7 and forward, and
i need this to still run on xp/vista.

So this solution is the most compatible.

I have attached a patch.

Comments are welcome.

Thanks,

/pedro

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

_______________________________________________
Libusb-win32-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/libusb-win32-devel

Index: libusb/src/windows.c
===================================================================
--- libusb/src/windows.c	(revision 413)
+++ libusb/src/windows.c	(working copy)
@@ -1156,7 +1156,7 @@
 {
     int ret;
     ret = _usb_abort_ep(context->dev, context->req.endpoint.endpoint);
-    WaitForSingleObject(context->ol.hEvent, 0);
+    WaitForSingleObject(context->ol.hEvent, INFINITE);
     return ret;
 }