On 11/04/13 00:16, Pete Batard wrote:
On 2013.04.04 19:47, Pete Batard wrote:
The problem is that the backend private information for transfers wasn't
getting correctly cleared before the transfer completion callback was
called.
OK, I had a chance to look more closely into it at last. I think the
patch is close to what we want, but needs just a little bit of tweaking.
That's true. There is also an assumption that the reading of ints is
atomic as it checks the fd value at the same time as it might be copied
to by the submission of the transfer. However that's a bigger greater
problem to fix (Peter Stuge helpfully pointed me at the discussion here:
http://libusb.org/ticket/81).
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.
Yup. We create a new fake fd for each transfer, at submission time (so
that even if a transfer is reused, it will have a new fd).
4. The OS completes the request.
5. The backend detects this when next handling events and the client has
the callbacks called.
OK. Bearing in mind that before we call on the callbacks,
windows_clear_transfer_priv -> usbi_free_fd -> _free_index which sets
our poll_fd[] entry for that transfer to "INVALID_WINFD" (i.e. no active
transfer) in a critical section. The index of a free entry from that
table is what we use as fake fd right now.
6. Without the fix in this patch, transfer_priv->pollable_fd.fd still
has a value of one.
Yep.
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.
With you so far. The poll_fd[] entry was cleared, so we're free to
reassign the same fake fd, and the changes added for WinCE removed the
calls we used to issue to the OS, for an actual fd, so that we now use
the index of the first free element in our table as an fd value => we're
most likely going to get an fd value of 1 again.
Yes. I agree that no longer asking the OS for an fd could well have
changed the behaviour here, whether due to which fd value is picked or
just through timing differences. I believe the issue that Xiaofan
mentioned (https://github.com/libusbx/libusbx/issues/52) pre-dates the
change to how FDs are generated.
10. The client then submits transfer A, reading from bulk endpoint 0x82
with a timeout of 2 seconds.
If client reuses transfer A and submits, a new fd should be created and
assigned to A's transfer_priv part, in which case we probably have an fd
value of 2 in transfer A's priv part at this stage, since 1 has just
been assigned. A new OVERLAPPED should also be assigned.
Thus if a new transfer that reuses A has been fully submitted,
OVERLAPPED should not be NULL and the fake fd should not be 1.
As such, I don't think a fully completed 10 will result in the issue
described (but a partially completed 10 probably will).
Agreed.
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.
OK.
14. The windows backend notices that fake fd 1 has been signalled and
starts to look through the flying transfer list.
OK.
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
The only way this should happen is if we are going through the flying
transfer list before the transfer that reuses A has been fully submitted
(or even initiated at all)
Otherwise we should have an fd value of 2 for transfer A, not 1.
Yes. To reference the actual code, the problem is that the thread
submitting the re-used transfer has executed this line:
https://github.com/libusbx/libusbx/blob/ad3e3d42ee0964e8181002a0344e40abec0001bd/libusb/io.c#L1377
but has yet to reach this line in the backend:
https://github.com/libusbx/libusbx/blob/ad3e3d42ee0964e8181002a0344e40abec0001bd/libusb/os/windows_usb.c#L2953
So when this line checks the fd it is reading the value from the last
time the transfer was submitted:
https://github.com/libusbx/libusbx/blob/ad3e3d42ee0964e8181002a0344e40abec0001bd/libusb/os/windows_usb.c#L2953
and thinks it's been completed. However the OVERLAPPED is NULL so
it causes a NULL pointer dereference.
Same story. This can only happen if A's submission for a new transfer
has not been completed yet, as the OVERLAPPED wouldn't be NULL otherwise.
Good point. I missed out that to get the NULL pointer reference that the
thread submitting the transfer has to have had time to execute this line:
https://github.com/libusbx/libusbx/blob/ad3e3d42ee0964e8181002a0344e40abec0001bd/libusb/os/windows_usb.c#L2953
between the event handling thread checking for the matching fd and then
calling "HasOverlappedIoCompletedSync" here:
https://github.com/libusbx/libusbx/blob/ad3e3d42ee0964e8181002a0344e40abec0001bd/libusb/os/windows_usb.c#L2125
On the other hand, the following line:
https://github.com/libusbx/libusbx/blob/master/libusb/os/windows_usb.c#L2116
makes it clear that if we have an inactive or semi inactive transfer in
our flying list, with an fd that hasn't been cleared and is being
reused, we're going to have an issue indeed, so regardless of what 10/15
should read, your explanation makes sense.
Yes. Thanks for taking the time to work out what I meant in my not
entirely clear explaination of each step. I was trying to strike the
balance between too much information and not enough and it had already
got to 15 steps :)
I think the reason we haven't picked on this issue so far is that before
the inclusion of WinCE, we were requesting an actual fd from the OS (See
the _open() line that was removed with [1]), so during the course of a
normal run, we would never see the same fd value, even for fake fds.
Right now, I think the fd cleanup when we process the original transfer
is what we want, but the piece that is missing would be to surround your
patch with a flying_transfers_lock or pass a pointer to the fd in
usbi_free_fd() and do the fd cleanup there (since we have a critical
section), to avoid any risks of having an OVERLAPPED that is NULL with
an fd that we may still be trying to process. This may be needed in case
we switch to processing the flying transfer list in a multithreaded app,
right before we clean the fd.
I've attached a patch for the usbi_free_fd change you suggested. I think
it still doesn't solve the read of the fd while trying to find the
transfer in the flying transfers, but I'm not sure any of the current
locks can help with that.
Regards,
Toby
>From 2ce7f9822cedb6690b40ff4573316f40536aa8d6 Mon Sep 17 00:00:00 2001
From: Toby Gray <toby.g...@realvnc.com>
Date: Thu, 11 Apr 2013 13:40:31 +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/poll_windows.c | 5 +++--
libusb/os/poll_windows.h | 2 +-
libusb/os/wince_usb.c | 2 +-
libusb/os/windows_usb.c | 14 +++++++-------
4 files changed, 12 insertions(+), 11 deletions(-)
diff --git a/libusb/os/poll_windows.c b/libusb/os/poll_windows.c
index 6bb91af..7ed19ba 100644
--- a/libusb/os/poll_windows.c
+++ b/libusb/os/poll_windows.c
@@ -403,17 +403,18 @@ static void _free_index(int _index)
*
* Note that the associated Windows handle is not closed by this call
*/
-void usbi_free_fd(int fd)
+void usbi_free_fd(struct winfd *wfd)
{
int _index;
CHECK_INIT_POLLING;
- _index = _fd_to_index_and_lock(fd);
+ _index = _fd_to_index_and_lock(wfd->fd);
if (_index < 0) {
return;
}
_free_index(_index);
+ *wfd = INVALID_WINFD;
LeaveCriticalSection(&_poll_fd[_index].mutex);
}
diff --git a/libusb/os/poll_windows.h b/libusb/os/poll_windows.h
index 1e92dda..deed206 100644
--- a/libusb/os/poll_windows.h
+++ b/libusb/os/poll_windows.h
@@ -94,7 +94,7 @@ void init_polling(void);
void exit_polling(void);
struct winfd usbi_create_fd(HANDLE handle, int access_mode,
struct usbi_transfer *transfer, cancel_transfer *cancel_fn);
-void usbi_free_fd(int fd);
+void usbi_free_fd(struct winfd* winfd);
struct winfd fd_to_winfd(int fd);
struct winfd handle_to_winfd(HANDLE handle);
struct winfd overlapped_to_winfd(OVERLAPPED* overlapped);
diff --git a/libusb/os/wince_usb.c b/libusb/os/wince_usb.c
index e4f7c7b..2e6eedd 100644
--- a/libusb/os/wince_usb.c
+++ b/libusb/os/wince_usb.c
@@ -593,7 +593,7 @@ static void wince_clear_transfer_priv(
// No need to cancel transfer as it is either complete or abandoned
wfd.itransfer = NULL;
CloseHandle(wfd.handle);
- usbi_free_fd(transfer_priv->pollable_fd.fd);
+ usbi_free_fd(&transfer_priv->pollable_fd);
}
static int wince_cancel_transfer(
diff --git a/libusb/os/windows_usb.c b/libusb/os/windows_usb.c
index 7c2428d..e0d2a1c 100644
--- a/libusb/os/windows_usb.c
+++ b/libusb/os/windows_usb.c
@@ -1911,7 +1911,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);
+ usbi_free_fd(&transfer_priv->pollable_fd);
safe_free(transfer_priv->hid_buffer);
// When auto claim is in use, attempt to release the auto-claimed interface
auto_release(itransfer);
@@ -2884,7 +2884,7 @@ static int winusbx_submit_control_transfer(int sub_api, struct usbi_transfer *it
&& (setup->request == LIBUSB_REQUEST_SET_CONFIGURATION) ) {
if (setup->value != priv->active_config) {
usbi_warn(ctx, "cannot set configuration other than the default one");
- usbi_free_fd(wfd.fd);
+ usbi_free_fd(&wfd);
return LIBUSB_ERROR_INVALID_PARAM;
}
wfd.overlapped->Internal = STATUS_COMPLETED_SYNCHRONOUSLY;
@@ -2893,7 +2893,7 @@ static int winusbx_submit_control_transfer(int sub_api, struct usbi_transfer *it
if (!WinUSBX[sub_api].ControlTransfer(wfd.handle, *setup, transfer->buffer + LIBUSB_CONTROL_SETUP_SIZE, size, NULL, wfd.overlapped)) {
if(GetLastError() != ERROR_IO_PENDING) {
usbi_warn(ctx, "ControlTransfer failed: %s", windows_error_str(0));
- usbi_free_fd(wfd.fd);
+ usbi_free_fd(&wfd);
return LIBUSB_ERROR_IO;
}
} else {
@@ -2978,7 +2978,7 @@ static int winusbx_submit_bulk_transfer(int sub_api, struct usbi_transfer *itran
if (!ret) {
if(GetLastError() != ERROR_IO_PENDING) {
usbi_err(ctx, "ReadPipe/WritePipe failed: %s", windows_error_str(0));
- usbi_free_fd(wfd.fd);
+ usbi_free_fd(&wfd);
return LIBUSB_ERROR_IO;
}
} else {
@@ -3087,7 +3087,7 @@ static int winusbx_reset_device(int sub_api, struct libusb_device_handle *dev_ha
{
// Cancel any pollable I/O
usbi_remove_pollfd(ctx, wfd.fd);
- usbi_free_fd(wfd.fd);
+ usbi_free_fd(&wfd);
wfd = handle_to_winfd(winusb_handle);
}
@@ -3964,7 +3964,7 @@ static int hid_submit_control_transfer(int sub_api, struct usbi_transfer *itrans
transfer_priv->pollable_fd = wfd;
transfer_priv->interface_number = (uint8_t)current_interface;
} else {
- usbi_free_fd(wfd.fd);
+ usbi_free_fd(&wfd);
}
return r;
@@ -4037,7 +4037,7 @@ static int hid_submit_bulk_transfer(int sub_api, struct usbi_transfer *itransfer
if (!ret) {
if (GetLastError() != ERROR_IO_PENDING) {
usbi_err(ctx, "HID transfer failed: %s", windows_error_str(0));
- usbi_free_fd(wfd.fd);
+ usbi_free_fd(&wfd);
safe_free(transfer_priv->hid_buffer);
return LIBUSB_ERROR_IO;
}
--
1.7.9
------------------------------------------------------------------------------
Precog is a next-generation analytics platform capable of advanced
analytics on semi-structured data. The platform includes APIs for building
apps and a phenomenal toolset for data science. Developers can use
our toolset for easy data analysis & visualization. Get a free account!
http://www2.precog.com/precogplatform/slashdotnewsletter
_______________________________________________
libusbx-devel mailing list
libusbx-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/libusbx-devel
