Prevent transfer from being submitted twice or improperly canceled

This ensures that tpriv->urbs and tpriv->iso_urbs are always set to NULL
whenever a transfer is not submitted.  In this way, submit_*_transfer()
and cancel_*_transfer() can error check to ensure that the transfer is
in the correct state to be either submitted or canceled, preventing
potential memory leaks or double frees.

Signed-off-by: David Moore <dcm@acm.org>

2 files changed, 35 insertions, 11 deletions

diff --git a/libusb/io.c b/libusb/io.c
index 89a94a2..320bc4b 100644
--- a/libusb/io.c
+++ b/libusb/io.c
@@ -1101,12 +1101,10 @@ API_EXPORTED void libusb_free_transfer(struct libusb_transfer *transfer)
  * Submit a transfer. This function will fire off the USB transfer and then
  * return immediately.
  *
- * It is undefined behaviour to submit a transfer that has already been
- * submitted but has not yet completed.
- *
  * \param transfer the transfer to submit
  * \returns 0 on success
  * \returns LIBUSB_ERROR_NO_DEVICE if the device has been disconnected
+ * \returns LIBUSB_ERROR_BUSY if the transfer has already been submitted.
  * \returns another LIBUSB_ERROR code on other failure
  */
 API_EXPORTED int libusb_submit_transfer(struct libusb_transfer *transfer)
@@ -1134,8 +1132,6 @@ API_EXPORTED int libusb_submit_transfer(struct libusb_transfer *transfer)
 
 /** \ingroup asyncio
  * Asynchronously cancel a previously submitted transfer.
- * It is undefined behaviour to call this function on a transfer that is
- * already being cancelled or has already completed.
  * This function returns immediately, but this does not indicate cancellation
  * is complete. Your callback function will be invoked at some later time
  * with a transfer status of
@@ -1144,6 +1140,8 @@ API_EXPORTED int libusb_submit_transfer(struct libusb_transfer *transfer)
  *
  * \param transfer the transfer to cancel
  * \returns 0 on success
+ * \returns LIBUSB_ERROR_NOT_FOUND if the transfer is already complete or
+ * cancelled.
  * \returns a LIBUSB_ERROR code on failure
  */
 API_EXPORTED int libusb_cancel_transfer(struct libusb_transfer *transfer)
diff --git a/libusb/os/linux_usbfs.c b/libusb/os/linux_usbfs.c
index e015aff..46e770c 100644
--- a/libusb/os/linux_usbfs.c
+++ b/libusb/os/linux_usbfs.c
@@ -1245,6 +1245,7 @@ static void free_iso_urbs(struct linux_transfer_priv *tpriv)
 	}
 
 	free(tpriv->iso_urbs);
+	tpriv->iso_urbs = NULL;
 }
 
 static int submit_bulk_transfer(struct usbi_transfer *itransfer,
@@ -1260,6 +1261,9 @@ static int submit_bulk_transfer(struct usbi_transfer *itransfer,
 	int i;
 	size_t alloc_size;
 
+	if (tpriv->urbs)
+		return LIBUSB_ERROR_BUSY;
+
 	/* usbfs places a 16kb limit on bulk URBs. we divide up larger requests
 	 * into smaller units to meet such restriction, then fire off all the
 	 * units at once. it would be simpler if we just fired one unit at a time,
@@ -1311,6 +1315,7 @@ static int submit_bulk_transfer(struct usbi_transfer *itransfer,
 			if (i == 0) {
 				usbi_dbg("first URB failed, easy peasy");
 				free(urbs);
+				tpriv->urbs = NULL;
 				return r;
 			}
 
@@ -1365,6 +1370,9 @@ static int submit_iso_transfer(struct usbi_transfer *itransfer)
 	unsigned int packet_len;
 	unsigned char *urb_buffer = transfer->buffer;
 
+	if (tpriv->iso_urbs)
+		return LIBUSB_ERROR_BUSY;
+
 	/* usbfs places a 32kb limit on iso URBs. we divide up larger requests
 	 * into smaller units to meet such restriction, then fire off all the
 	 * units at once. it would be simpler if we just fired one unit at a time,
@@ -1512,6 +1520,9 @@ static int submit_control_transfer(struct usbi_transfer *itransfer)
 	struct usbfs_urb *urb;
 	int r;
 
+	if (tpriv->urbs)
+		return LIBUSB_ERROR_BUSY;
+
 	if (transfer->length - LIBUSB_CONTROL_SETUP_SIZE > MAX_CTRL_BUFFER_LENGTH)
 		return LIBUSB_ERROR_INVALID_PARAM;
 
@@ -1531,6 +1542,7 @@ static int submit_control_transfer(struct usbi_transfer *itransfer)
 	r = ioctl(dpriv->fd, IOCTL_USBFS_SUBMITURB, urb);
 	if (r < 0) {
 		free(urb);
+		tpriv->urbs = NULL;
 		if (errno == ENODEV)
 			return LIBUSB_ERROR_NO_DEVICE;
 
@@ -1571,6 +1583,9 @@ static int cancel_control_transfer(struct usbi_transfer *itransfer)
 		__device_handle_priv(transfer->dev_handle);
 	int r;
 
+	if (!tpriv->urbs)
+		return LIBUSB_ERROR_NOT_FOUND;
+
 	tpriv->reap_action = CANCELLED;
 	r = ioctl(dpriv->fd, IOCTL_USBFS_DISCARDURB, tpriv->urbs);
 	if(r) {
@@ -1587,7 +1602,7 @@ static int cancel_control_transfer(struct usbi_transfer *itransfer)
 	return 0;
 }
 
-static void cancel_bulk_transfer(struct usbi_transfer *itransfer)
+static int cancel_bulk_transfer(struct usbi_transfer *itransfer)
 {
 	struct linux_transfer_priv *tpriv = usbi_transfer_get_os_priv(itransfer);
 	struct libusb_transfer *transfer =
@@ -1596,6 +1611,9 @@ static void cancel_bulk_transfer(struct usbi_transfer *itransfer)
 		__device_handle_priv(transfer->dev_handle);
 	int i;
 
+	if (!tpriv->urbs)
+		return LIBUSB_ERROR_NOT_FOUND;
+
 	tpriv->reap_action = CANCELLED;
 	for (i = 0; i < tpriv->num_urbs; i++) {
 		int tmp = ioctl(dpriv->fd, IOCTL_USBFS_DISCARDURB, &tpriv->urbs[i]);
@@ -1603,9 +1621,10 @@ static void cancel_bulk_transfer(struct usbi_transfer *itransfer)
 			usbi_warn(TRANSFER_CTX(transfer),
 				"unrecognised discard errno %d", errno);
 	}
+	return 0;
 }
 
-static void cancel_iso_transfer(struct usbi_transfer *itransfer)
+static int cancel_iso_transfer(struct usbi_transfer *itransfer)
 {
 	struct linux_transfer_priv *tpriv = usbi_transfer_get_os_priv(itransfer);
 	struct libusb_transfer *transfer =
@@ -1614,6 +1633,9 @@ static void cancel_iso_transfer(struct usbi_transfer *itransfer)
 		__device_handle_priv(transfer->dev_handle);
 	int i;
 
+	if (!tpriv->iso_urbs)
+		return LIBUSB_ERROR_NOT_FOUND;
+
 	tpriv->reap_action = CANCELLED;
 	for (i = 0; i < tpriv->num_urbs; i++) {
 		int tmp = ioctl(dpriv->fd, IOCTL_USBFS_DISCARDURB, tpriv->iso_urbs[i]);
@@ -1621,6 +1643,7 @@ static void cancel_iso_transfer(struct usbi_transfer *itransfer)
 			usbi_warn(TRANSFER_CTX(transfer),
 				"unrecognised discard errno %d", errno);
 	}
+	return 0;
 }
 
 static int op_cancel_transfer(struct usbi_transfer *itransfer)
@@ -1633,11 +1656,9 @@ static int op_cancel_transfer(struct usbi_transfer *itransfer)
 		return cancel_control_transfer(itransfer);
 	case LIBUSB_TRANSFER_TYPE_BULK:
 	case LIBUSB_TRANSFER_TYPE_INTERRUPT:
-		cancel_bulk_transfer(itransfer);
-		return 0;
+		return cancel_bulk_transfer(itransfer);
 	case LIBUSB_TRANSFER_TYPE_ISOCHRONOUS:
-		cancel_iso_transfer(itransfer);
-		return 0;
+		return cancel_iso_transfer(itransfer);
 	default:
 		usbi_err(TRANSFER_CTX(transfer),
 			"unknown endpoint type %d", transfer->type);
@@ -1656,6 +1677,7 @@ static void op_clear_transfer_priv(struct usbi_transfer *itransfer)
 	case LIBUSB_TRANSFER_TYPE_BULK:
 	case LIBUSB_TRANSFER_TYPE_INTERRUPT:
 		free(tpriv->urbs);
+		tpriv->urbs = NULL;
 		break;
 	case LIBUSB_TRANSFER_TYPE_ISOCHRONOUS:
 		free_iso_urbs(tpriv);
@@ -1697,6 +1719,7 @@ static int handle_bulk_completion(struct usbi_transfer *itransfer,
 			usbi_dbg("CANCEL: last URB handled, reporting");
 			if (tpriv->reap_action == CANCELLED) {
 				free(tpriv->urbs);
+				tpriv->urbs = NULL;
 				usbi_handle_transfer_cancellation(itransfer);
 				return 0;
 			}
@@ -1762,6 +1785,7 @@ static int handle_bulk_completion(struct usbi_transfer *itransfer,
 
 out:
 	free(tpriv->urbs);
+	tpriv->urbs = NULL;
 	usbi_handle_transfer_completion(itransfer, status);
 	return 0;
 }
@@ -1860,6 +1884,7 @@ static int handle_control_completion(struct usbi_transfer *itransfer,
 			usbi_warn(ITRANSFER_CTX(itransfer),
 				"cancel: unrecognised urb status %d", urb->status);
 		free(tpriv->urbs);
+		tpriv->urbs = NULL;
 		usbi_handle_transfer_cancellation(itransfer);
 		return 0;
 	}
@@ -1887,6 +1912,7 @@ static int handle_control_completion(struct usbi_transfer *itransfer,
 	}
 
 	free(tpriv->urbs);
+	tpriv->urbs = NULL;
 	usbi_handle_transfer_completion(itransfer, status);
 	return 0;
 }