From 414ddc32c504a1d91df1f30fdec5a60ab7c3472b Mon Sep 17 00:00:00 2001 From: Cameron Gutman Date: Fri, 1 Jan 2021 17:34:07 -0600 Subject: [PATCH] Do not wait in GetOverlappedResult() in hid_read_timeout() This is unsafe because the event is auto-reset, therefore the call to WaitForSingleObject() resets the event which GetOverlappedResult() will try to wait on. Even though the overlapped operation is guaranteed to be completed at the point we call GetOverlappedResult(), it will still wait on the event handle for a short time to trigger the reset for auto-reset events. This amounts to roughly a 100 ms sleep each time GetOverlappedResult() is called for a completed I/O with a non-signalled event. In the context of HIDAPI, this extra sleep means that callers that loop on hid_read_timeout() with timeout=0 will loop forever, since the 100 ms sleep each iteration ensures ReadFile() will always have new data. --- src/hidapi/windows/hid.c | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/src/hidapi/windows/hid.c b/src/hidapi/windows/hid.c index ca8bb6d41..4b0f7cbd0 100644 --- a/src/hidapi/windows/hid.c +++ b/src/hidapi/windows/hid.c @@ -820,20 +820,19 @@ int HID_API_EXPORT HID_API_CALL hid_read_timeout(hid_device *dev, unsigned char } } - if (milliseconds >= 0) { - /* See if there is any data yet. */ - res = WaitForSingleObject(ev, milliseconds); - if (res != WAIT_OBJECT_0) { - /* There was no data this time. Return zero bytes available, - but leave the Overlapped I/O running. */ - return 0; - } + /* See if there is any data yet. */ + res = WaitForSingleObject(ev, milliseconds >= 0 ? milliseconds : INFINITE); + if (res != WAIT_OBJECT_0) { + /* There was no data this time. Return zero bytes available, + but leave the Overlapped I/O running. */ + return 0; } - /* Either WaitForSingleObject() told us that ReadFile has completed, or - we are in non-blocking mode. Get the number of bytes read. The actual - data has been copied to the data[] array which was passed to ReadFile(). */ - res = GetOverlappedResult(dev->device_handle, &dev->ol, &bytes_read, TRUE/*wait*/); + /* Get the number of bytes read. The actual data has been copied to the data[] + array which was passed to ReadFile(). We must not wait here because we've + already waited on our event above, and since it's auto-reset, it will have + been reset back to unsignalled by now. */ + res = GetOverlappedResult(dev->device_handle, &dev->ol, &bytes_read, FALSE/*don't wait*/); /* Set pending back to false, even if GetOverlappedResult() returned error. */ dev->read_pending = FALSE;