libusb: Properly wait for transfer cancellation on read_thread() exit

While triaging libusb bugs, I took an indepth look at:
https://github.com/libusbx/libusbx/issues/25

This has lead me to the conclusion that there are 2 issues with hidapi's
libusb code wrt waiting for the transfer cancellation on read_thread()
exit:

1) There is a race where hid_close() can successfully cancel the transfer
after a read_callback() has submitted it but before read_thread() checks
shutdown_thread.  If this race is hit, then the libusb_cancel_transfer()
in read_thread() will fail, causing read_thread() to not call
libusb_handle_events() to complete the cancelled transfer.  hid_close()
will then free the transfer, and if later on libusb_handle_events() gets
called on the same context, it will try to complete the now freed
transfer.  This is what I believe leads to the segfault described in
https://github.com/libusbx/libusbx/issues/25

2) hidapi uses one read_thread() per hid_device, so if there are multiple
hid_devices then there are multiple threads calling
libusb_handle_events(), in this case there is no guarantee that a single
libusb_handle_events() call will successfully lead to the cancelled
transfer being completed.  If the transfer completion is already handled
by another read_thread() and there are no other events, then the
libusb_handle_events() call will hang, and thus the pthread_join() and
thus hidapi_close() will hang.

As number 2 is a generic problem found in more libusb apps, libusb has
gotten a new API called libusb_handle_events_completed(), which takes an
extra pointer to an int, whose contents must be set to nonzero on
completion by the callback, which allows waiting for the completion of a
specific transfer in a race-free manner.

This patch switches the waiting for the transfer's final completion to
using libusb_handle_events_completed(), thereby fixing both issues.  Note
the while is necessary since libusb_handle_events_completed(), like
libusb_handle_events(), will return as soon as it has handled *any* event.
The difference with libusb_handle_events_completed() is that once it has
all the necessary internal libusb locks, it checks the contents of the
completed parameter, and will bail if that has become nonzero without
waiting for further events.

Signed-off-by: Hans de Goede <[email protected]>

configure.ac

@@ -68,7 +68,7 @@ case $host in
 
 	# HIDAPI/libusb libs
 	AC_CHECK_LIB([rt], [clock_gettime], [LIBS_LIBUSB_PRIVATE+=" -lrt"], [hidapi_lib_error librt])
-	PKG_CHECK_MODULES([libusb], [libusb-1.0], true, [hidapi_lib_error libusb-1.0])
+	PKG_CHECK_MODULES([libusb], [libusb-1.0 >= 1.0.9], true, [hidapi_lib_error libusb-1.0])
 	LIBS_LIBUSB_PRIVATE+=" $libusb_LIBS"
 	CFLAGS_LIBUSB+=" $libusb_CFLAGS"
 	;;

libusb/hid.c

@@ -105,6 +105,7 @@ struct hid_device_ {
 	pthread_cond_t condition;
 	pthread_barrier_t barrier; /* Ensures correct startup sequence */
 	int shutdown_thread;
+	int cancelled;
 	struct libusb_transfer *transfer;
 
 	/* List of received input reports. */
@@ -683,10 +684,12 @@ static void read_callback(struct libusb_transfer *transfer)
 	}
 	else if (transfer->status == LIBUSB_TRANSFER_CANCELLED) {
 		dev->shutdown_thread = 1;
+		dev->cancelled = 1;
 		return;
 	}
 	else if (transfer->status == LIBUSB_TRANSFER_NO_DEVICE) {
 		dev->shutdown_thread = 1;
+		dev->cancelled = 1;
 		return;
 	}
 	else if (transfer->status == LIBUSB_TRANSFER_TIMED_OUT) {
@@ -701,6 +704,7 @@ static void read_callback(struct libusb_transfer *transfer)
 	if (res != 0) {
 		LOG("Unable to submit URB. libusb error code: %d\n", res);
 		dev->shutdown_thread = 1;
+		dev->cancelled = 1;
 	}
 }
 
@@ -750,10 +754,10 @@ static void *read_thread(void *param)
 
 	/* Cancel any transfer that may be pending. This call will fail
 	   if no transfers are pending, but that's OK. */
-	if (libusb_cancel_transfer(dev->transfer) == 0) {
-		/* The transfer was cancelled, so wait for its completion. */
-		libusb_handle_events(usb_context);
-	}
+	libusb_cancel_transfer(dev->transfer);
+
+	while (!dev->cancelled)
+		libusb_handle_events_completed(usb_context, &dev->cancelled);
 
 	/* Now that the read thread is stopping, Wake any threads which are
 	   waiting on data (in hid_read_timeout()). Do this under a mutex to