From bae23de82f5356656555fb81a18c46806d744c16 Mon Sep 17 00:00:00 2001 From: Armin Novak Date: Tue, 16 Nov 2021 08:40:34 +0100 Subject: [PATCH] Fixed issues with libusb device unref --- channels/urbdrc/client/data_transfer.c | 11 +- .../urbdrc/client/libusb/libusb_udevice.c | 160 ++++++++++-------- channels/urbdrc/client/urbdrc_main.c | 5 +- channels/urbdrc/client/urbdrc_main.h | 2 +- 4 files changed, 106 insertions(+), 72 deletions(-) diff --git a/channels/urbdrc/client/data_transfer.c b/channels/urbdrc/client/data_transfer.c index f88528482..5ac972014 100644 --- a/channels/urbdrc/client/data_transfer.c +++ b/channels/urbdrc/client/data_transfer.c @@ -781,6 +781,7 @@ static UINT urb_isoch_transfer(IUDEVICE* pdev, URBDRC_CHANNEL_CALLBACK* callback UINT32 RequestField, UINT32 MessageId, IUDEVMAN* udevman, int transferDir) { + int rc; UINT32 EndpointAddress; UINT32 PipeHandle, TransferFlags, StartFrame, NumberOfPackets; UINT32 ErrorCount, OutputBufferSize; @@ -807,11 +808,15 @@ static UINT urb_isoch_transfer(IUDEVICE* pdev, URBDRC_CHANNEL_CALLBACK* callback packetDescriptorData = Stream_Pointer(s); Stream_Seek(s, NumberOfPackets * 12); Stream_Read_UINT32(s, OutputBufferSize); - return pdev->isoch_transfer( + rc = pdev->isoch_transfer( pdev, callback, MessageId, RequestId, EndpointAddress, TransferFlags, StartFrame, ErrorCount, noAck, packetDescriptorData, NumberOfPackets, OutputBufferSize, (transferDir == USBD_TRANSFER_DIRECTION_OUT) ? Stream_Pointer(s) : NULL, urb_isoch_transfer_cb, 2000); + + if (rc < 0) + return ERROR_INTERNAL_ERROR; + return (UINT)rc; } static UINT urb_control_descriptor_request(IUDEVICE* pdev, URBDRC_CHANNEL_CALLBACK* callback, @@ -1755,8 +1760,8 @@ static UINT urbdrc_process_transfer_request(IUDEVICE* pdev, URBDRC_CHANNEL_CALLB if (error) { WLog_Print(urbdrc->log, WLOG_WARN, - "USB transfer request URB Function %08" PRIx32 " failed with %08" PRIx32, - URB_Function, error); + "USB transfer request URB Function '%s' [0x%08x] failed with %08" PRIx32, + urb_function_string(URB_Function), URB_Function, error); } return error; diff --git a/channels/urbdrc/client/libusb/libusb_udevice.c b/channels/urbdrc/client/libusb/libusb_udevice.c index c6527dfdc..2aaab176a 100644 --- a/channels/urbdrc/client/libusb/libusb_udevice.c +++ b/channels/urbdrc/client/libusb/libusb_udevice.c @@ -494,23 +494,19 @@ static int func_claim_all_interface(URBDRC_PLUGIN* urbdrc, LIBUSB_DEVICE_HANDLE* static LIBUSB_DEVICE* udev_get_libusb_dev(libusb_context* context, uint8_t bus_number, uint8_t dev_number) { - ssize_t i, total_device; - LIBUSB_DEVICE** libusb_list; + ssize_t i; + LIBUSB_DEVICE** libusb_list = NULL; LIBUSB_DEVICE* device = NULL; - total_device = libusb_get_device_list(context, &libusb_list); + const ssize_t total_device = libusb_get_device_list(context, &libusb_list); for (i = 0; i < total_device; i++) { LIBUSB_DEVICE* dev = libusb_list[i]; if ((bus_number == libusb_get_bus_number(dev)) && (dev_number == libusb_get_device_address(dev))) - { device = dev; - } else - { libusb_unref_device(dev); - } } libusb_free_device_list(libusb_list, 0); @@ -924,11 +920,17 @@ static int libusb_udev_os_feature_descriptor_request(IUDEVICE* idev, UINT32 Requ BYTE Recipient, BYTE InterfaceNumber, BYTE Ms_PageIndex, UINT16 Ms_featureDescIndex, UINT32* UsbdStatus, UINT32* BufferSize, - BYTE* Buffer, int Timeout) + BYTE* Buffer, UINT32 Timeout) { UDEVICE* pdev = (UDEVICE*)idev; BYTE ms_string_desc[0x13] = { 0 }; int error = 0; + + WINPR_ASSERT(idev); + WINPR_ASSERT(UsbdStatus); + WINPR_ASSERT(BufferSize); + WINPR_ASSERT(*BufferSize <= UINT16_MAX); + /* pdev->request_queue->register_request(pdev->request_queue, RequestId, NULL, 0); */ @@ -942,14 +944,14 @@ static int libusb_udev_os_feature_descriptor_request(IUDEVICE* idev, UINT32 Requ { const BYTE bMS_Vendorcode = ms_string_desc[16]; /** get os descriptor */ - error = libusb_control_transfer(pdev->libusb_handle, - LIBUSB_ENDPOINT_IN | LIBUSB_REQUEST_TYPE_VENDOR | Recipient, - bMS_Vendorcode, (InterfaceNumber << 8) | Ms_PageIndex, - Ms_featureDescIndex, Buffer, *BufferSize, Timeout); + error = libusb_control_transfer( + pdev->libusb_handle, LIBUSB_ENDPOINT_IN | LIBUSB_REQUEST_TYPE_VENDOR | Recipient, + bMS_Vendorcode, (UINT16)((InterfaceNumber << 8) | Ms_PageIndex), Ms_featureDescIndex, + Buffer, (UINT16)*BufferSize, Timeout); log_libusb_result(pdev->urbdrc->log, WLOG_DEBUG, "libusb_control_transfer", error); if (error >= 0) - *BufferSize = error; + *BufferSize = (UINT32)error; } if (error < 0) @@ -1208,6 +1210,7 @@ static int libusb_udev_isoch_transfer(IUDEVICE* idev, URBDRC_CHANNEL_CALLBACK* c UINT32 NumberOfPackets, UINT32 BufferSize, const BYTE* Buffer, t_isoch_transfer_cb cb, UINT32 Timeout) { + int rc; UINT32 iso_packet_size; UDEVICE* pdev = (UDEVICE*)idev; ASYNC_TRANSFER_USER_DATA* user_data; @@ -1233,7 +1236,7 @@ static int libusb_udev_isoch_transfer(IUDEVICE* idev, URBDRC_CHANNEL_CALLBACK* c Stream_Seek(user_data->data, (NumberOfPackets * 12)); iso_packet_size = BufferSize / NumberOfPackets; - iso_transfer = libusb_alloc_transfer(NumberOfPackets); + iso_transfer = libusb_alloc_transfer((int)NumberOfPackets); if (iso_transfer == NULL) { @@ -1257,7 +1260,10 @@ static int libusb_udev_isoch_transfer(IUDEVICE* idev, URBDRC_CHANNEL_CALLBACK* c request_free(iso_transfer); return -1; } - return libusb_submit_transfer(iso_transfer); + rc = libusb_submit_transfer(iso_transfer); + if (log_libusb_result(urbdrc->log, WLOG_ERROR, "", rc)) + return -1; + return rc; } static BOOL libusb_udev_control_transfer(IUDEVICE* idev, UINT32 RequestId, UINT32 EndpointAddress, @@ -1268,11 +1274,14 @@ static BOOL libusb_udev_control_transfer(IUDEVICE* idev, UINT32 RequestId, UINT3 int status = 0; UDEVICE* pdev = (UDEVICE*)idev; + WINPR_ASSERT(BufferSize); + WINPR_ASSERT(*BufferSize <= UINT16_MAX); + if (!pdev || !pdev->urbdrc) return FALSE; status = libusb_control_transfer(pdev->libusb_handle, bmRequestType, Request, Value, Index, - Buffer, *BufferSize, Timeout); + Buffer, (UINT16)*BufferSize, Timeout); if (status >= 0) *BufferSize = (UINT32)status; @@ -1291,6 +1300,7 @@ static int libusb_udev_bulk_or_interrupt_transfer(IUDEVICE* idev, URBDRC_CHANNEL BOOL NoAck, UINT32 BufferSize, const BYTE* data, t_isoch_transfer_cb cb, UINT32 Timeout) { + int rc; UINT32 transfer_type; UDEVICE* pdev = (UDEVICE*)idev; const LIBUSB_ENDPOINT_DESCEIPTOR* ep_desc; @@ -1367,7 +1377,10 @@ static int libusb_udev_bulk_or_interrupt_transfer(IUDEVICE* idev, URBDRC_CHANNEL request_free(transfer); return -1; } - return libusb_submit_transfer(transfer); + rc = libusb_submit_transfer(transfer); + if (log_libusb_result(urbdrc->log, WLOG_ERROR, "", rc)) + return -1; + return rc; } static int func_cancel_xact_request(URBDRC_PLUGIN* urbdrc, struct libusb_transfer* transfer) @@ -1501,6 +1514,8 @@ static void udev_free(IUDEVICE* idev) static void udev_load_interface(UDEVICE* pdev) { + WINPR_ASSERT(pdev); + /* load interface */ /* Basic */ BASIC_STATE_FUNC_REGISTER(channelManager, pdev); @@ -1544,47 +1559,52 @@ static void udev_load_interface(UDEVICE* pdev) static int udev_get_device_handle(URBDRC_PLUGIN* urbdrc, libusb_context* ctx, UDEVICE* pdev, UINT16 bus_number, UINT16 dev_number) { - int error; - ssize_t i, total_device; - uint8_t port_numbers[16]; - LIBUSB_DEVICE** libusb_list; - total_device = libusb_get_device_list(ctx, &libusb_list); - /* Look for device. */ - error = -1; + int error = -1; + ssize_t i; + uint8_t port_numbers[16] = { 0 }; + LIBUSB_DEVICE** libusb_list = NULL; + const ssize_t total_device = libusb_get_device_list(ctx, &libusb_list); + WINPR_ASSERT(urbdrc); + + /* Look for device. */ for (i = 0; i < total_device; i++) { LIBUSB_DEVICE* dev = libusb_list[i]; if ((bus_number != libusb_get_bus_number(dev)) || (dev_number != libusb_get_device_address(dev))) - continue; - - error = libusb_open(dev, &pdev->libusb_handle); - - if (log_libusb_result(urbdrc->log, WLOG_ERROR, "libusb_open", error)) - break; - - /* get port number */ - error = libusb_get_port_numbers(dev, port_numbers, sizeof(port_numbers)); - - if (error < 1) + libusb_unref_device(dev); + else { - /* Prevent open hub, treat as error. */ - log_libusb_result(urbdrc->log, WLOG_ERROR, "libusb_get_port_numbers", error); - break; + error = libusb_open(dev, &pdev->libusb_handle); + + if (log_libusb_result(urbdrc->log, WLOG_ERROR, "libusb_open", error)) + { + libusb_unref_device(dev); + continue; + } + + /* get port number */ + error = libusb_get_port_numbers(dev, port_numbers, sizeof(port_numbers)); + if (error < 1) + { + /* Prevent open hub, treat as error. */ + log_libusb_result(urbdrc->log, WLOG_ERROR, "libusb_get_port_numbers", error); + libusb_unref_device(dev); + continue; + } + + pdev->port_number = port_numbers[(error - 1)]; + error = 0; + WLog_Print(urbdrc->log, WLOG_DEBUG, " Port: %d", pdev->port_number); + /* gen device path */ + sprintf(pdev->path, "%" PRIu16 "-%d", bus_number, pdev->port_number); + + WLog_Print(urbdrc->log, WLOG_DEBUG, " DevPath: %s", pdev->path); } - - pdev->port_number = port_numbers[(error - 1)]; - error = 0; - WLog_Print(urbdrc->log, WLOG_DEBUG, " Port: %d", pdev->port_number); - /* gen device path */ - sprintf(pdev->path, "%" PRIu16 "-%d", bus_number, pdev->port_number); - - WLog_Print(urbdrc->log, WLOG_DEBUG, " DevPath: %s", pdev->path); - break; } - libusb_free_device_list(libusb_list, 1); + libusb_free_device_list(libusb_list, 0); if (error < 0) return -1; @@ -1594,33 +1614,35 @@ static int udev_get_device_handle(URBDRC_PLUGIN* urbdrc, libusb_context* ctx, UD static int udev_get_hub_handle(URBDRC_PLUGIN* urbdrc, libusb_context* ctx, UDEVICE* pdev, UINT16 bus_number, UINT16 dev_number) { - int error; - ssize_t i, total_device; - LIBUSB_DEVICE** libusb_list; - LIBUSB_DEVICE_HANDLE* handle; - total_device = libusb_get_device_list(ctx, &libusb_list); + int error = -1; + ssize_t i; + LIBUSB_DEVICE** libusb_list = NULL; + LIBUSB_DEVICE_HANDLE* handle = NULL; + const ssize_t total_device = libusb_get_device_list(ctx, &libusb_list); + + WINPR_ASSERT(urbdrc); /* Look for device hub. */ - error = -1; - for (i = 0; i < total_device; i++) { LIBUSB_DEVICE* dev = libusb_list[i]; if ((bus_number != libusb_get_bus_number(dev)) || (1 != libusb_get_device_address(dev))) /* Root hub allways first on bus. */ - continue; + libusb_unref_device(dev); + else + { + WLog_Print(urbdrc->log, WLOG_DEBUG, " Open hub: %" PRIu16 "", bus_number); + error = libusb_open(dev, &handle); - WLog_Print(urbdrc->log, WLOG_DEBUG, " Open hub: %" PRIu16 "", bus_number); - error = libusb_open(dev, &handle); - - if (!log_libusb_result(urbdrc->log, WLOG_ERROR, "libusb_open", error)) - pdev->hub_handle = handle; - - break; + if (!log_libusb_result(urbdrc->log, WLOG_ERROR, "libusb_open", error)) + pdev->hub_handle = handle; + else + libusb_unref_device(dev); + } } - libusb_free_device_list(libusb_list, 1); + libusb_free_device_list(libusb_list, 0); if (error < 0) return -1; @@ -1649,6 +1671,9 @@ static IUDEVICE* udev_init(URBDRC_PLUGIN* urbdrc, libusb_context* context, LIBUS LIBUSB_DEVICE_DESCRIPTOR* devDescriptor; LIBUSB_CONFIG_DESCRIPTOR* config_temp; LIBUSB_INTERFACE_DESCRIPTOR interface_temp; + + WINPR_ASSERT(urbdrc); + pdev = (PUDEVICE)calloc(1, sizeof(UDEVICE)); if (!pdev) @@ -1767,7 +1792,10 @@ size_t udev_new_by_id(URBDRC_PLUGIN* urbdrc, libusb_context* ctx, UINT16 idVendo WLog_Print(urbdrc->log, WLOG_INFO, "VID: 0x%04" PRIX16 ", PID: 0x%04" PRIX16 "", idVendor, idProduct); total_device = libusb_get_device_list(ctx, &libusb_list); - array = (UDEVICE**)calloc(total_device, sizeof(UDEVICE*)); + if (total_device < 0) + return 0; + + array = (UDEVICE**)calloc((size_t)total_device, sizeof(UDEVICE*)); if (!array) goto fail; @@ -1786,9 +1814,7 @@ size_t udev_new_by_id(URBDRC_PLUGIN* urbdrc, libusb_context* ctx, UINT16 idVendo num++; } else - { libusb_unref_device(dev); - } free(descriptor); } diff --git a/channels/urbdrc/client/urbdrc_main.c b/channels/urbdrc/client/urbdrc_main.c index f32e4cce3..ea39756f1 100644 --- a/channels/urbdrc/client/urbdrc_main.c +++ b/channels/urbdrc/client/urbdrc_main.c @@ -970,8 +970,12 @@ UINT DVCPluginEntry(IDRDYNVC_ENTRY_POINTS* pEntryPoints) urbdrc->vchannel_status = INIT_CHANNEL_IN; status = pEntryPoints->RegisterPlugin(pEntryPoints, URBDRC_CHANNEL_NAME, &urbdrc->iface); + /* After we register the plugin free will be taken care of by dynamic channel */ if (status != CHANNEL_RC_OK) + { + free(urbdrc); goto fail; + } urbdrc->log = WLog_Get(TAG); @@ -989,7 +993,6 @@ UINT DVCPluginEntry(IDRDYNVC_ENTRY_POINTS* pEntryPoints) return urbdrc_load_udevman_addin(&urbdrc->iface, urbdrc->subsystem, args); fail: - urbdrc_plugin_terminated(&urbdrc->iface); return status; } diff --git a/channels/urbdrc/client/urbdrc_main.h b/channels/urbdrc/client/urbdrc_main.h index 0db8da4e7..ad6762150 100644 --- a/channels/urbdrc/client/urbdrc_main.h +++ b/channels/urbdrc/client/urbdrc_main.h @@ -149,7 +149,7 @@ struct _IUDEVICE int (*os_feature_descriptor_request)(IUDEVICE* idev, UINT32 RequestId, BYTE Recipient, BYTE InterfaceNumber, BYTE Ms_PageIndex, UINT16 Ms_featureDescIndex, UINT32* UsbdStatus, - UINT32* BufferSize, BYTE* Buffer, int Timeout); + UINT32* BufferSize, BYTE* Buffer, UINT32 Timeout); void (*cancel_all_transfer_request)(IUDEVICE* idev);