Transfer locking

At least on Linux, there were some possible races that could occur if
a transfer is cancelled from one thread while another thread is handling
an event for that transfer, or for if a transfer completes while it is
still being submitted from another thread, etc.

On the global level, transfers could be submitted and cancelled at the
same time.

Fix those issues with transfer-level locks.

3 files changed, 66 insertions, 17 deletions

diff --git a/libusb/io.c b/libusb/io.c
index b63c000..ef286dd 100644
--- a/libusb/io.c
+++ b/libusb/io.c
@@ -1072,6 +1072,7 @@ API_EXPORTED struct libusb_transfer *libusb_alloc_transfer(int iso_packets)
 
 	memset(itransfer, 0, alloc_size);
 	itransfer->num_iso_packets = iso_packets;
+	pthread_mutex_init(&itransfer->lock, NULL);
 	return __USBI_TRANSFER_TO_LIBUSB_TRANSFER(itransfer);
 }
 
@@ -1087,6 +1088,9 @@ API_EXPORTED struct libusb_transfer *libusb_alloc_transfer(int iso_packets)
  * It is legal to call this function with a NULL transfer. In this case,
  * the function will simply return safely.
  *
+ * It is not legal to free an active transfer (one which has been submitted
+ * and has not yet completed).
+ *
  * \param transfer the transfer to free
  */
 API_EXPORTED void libusb_free_transfer(struct libusb_transfer *transfer)
@@ -1099,6 +1103,7 @@ API_EXPORTED void libusb_free_transfer(struct libusb_transfer *transfer)
 		free(transfer->buffer);
 
 	itransfer = __LIBUSB_TRANSFER_TO_USBI_TRANSFER(transfer);
+	pthread_mutex_destroy(&itransfer->lock);
 	free(itransfer);
 }
 
@@ -1118,11 +1123,14 @@ API_EXPORTED int libusb_submit_transfer(struct libusb_transfer *transfer)
 		__LIBUSB_TRANSFER_TO_USBI_TRANSFER(transfer);
 	int r;
 
+	pthread_mutex_lock(&itransfer->lock);
 	itransfer->transferred = 0;
 	itransfer->flags = 0;
 	r = calculate_timeout(itransfer);
-	if (r < 0)
-		return LIBUSB_ERROR_OTHER;
+	if (r < 0) {
+		r = LIBUSB_ERROR_OTHER;
+		goto out;
+	}
 
 	add_to_flying_list(itransfer);
 	r = usbi_backend->submit_transfer(itransfer);
@@ -1132,6 +1140,8 @@ API_EXPORTED int libusb_submit_transfer(struct libusb_transfer *transfer)
 		pthread_mutex_unlock(&TRANSFER_CTX(transfer)->flying_transfers_lock);
 	}
 
+out:
+	pthread_mutex_unlock(&itransfer->lock);
 	return r;
 }
 
@@ -1156,10 +1166,12 @@ API_EXPORTED int libusb_cancel_transfer(struct libusb_transfer *transfer)
 	int r;
 
 	usbi_dbg("");
+	pthread_mutex_lock(&itransfer->lock);
 	r = usbi_backend->cancel_transfer(itransfer);
 	if (r < 0)
 		usbi_err(TRANSFER_CTX(transfer),
 			"cancel transfer failed error %d", r);
+	pthread_mutex_unlock(&itransfer->lock);
 	return r;
 }
 
@@ -1167,7 +1179,10 @@ API_EXPORTED int libusb_cancel_transfer(struct libusb_transfer *transfer)
  * This will invoke the user-supplied callback function, which may end up
  * freeing the transfer. Therefore you cannot use the transfer structure
  * after calling this function, and you should free all backend-specific
- * data before calling it. */
+ * data before calling it.
+ * Do not call this function with the usbi_transfer lock held. User-specified
+ * callback functions may attempt to directly resubmit the transfer, which
+ * will attempt to take the lock. */
 void usbi_handle_transfer_completion(struct usbi_transfer *itransfer,
 	enum libusb_transfer_status status)
 {
@@ -1208,7 +1223,9 @@ void usbi_handle_transfer_completion(struct usbi_transfer *itransfer,
 /* Similar to usbi_handle_transfer_completion() but exclusively for transfers
  * that were asynchronously cancelled. The same concerns w.r.t. freeing of
  * transfers exist here.
- */
+ * Do not call this function with the usbi_transfer lock held. User-specified
+ * callback functions may attempt to directly resubmit the transfer, which
+ * will attempt to take the lock. */
 void usbi_handle_transfer_cancellation(struct usbi_transfer *transfer)
 {
 	/* if the URB was cancelled due to timeout, report timeout to the user */
diff --git a/libusb/libusbi.h b/libusb/libusbi.h
index 9813c4d..6e9e2e0 100644
--- a/libusb/libusbi.h
+++ b/libusb/libusbi.h
@@ -242,6 +242,15 @@ struct usbi_transfer {
 	struct timeval timeout;
 	int transferred;
 	uint8_t flags;
+
+	/* this lock is held during libusb_submit_transfer() and
+	 * libusb_cancel_transfer() (allowing the OS backend to prevent duplicate
+	 * cancellation, submission-during-cancellation, etc). the OS backend
+	 * should also take this lock in the handle_events path, to prevent the user
+	 * cancelling the transfer from another thread while you are processing
+	 * its completion (presumably there would be races within your OS backend
+	 * if this were possible). */
+	pthread_mutex_t lock;
 };
 
 #define __USBI_TRANSFER_TO_LIBUSB_TRANSFER(transfer) \
@@ -733,6 +742,10 @@ struct usbi_os_backend {
 	 * This function should also be able to detect disconnection of the
 	 * device, reporting that situation with usbi_handle_disconnect().
 	 *
+	 * When processing an event related to a transfer, you probably want to
+	 * take usbi_transfer.lock to prevent races. See the documentation for
+	 * the usbi_transfer structure.
+	 *
 	 * Return 0 on success, or a LIBUSB_ERROR code on failure.
 	 */
 	int (*handle_events)(struct libusb_context *ctx,
diff --git a/libusb/os/linux_usbfs.c b/libusb/os/linux_usbfs.c
index bffd780..a151c8d 100644
--- a/libusb/os/linux_usbfs.c
+++ b/libusb/os/linux_usbfs.c
@@ -1749,6 +1749,7 @@ static int handle_bulk_completion(struct usbi_transfer *itransfer,
 	int urb_idx = urb - tpriv->urbs;
 	enum libusb_transfer_status status = LIBUSB_TRANSFER_COMPLETED;
 
+	pthread_mutex_lock(&itransfer->lock);
 	usbi_dbg("handling completion status %d of bulk urb %d/%d", urb->status,
 		urb_idx + 1, num_urbs);
 
@@ -1792,14 +1793,15 @@ static int handle_bulk_completion(struct usbi_transfer *itransfer,
 			if (tpriv->reap_action == CANCELLED) {
 				free(tpriv->urbs);
 				tpriv->urbs = NULL;
+				pthread_mutex_unlock(&itransfer->lock);
 				usbi_handle_transfer_cancellation(itransfer);
-				return 0;
+				goto out_unlock;
 			}
 			if (tpriv->reap_action != COMPLETED_EARLY)
 				status = LIBUSB_TRANSFER_ERROR;
-			goto out;
+			goto completed;
 		}
-		return 0;
+		goto out_unlock;
 	}
 
 	if (urb->status == 0 ||
@@ -1813,23 +1815,23 @@ static int handle_bulk_completion(struct usbi_transfer *itransfer,
 	case -EPIPE:
 		usbi_dbg("detected endpoint stall");
 		status = LIBUSB_TRANSFER_STALL;
-		goto out;
+		goto completed;
 	case -EOVERFLOW:
 		/* overflow can only ever occur in the last urb */
 		usbi_dbg("overflow, actual_length=%d", urb->actual_length);
 		status = LIBUSB_TRANSFER_OVERFLOW;
-		goto out;
+		goto completed;
 	case -ETIME:
 	case -EPROTO:
 	case -EILSEQ:
 		usbi_dbg("low level error %d", urb->status);
 		status = LIBUSB_TRANSFER_ERROR;
-		goto out;
+		goto completed;
 	default:
 		usbi_warn(ITRANSFER_CTX(itransfer),
 			"unrecognised urb status %d", urb->status);
 		status = LIBUSB_TRANSFER_ERROR;
-		goto out;
+		goto completed;
 	}
 
 	/* if we're the last urb or we got less data than requested then we're
@@ -1855,16 +1857,20 @@ static int handle_bulk_completion(struct usbi_transfer *itransfer,
 				usbi_warn(TRANSFER_CTX(transfer),
 					"unrecognised discard errno %d", errno);
 		}
-		return 0;
+		goto out_unlock;
 	} else {
-		return 0;
+		goto out_unlock;
 	}
 
-out:
+completed:
 	free(tpriv->urbs);
 	tpriv->urbs = NULL;
+	pthread_mutex_unlock(&itransfer->lock);
 	usbi_handle_transfer_completion(itransfer, status);
 	return 0;
+out_unlock:
+	pthread_mutex_unlock(&itransfer->lock);
+	return 0;
 }
 
 static int handle_iso_completion(struct usbi_transfer *itransfer,
@@ -1877,6 +1883,7 @@ static int handle_iso_completion(struct usbi_transfer *itransfer,
 	int urb_idx = 0;
 	int i;
 
+	pthread_mutex_lock(&itransfer->lock);
 	for (i = 0; i < num_urbs; i++) {
 		if (urb == tpriv->iso_urbs[i]) {
 			urb_idx = i + 1;
@@ -1885,6 +1892,7 @@ static int handle_iso_completion(struct usbi_transfer *itransfer,
 	}
 	if (urb_idx == 0) {
 		usbi_err(TRANSFER_CTX(transfer), "could not locate urb!");
+		pthread_mutex_unlock(&itransfer->lock);
 		return LIBUSB_ERROR_NOT_FOUND;
 	}
 
@@ -1911,13 +1919,17 @@ static int handle_iso_completion(struct usbi_transfer *itransfer,
 		if (tpriv->num_retired == num_urbs) {
 			usbi_dbg("CANCEL: last URB handled, reporting");
 			free_iso_urbs(tpriv);
-			if (tpriv->reap_action == CANCELLED)
+			if (tpriv->reap_action == CANCELLED) {
+				pthread_mutex_unlock(&itransfer->lock);
 				usbi_handle_transfer_cancellation(itransfer);
-			else
+			} else {
+				pthread_mutex_unlock(&itransfer->lock);
 				usbi_handle_transfer_completion(itransfer,
 					LIBUSB_TRANSFER_ERROR);
+			}
+			return 0;
 		}
-		return 0;
+		goto out;
 	}
 
 	switch (urb->status) {
@@ -1939,9 +1951,13 @@ static int handle_iso_completion(struct usbi_transfer *itransfer,
 	if (urb_idx == num_urbs) {
 		usbi_dbg("last URB in transfer --> complete!");
 		free_iso_urbs(tpriv);
+		pthread_mutex_unlock(&itransfer->lock);
 		usbi_handle_transfer_completion(itransfer, LIBUSB_TRANSFER_COMPLETED);
+		return 0;
 	}
 
+out:
+	pthread_mutex_unlock(&itransfer->lock);
 	return 0;
 }
 
@@ -1951,6 +1967,7 @@ static int handle_control_completion(struct usbi_transfer *itransfer,
 	struct linux_transfer_priv *tpriv = usbi_transfer_get_os_priv(itransfer);
 	int status;
 
+	pthread_mutex_lock(&itransfer->lock);
 	usbi_dbg("handling completion status %d", urb->status);
 
 	if (urb->status == 0)
@@ -1962,6 +1979,7 @@ static int handle_control_completion(struct usbi_transfer *itransfer,
 				"cancel: unrecognised urb status %d", urb->status);
 		free(tpriv->urbs);
 		tpriv->urbs = NULL;
+		pthread_mutex_unlock(&itransfer->lock);
 		usbi_handle_transfer_cancellation(itransfer);
 		return 0;
 	}
@@ -1990,6 +2008,7 @@ static int handle_control_completion(struct usbi_transfer *itransfer,
 
 	free(tpriv->urbs);
 	tpriv->urbs = NULL;
+	pthread_mutex_unlock(&itransfer->lock);
 	usbi_handle_transfer_completion(itransfer, status);
 	return 0;
 }