Hi,
As I mentioned in a previous email, I've been tracking down a rare NULL
pointer dereference in the windows desktop backend.
The problem is that the backend private information for transfers wasn't
getting correctly cleared before the transfer completion callback was
called.
This meant that the fake fd number inside the transfer still contained
the old value. As there is a period of time where a transfer is in the
flying transfer list but hasn't been submitted to the backend, this
meant that the wrong transfer could be completed.
The problematic sequence is along the lines of the following:
1. Client creates a transfer using libusb_alloc_transfer. Let's call
this transfer A.
2. Transfer A is submitted, e.g. reading from bulk endpoint 0x81.
3. The backend assigns a fake fd of 1 to transfer A and then submits it
to the OS.
4. The OS completes the request.
5. The backend detects this when next handling events and the client has
the callbacks called.
6. Without the fix in this patch, transfer_priv->pollable_fd.fd still
has a value of one.
7. Client creates a new transfer using libusb_alloc_transfer. Let's call
this transfer B.
8. Transer B is submitted, e.g. reading from bulk endpoing 0x81 with
infinite timeout.
9. The backend assigns a fake fd of 1 to transfer B and then submits it
to the OS.
10. The client then submits transfer A, reading from bulk endpoint 0x82
with a timeout of 2 seconds.
11. Before the backend is given transfer A it is added to the flying
transfer list.
12. The OS completes transfer B.
13. A separate thread starts to run and asks libusb to handle events.
14. The windows backend notices that fake fd 1 has been signalled and
starts to look through the flying transfer list.
15. The windows backend finds transfer A as it is earlier in the flying
transfer list as it has an earlier timeout and has the old fake fd value
of 1 and thinks it's been completed. However the OVERLAPPED is NULL so
it causes a NULL pointer dereference.
Tracking this down was further complicated by the NULL pointer
dereference taking a bit of time to handle. By which time the thread
which was resubmitting transfer A had gone and updated the fd value and
set a valid OVERLAPPED structure.
The attached patch makes sure this can't happen by making sure
windows_clear_transfer_priv correctly clears the pollable_fd. This means
that step 15 in the above sequence correctly skips over transfer A as
the fake fd value will be -1 (until the transfer is actually submitted
to the OS).
It's also a problem with the WinCE backend, so I've included a similar
fix in the patch.
I understand that this is probably too risky a change to get into
1.0.15, especially as no-one else seems to have reported it.
Regards,
Toby
>From f9da0f7b1470f098ae76fee686562c5fcb17961d Mon Sep 17 00:00:00 2001
From: Toby Gray <toby.g...@realvnc.com>
Date: Thu, 4 Apr 2013 16:27:21 +0100
Subject: [PATCH] Windows and WinCE: Correctly clear backend transfer private
information.
* Without this fix if a transfer is reused then there is a period of
time between it being adding to the flying transfer list and
submitted where the fd value will be the old fd value.
* If this occurs at the same time as all of the following conditions
then the incorrect transfer will be handled as having completed:
* The old fd value in the reused transfer has been recycled for a
currently pending transfer.
* This other pending transfer has a later timeout than the reused
transfer (so therefore comes later in the flying transfer list).
* The other pending transfer completes, therefore signalling the fd.
As the flying transfer list is examined in order when handling events,
the resubmitted transfer with the old fd value will be considered as
completed. This will generally cause a NULL pointer dereference as the
OVERLAPPED structure was already freed.
---
libusb/os/wince_usb.c | 1 +
libusb/os/windows_usb.c | 1 +
2 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/libusb/os/wince_usb.c b/libusb/os/wince_usb.c
index e4f7c7b..b4eb904 100644
--- a/libusb/os/wince_usb.c
+++ b/libusb/os/wince_usb.c
@@ -594,6 +594,7 @@ static void wince_clear_transfer_priv(
wfd.itransfer = NULL;
CloseHandle(wfd.handle);
usbi_free_fd(transfer_priv->pollable_fd.fd);
+ transfer_priv->pollable_fd = INVALID_WINFD;
}
static int wince_cancel_transfer(
diff --git a/libusb/os/windows_usb.c b/libusb/os/windows_usb.c
index 7c2428d..9b5de45 100644
--- a/libusb/os/windows_usb.c
+++ b/libusb/os/windows_usb.c
@@ -1912,6 +1912,7 @@ static void windows_clear_transfer_priv(struct usbi_transfer *itransfer)
struct windows_transfer_priv *transfer_priv = (struct windows_transfer_priv*)usbi_transfer_get_os_priv(itransfer);
usbi_free_fd(transfer_priv->pollable_fd.fd);
+ transfer_priv->pollable_fd = INVALID_WINFD;
safe_free(transfer_priv->hid_buffer);
// When auto claim is in use, attempt to release the auto-claimed interface
auto_release(itransfer);
--
1.7.9
------------------------------------------------------------------------------
Minimize network downtime and maximize team effectiveness.
Reduce network management and security costs.Learn how to hire
the most talented Cisco Certified professionals. Visit the
Employer Resources Portal
http://www.cisco.com/web/learning/employer_resources/index.html
_______________________________________________
libusbx-devel mailing list
libusbx-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/libusbx-devel
