Linux: Fix #81 URB double free race condition on device disconnect
authorPete Batard <pbatard@gmail.com>
Thu, 16 Jun 2011 09:49:13 +0000 (10:49 +0100)
committerPeter Stuge <peter@stuge.se>
Mon, 17 Oct 2011 14:25:50 +0000 (16:25 +0200)
A submitted transfer that has just been failed by the kernel could be
picked up by an event handler to be cleaned up, where freeing of URB
memory would race with the submit function doing it's own cleanup and
freeing as a result of the submit failing.

libusb_submit_transfer() always holds itransfer->lock, so the race can
be avoided by taking that lock also in the cleanup path and checking
that the URB memory has not already been freed before freeing it there.

As http://libusb.org/ticket/81#comment:14 notes there is still another
possible, but unlikely, race condition between libusb_submit_transfer()
and an event handling thread. That will require more work to solve.

[stuge: Add check in cleanup path that URBs have not already been freed]

libusb/os/linux_usbfs.c

index c84e805..2b81189 100644 (file)
@@ -1860,15 +1860,22 @@ static void op_clear_transfer_priv(struct usbi_transfer *itransfer)
                USBI_TRANSFER_TO_LIBUSB_TRANSFER(itransfer);
        struct linux_transfer_priv *tpriv = usbi_transfer_get_os_priv(itransfer);
 
+       /* urbs can be freed also in submit_transfer so lock mutex first */
        switch (transfer->type) {
        case LIBUSB_TRANSFER_TYPE_CONTROL:
        case LIBUSB_TRANSFER_TYPE_BULK:
        case LIBUSB_TRANSFER_TYPE_INTERRUPT:
-               free(tpriv->urbs);
+               usbi_mutex_lock(&itransfer->lock);
+               if (tpriv->urbs)
+                       free(tpriv->urbs);
                tpriv->urbs = NULL;
+               usbi_mutex_unlock(&itransfer->lock);
                break;
        case LIBUSB_TRANSFER_TYPE_ISOCHRONOUS:
-               free_iso_urbs(tpriv);
+               usbi_mutex_lock(&itransfer->lock);
+               if (tpriv->iso_urbs)
+                       free_iso_urbs(tpriv);
+               usbi_mutex_unlock(&itransfer->lock);
                break;
        default:
                usbi_err(TRANSFER_CTX(transfer),