Update regression fix patch for late transfer free's
Resolves: #2056326
This commit is contained in:
Benjamin Berg
parent
ddf6170c92
commit
36cafe8c9f
183
1073.patch
183
1073.patch
|
|
@ -1,72 +1,133 @@
|
|||
From bf833ee6adf58bd4a4a468aa729cdc78bdc13ede Mon Sep 17 00:00:00 2001
|
||||
From 1cb11c5ba0d1d1266fe4ddd70f29d081f9d16802 Mon Sep 17 00:00:00 2001
|
||||
From: Benjamin Berg <bberg@redhat.com>
|
||||
Date: Tue, 15 Feb 2022 11:13:41 +0100
|
||||
Subject: [PATCH 1/2] core: Catch NULL dev_handle when getting a transfer's
|
||||
context
|
||||
Subject: [PATCH] io: Track device in usbi_transfer
|
||||
|
||||
The dev_handle will be set to NULL when the transfer is still in-flight
|
||||
while the device is closed. In that case, the transfer free function
|
||||
will try to access the context and would run into a NULL pointer
|
||||
dereference.
|
||||
transfer->dev_handle currently has the behaviour that it will be unset
|
||||
if the device is closed. The sync API uses this fact to catch an error
|
||||
case.
|
||||
|
||||
Add a test for dev_handle being valid before dereferencing it further.
|
||||
In other cases, transfer->dev_handle will keep its value, which means
|
||||
that if the transfer lives longer than the device handle, the pointer
|
||||
becomes invalid.
|
||||
|
||||
The transfer does however keep a reference to the device, which owns the
|
||||
pointer to the context. As such, we can track this reference internal to
|
||||
the transfer, and it is set while the transfer is in-flight.
|
||||
|
||||
With this, switch the logging infrastructure to use itransfer->dev->ctx
|
||||
while checking that itransfer->dev is non-NULL.
|
||||
|
||||
Note that this was a regression caused by 6cae9c6dbd74 ("core: update
|
||||
usbi_dbg to take the context as an argument"), specifically when
|
||||
resolving the context while freeing a transfer after closing a device.
|
||||
|
||||
Note that the transfer will now keep a reference to the device until it
|
||||
is free'ed. This allows it to use the correct context for logging even
|
||||
in libusb_free_transfer.
|
||||
|
||||
The alternative to all this would be to just explicitly pass NULL to the
|
||||
log handler in libusb_free_transfer.
|
||||
---
|
||||
libusb/libusbi.h | 2 +-
|
||||
1 file changed, 1 insertion(+), 1 deletion(-)
|
||||
|
||||
diff --git a/libusb/libusbi.h b/libusb/libusbi.h
|
||||
index 158a9af5..dde43df2 100644
|
||||
--- a/libusb/libusbi.h
|
||||
+++ b/libusb/libusbi.h
|
||||
@@ -330,7 +330,7 @@ void usbi_log(struct libusb_context *ctx, enum libusb_log_level level,
|
||||
|
||||
#define DEVICE_CTX(dev) ((dev)->ctx)
|
||||
#define HANDLE_CTX(handle) (DEVICE_CTX((handle)->dev))
|
||||
-#define TRANSFER_CTX(transfer) (HANDLE_CTX((transfer)->dev_handle))
|
||||
+#define TRANSFER_CTX(transfer) ((transfer)->dev_handle ? HANDLE_CTX((transfer)->dev_handle) : NULL)
|
||||
#define ITRANSFER_CTX(itransfer) \
|
||||
(TRANSFER_CTX(USBI_TRANSFER_TO_LIBUSB_TRANSFER(itransfer)))
|
||||
|
||||
|
||||
From 6428090ea77dfb80906a146977ea7fd6de4718c8 Mon Sep 17 00:00:00 2001
|
||||
From: Benjamin Berg <bberg@redhat.com>
|
||||
Date: Tue, 15 Feb 2022 10:59:00 +0100
|
||||
Subject: [PATCH 2/2] io: Unset dev_handle when removing transfer from flying
|
||||
list
|
||||
|
||||
API users might hold on to transfers a bit longer than they are in the
|
||||
flying list. If they then close the device prior to freeing all
|
||||
transfers, we would end up with invalid pointers to the device.
|
||||
|
||||
Fix this by setting the device handle to NULL when removing the device
|
||||
from the flying list. This matches the behaviour when the device is
|
||||
closed while the transfer is still in the flying list.
|
||||
|
||||
Specifically, the libgusb wrapper will currently only free the
|
||||
underlying transfer in a later mainloop iteration (as a side effect on
|
||||
how GTask does memory management). It is possible to fix this, but it
|
||||
would make memory management within libgusb much more error prone.
|
||||
---
|
||||
libusb/io.c | 2 ++
|
||||
1 file changed, 2 insertions(+)
|
||||
libusb/io.c | 20 ++++++++++++--------
|
||||
libusb/libusbi.h | 10 +++++++---
|
||||
2 files changed, 19 insertions(+), 11 deletions(-)
|
||||
|
||||
diff --git a/libusb/io.c b/libusb/io.c
|
||||
index 0d2ac9ea..4e6d8984 100644
|
||||
index 0d2ac9ea..b919e9d9 100644
|
||||
--- a/libusb/io.c
|
||||
+++ b/libusb/io.c
|
||||
@@ -1456,6 +1456,7 @@ static int add_to_flying_list(struct usbi_transfer *itransfer)
|
||||
* if it fails to update the timer for the next timeout. */
|
||||
static int remove_from_flying_list(struct usbi_transfer *itransfer)
|
||||
{
|
||||
+ struct libusb_transfer *transfer = USBI_TRANSFER_TO_LIBUSB_TRANSFER(itransfer);
|
||||
struct libusb_context *ctx = ITRANSFER_CTX(itransfer);
|
||||
int rearm_timer;
|
||||
int r = 0;
|
||||
@@ -1466,6 +1467,7 @@ static int remove_from_flying_list(struct usbi_transfer *itransfer)
|
||||
list_del(&itransfer->list);
|
||||
if (rearm_timer)
|
||||
r = arm_timer_for_next_timeout(ctx);
|
||||
+ transfer->dev_handle = NULL;
|
||||
usbi_mutex_unlock(&ctx->flying_transfers_lock);
|
||||
@@ -1344,6 +1344,8 @@ void API_EXPORTED libusb_free_transfer(struct libusb_transfer *transfer)
|
||||
|
||||
itransfer = LIBUSB_TRANSFER_TO_USBI_TRANSFER(transfer);
|
||||
usbi_mutex_destroy(&itransfer->lock);
|
||||
+ if (itransfer->dev)
|
||||
+ libusb_unref_device(itransfer->dev);
|
||||
|
||||
priv_size = PTR_ALIGN(usbi_backend.transfer_priv_size);
|
||||
ptr = (unsigned char *)itransfer - priv_size;
|
||||
@@ -1489,9 +1491,15 @@ int API_EXPORTED libusb_submit_transfer(struct libusb_transfer *transfer)
|
||||
{
|
||||
struct usbi_transfer *itransfer =
|
||||
LIBUSB_TRANSFER_TO_USBI_TRANSFER(transfer);
|
||||
- struct libusb_context *ctx = TRANSFER_CTX(transfer);
|
||||
+ struct libusb_context *ctx;
|
||||
int r;
|
||||
|
||||
+ assert(transfer->dev_handle);
|
||||
+ if (itransfer->dev)
|
||||
+ libusb_unref_device(itransfer->dev);
|
||||
+ itransfer->dev = libusb_ref_device(transfer->dev_handle->dev);
|
||||
+
|
||||
+ ctx = HANDLE_CTX(transfer->dev_handle);
|
||||
usbi_dbg(ctx, "transfer %p", transfer);
|
||||
|
||||
/*
|
||||
@@ -1551,8 +1559,6 @@ int API_EXPORTED libusb_submit_transfer(struct libusb_transfer *transfer)
|
||||
r = usbi_backend.submit_transfer(itransfer);
|
||||
if (r == LIBUSB_SUCCESS) {
|
||||
itransfer->state_flags |= USBI_TRANSFER_IN_FLIGHT;
|
||||
- /* keep a reference to this device */
|
||||
- libusb_ref_device(transfer->dev_handle->dev);
|
||||
}
|
||||
usbi_mutex_unlock(&itransfer->lock);
|
||||
|
||||
@@ -1659,7 +1665,6 @@ int usbi_handle_transfer_completion(struct usbi_transfer *itransfer,
|
||||
{
|
||||
struct libusb_transfer *transfer =
|
||||
USBI_TRANSFER_TO_LIBUSB_TRANSFER(itransfer);
|
||||
- struct libusb_device_handle *dev_handle = transfer->dev_handle;
|
||||
struct libusb_context *ctx = ITRANSFER_CTX(itransfer);
|
||||
uint8_t flags;
|
||||
int r;
|
||||
@@ -1693,7 +1698,6 @@ int usbi_handle_transfer_completion(struct usbi_transfer *itransfer,
|
||||
* this point. */
|
||||
if (flags & LIBUSB_TRANSFER_FREE_TRANSFER)
|
||||
libusb_free_transfer(transfer);
|
||||
- libusb_unref_device(dev_handle->dev);
|
||||
return r;
|
||||
}
|
||||
|
||||
@@ -1727,10 +1731,10 @@ int usbi_handle_transfer_cancellation(struct usbi_transfer *itransfer)
|
||||
* function will be called the next time an event handler runs. */
|
||||
void usbi_signal_transfer_completion(struct usbi_transfer *itransfer)
|
||||
{
|
||||
- libusb_device_handle *dev_handle = USBI_TRANSFER_TO_LIBUSB_TRANSFER(itransfer)->dev_handle;
|
||||
+ struct libusb_device *dev = itransfer->dev;
|
||||
|
||||
- if (dev_handle) {
|
||||
- struct libusb_context *ctx = HANDLE_CTX(dev_handle);
|
||||
+ if (dev) {
|
||||
+ struct libusb_context *ctx = DEVICE_CTX(dev);
|
||||
unsigned int event_flags;
|
||||
|
||||
usbi_mutex_lock(&ctx->event_data_lock);
|
||||
diff --git a/libusb/libusbi.h b/libusb/libusbi.h
|
||||
index 158a9af5..5f0d5c2e 100644
|
||||
--- a/libusb/libusbi.h
|
||||
+++ b/libusb/libusbi.h
|
||||
@@ -329,10 +329,11 @@ void usbi_log(struct libusb_context *ctx, enum libusb_log_level level,
|
||||
#endif /* ENABLE_LOGGING */
|
||||
|
||||
#define DEVICE_CTX(dev) ((dev)->ctx)
|
||||
-#define HANDLE_CTX(handle) (DEVICE_CTX((handle)->dev))
|
||||
-#define TRANSFER_CTX(transfer) (HANDLE_CTX((transfer)->dev_handle))
|
||||
+#define HANDLE_CTX(handle) ((handle) ? DEVICE_CTX((handle)->dev) : NULL)
|
||||
#define ITRANSFER_CTX(itransfer) \
|
||||
- (TRANSFER_CTX(USBI_TRANSFER_TO_LIBUSB_TRANSFER(itransfer)))
|
||||
+ ((itransfer)->dev ? DEVICE_CTX((itransfer)->dev) : NULL)
|
||||
+#define TRANSFER_CTX(transfer) \
|
||||
+ (ITRANSFER_CTX(LIBUSB_TRANSFER_TO_USBI_TRANSFER(transfer)))
|
||||
|
||||
#define IS_EPIN(ep) (0 != ((ep) & LIBUSB_ENDPOINT_IN))
|
||||
#define IS_EPOUT(ep) (!IS_EPIN(ep))
|
||||
@@ -562,6 +563,9 @@ struct usbi_transfer {
|
||||
uint32_t state_flags; /* Protected by usbi_transfer->lock */
|
||||
uint32_t timeout_flags; /* Protected by the flying_stransfers_lock */
|
||||
|
||||
+ /* This is used for logging mostly. As long as it is set, the */
|
||||
+ struct libusb_device *dev;
|
||||
+
|
||||
/* this lock is held during libusb_submit_transfer() and
|
||||
* libusb_cancel_transfer() (allowing the OS backend to prevent duplicate
|
||||
* cancellation, submission-during-cancellation, etc). the OS backend
|
||||
|
|
|
|||
Loading…
Reference in New Issue