From bf833ee6adf58bd4a4a468aa729cdc78bdc13ede Mon Sep 17 00:00:00 2001 From: Benjamin Berg Date: Tue, 15 Feb 2022 11:13:41 +0100 Subject: [PATCH 1/2] core: Catch NULL dev_handle when getting a transfer's context 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. Add a test for dev_handle being valid before dereferencing it further. --- 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 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(+) diff --git a/libusb/io.c b/libusb/io.c index 0d2ac9ea..4e6d8984 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); return r;