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>

diff --git a/libusb/io.c b/libusb/io.c
index 89a94a2..320bc4b 100644 (file)
--- 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 (file)
--- 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;
 }