Merge pull request #6363 from akallabeth/urbdrc_double_free_fix

[URBDRC] Added return checks for replaced HashTable with ArrayList

channels/urbdrc/client/data_transfer.c

@@ -765,9 +765,7 @@ static void urb_isoch_transfer_cb(IUDEVICE* pdev, URBDRC_CHANNEL_CALLBACK* callb
 		Stream_Write_UINT32(out, OutputBufferSize); /** OutputBufferSize */
 		Stream_Seek(out, OutputBufferSize);
 
-		if (!pdev->isChannelClosed(pdev))
+		stream_write_and_free(callback->plugin, callback->channel, out);
-			callback->channel->Write(callback->channel, Stream_GetPosition(out), Stream_Buffer(out),
-			                         NULL);
 	}
 }
 

channels/urbdrc/client/libusb/libusb_udevice.c

@@ -75,12 +75,59 @@ struct _ASYNC_TRANSFER_USER_DATA
 	UINT32 OutputBufferSize;
 	URBDRC_CHANNEL_CALLBACK* callback;
 	t_isoch_transfer_cb cb;
-	wHashTable* queue;
+	wArrayList* queue;
 #if !defined(HAVE_STREAM_ID_API)
 	UINT32 streamID;
 #endif
 };
 
+static struct libusb_transfer* list_contains(wArrayList* list, UINT32 streamID)
+{
+	int x, count;
+	if (!list)
+		return NULL;
+	count = ArrayList_Count(list);
+	for (x = 0; x < count; x++)
+	{
+		struct libusb_transfer* transfer = ArrayList_GetItem(list, x);
+
+#if defined(HAVE_STREAM_ID_API)
+		const UINT32 currentID = libusb_transfer_get_stream_id(transfer);
+#else
+		const ASYNC_TRANSFER_USER_DATA* user_data = (ASYNC_TRANSFER_USER_DATA*)transfer->user_data;
+		const UINT32 currentID = user_data->streamID;
+#endif
+		if (currentID == streamID)
+			return transfer;
+	}
+	return NULL;
+}
+
+static UINT32 stream_id_from_buffer(struct libusb_transfer* transfer)
+{
+	if (!transfer)
+		return 0;
+#if defined(HAVE_STREAM_ID_API)
+	return libusb_transfer_get_stream_id(transfer);
+#else
+	ASYNC_TRANSFER_USER_DATA* user_data = (ASYNC_TRANSFER_USER_DATA*)transfer->user_data;
+	if (!user_data)
+		return 0;
+	return user_data->streamID;
+#endif
+}
+
+static void set_stream_id_for_buffer(struct libusb_transfer* transfer, UINT32 streamID)
+{
+#if defined(HAVE_STREAM_ID_API)
+	libusb_transfer_set_stream_id(transfer, streamID);
+#else
+	ASYNC_TRANSFER_USER_DATA* user_data = (ASYNC_TRANSFER_USER_DATA*)transfer->user_data;
+	if (!user_data)
+		return;
+	user_data->streamID = streamID;
+#endif
+}
 static BOOL log_libusb_result(wLog* log, DWORD lvl, const char* fmt, int error, ...)
 {
 	if (error < 0)
@@ -176,7 +223,6 @@ static ASYNC_TRANSFER_USER_DATA* async_transfer_user_data_new(IUDEVICE* idev, UI
 
 static void async_transfer_user_data_free(ASYNC_TRANSFER_USER_DATA* user_data)
 {
-
 	if (user_data)
 	{
 		Stream_Free(user_data->data, TRUE);
@@ -187,15 +233,12 @@ static void async_transfer_user_data_free(ASYNC_TRANSFER_USER_DATA* user_data)
 static void func_iso_callback(struct libusb_transfer* transfer)
 {
 	ASYNC_TRANSFER_USER_DATA* user_data = (ASYNC_TRANSFER_USER_DATA*)transfer->user_data;
-#if defined(HAVE_STREAM_ID_API)
+	const UINT32 streamID = stream_id_from_buffer(transfer);
-	const UINT32 streamID = libusb_transfer_get_stream_id(transfer);
+	wArrayList* list = user_data->queue;
-#else
-	const UINT32 streamID = user_data->streamID;
-#endif
 
+	ArrayList_Lock(list);
 	switch (transfer->status)
 	{
-
 		case LIBUSB_TRANSFER_COMPLETED:
 		{
 			int i;
@@ -235,7 +278,7 @@ static void func_iso_callback(struct libusb_transfer* transfer)
 			const UINT32 InterfaceId =
 			    ((STREAM_ID_PROXY << 30) | user_data->idev->get_ReqCompletion(user_data->idev));
 
-			if (HashTable_Contains(user_data->queue, (void*)(size_t)streamID))
+			if (list_contains(list, streamID))
 			{
 				if (!user_data->noack)
 				{
@@ -247,13 +290,14 @@ static void func_iso_callback(struct libusb_transfer* transfer)
 					              user_data->OutputBufferSize);
 					user_data->data = NULL;
 				}
-				HashTable_Remove(user_data->queue, (void*)(size_t)streamID);
+				ArrayList_Remove(list, transfer);
 			}
 		}
 		break;
 		default:
 			break;
 	}
+	ArrayList_Unlock(list);
 }
 
 static const LIBUSB_ENDPOINT_DESCEIPTOR* func_get_ep_desc(LIBUSB_CONFIG_DESCRIPTOR* LibusbConfig,
@@ -289,6 +333,7 @@ static void func_bulk_transfer_cb(struct libusb_transfer* transfer)
 {
 	ASYNC_TRANSFER_USER_DATA* user_data;
 	uint32_t streamID;
+	wArrayList* list;
 
 	user_data = (ASYNC_TRANSFER_USER_DATA*)transfer->user_data;
 	if (!user_data)
@@ -296,14 +341,11 @@ static void func_bulk_transfer_cb(struct libusb_transfer* transfer)
 		WLog_ERR(TAG, "[%s]: Invalid transfer->user_data!");
 		return;
 	}
+	list = user_data->queue;
+	ArrayList_Lock(list);
+	streamID = stream_id_from_buffer(transfer);
 
-#if defined(HAVE_STREAM_ID_API)
+	if (list_contains(list, streamID))
-	streamID = libusb_transfer_get_stream_id(transfer);
-#else
-	streamID = user_data->streamID;
-#endif
-
-	if (HashTable_Contains(user_data->queue, (void*)(size_t)streamID))
 	{
 		const UINT32 InterfaceId =
 		    ((STREAM_ID_PROXY << 30) | user_data->idev->get_ReqCompletion(user_data->idev));
@@ -314,8 +356,9 @@ static void func_bulk_transfer_cb(struct libusb_transfer* transfer)
 		              transfer->status, user_data->StartFrame, user_data->ErrorCount,
 		              transfer->actual_length);
 		user_data->data = NULL;
-		HashTable_Remove(user_data->queue, (void*)(size_t)streamID);
+		ArrayList_Remove(list, transfer);
 	}
+	ArrayList_Unlock(list);
 }
 
 static BOOL func_set_usbd_status(URBDRC_PLUGIN* urbdrc, UDEVICE* pdev, UINT32* status,
@@ -884,7 +927,10 @@ static int libusb_udev_os_feature_descriptor_request(IUDEVICE* idev, UINT32 Requ
 		                                LIBUSB_ENDPOINT_IN | LIBUSB_REQUEST_TYPE_VENDOR | Recipient,
 		                                bMS_Vendorcode, (InterfaceNumber << 8) | Ms_PageIndex,
 		                                Ms_featureDescIndex, Buffer, *BufferSize, Timeout);
-		*BufferSize = error;
+		log_libusb_result(pdev->urbdrc->log, WLOG_DEBUG, "libusb_control_transfer", error);
+
+		if (error >= 0)
+			*BufferSize = error;
 	}
 
 	if (error < 0)
@@ -1044,10 +1090,28 @@ static int libusb_udev_is_already_send(IUDEVICE* idev)
 	return (pdev->status & URBDRC_DEVICE_ALREADY_SEND) ? 1 : 0;
 }
 
+/* This is called from channel cleanup code.
+ * Avoid double free, just remove the device and mark the channel closed. */
+static void libusb_udev_mark_channel_closed(IUDEVICE* idev)
+{
+	UDEVICE* pdev = (UDEVICE*)idev;
+	if (pdev && ((pdev->status & URBDRC_DEVICE_CHANNEL_CLOSED) == 0))
+	{
+		URBDRC_PLUGIN* urbdrc = pdev->urbdrc;
+		const uint8_t busNr = idev->get_bus_number(idev);
+		const uint8_t devNr = idev->get_dev_number(idev);
+
+		pdev->status |= URBDRC_DEVICE_CHANNEL_CLOSED;
+		urbdrc->udevman->unregister_udevice(urbdrc->udevman, busNr, devNr);
+	}
+}
+
+/* This is called by local events where the device is removed or in an error
+ * state. Remove the device from redirection and close the channel. */
 static void libusb_udev_channel_closed(IUDEVICE* idev)
 {
 	UDEVICE* pdev = (UDEVICE*)idev;
-	if (pdev)
+	if (pdev && ((pdev->status & URBDRC_DEVICE_CHANNEL_CLOSED) == 0))
 	{
 		URBDRC_PLUGIN* urbdrc = pdev->urbdrc;
 		const uint8_t busNr = idev->get_bus_number(idev);
@@ -1061,10 +1125,8 @@ static void libusb_udev_channel_closed(IUDEVICE* idev)
 		pdev->status |= URBDRC_DEVICE_CHANNEL_CLOSED;
 
 		if (channel)
-		{
-			/* Notify the server the device is no longer available. */
 			channel->Write(channel, 0, NULL, NULL);
-		}
+
 		urbdrc->udevman->unregister_udevice(urbdrc->udevman, busNr, devNr);
 	}
 }
@@ -1165,14 +1227,18 @@ static int libusb_udev_isoch_transfer(IUDEVICE* idev, URBDRC_CHANNEL_CALLBACK* c
 	libusb_fill_iso_transfer(iso_transfer, pdev->libusb_handle, EndpointAddress,
 	                         Stream_Pointer(user_data->data), BufferSize, NumberOfPackets,
 	                         func_iso_callback, user_data, Timeout);
-#if defined(HAVE_STREAM_ID_API)
+	set_stream_id_for_buffer(iso_transfer, streamID);
-	libusb_transfer_set_stream_id(iso_transfer, streamID);
-#else
-	user_data->streamID = streamID;
-#endif
 	libusb_set_iso_packet_lengths(iso_transfer, iso_packet_size);
 
-	HashTable_Add(pdev->request_queue, (void*)(size_t)streamID, iso_transfer);
+	if (ArrayList_Add(pdev->request_queue, iso_transfer) < 0)
+	{
+		WLog_Print(urbdrc->log, WLOG_WARN,
+		           "Failed to queue iso transfer, streamID %08" PRIx32 " already in use!",
+		           streamID);
+		async_transfer_user_data_free(user_data);
+		libusb_free_transfer(iso_transfer);
+		return -1;
+	}
 	return libusb_submit_transfer(iso_transfer);
 }
 
@@ -1277,21 +1343,24 @@ static int libusb_udev_bulk_or_interrupt_transfer(IUDEVICE* idev, URBDRC_CHANNEL
 			return -1;
 	}
 
-#if defined(HAVE_STREAM_ID_API)
+	set_stream_id_for_buffer(transfer, streamID);
-	libusb_transfer_set_stream_id(transfer, streamID);
+
-#else
+	if (ArrayList_Add(pdev->request_queue, transfer) < 0)
-	user_data->streamID = streamID;
+	{
-#endif
+		WLog_Print(urbdrc->log, WLOG_WARN,
-	HashTable_Add(pdev->request_queue, (void*)(size_t)streamID, transfer);
+		           "Failed to queue transfer, streamID %08" PRIx32 " already in use!", streamID);
+		async_transfer_user_data_free(user_data);
+		libusb_free_transfer(transfer);
+		return -1;
+	}
 	return libusb_submit_transfer(transfer);
 }
 
-static int func_cancel_xact_request(URBDRC_PLUGIN* urbdrc, wHashTable* queue, uint32_t streamID,
+static int func_cancel_xact_request(URBDRC_PLUGIN* urbdrc, struct libusb_transfer* transfer)
-                                    struct libusb_transfer* transfer)
 {
 	int status;
 
-	if (!urbdrc || !queue || !transfer)
+	if (!urbdrc || !transfer)
 		return -1;
 
 	status = libusb_cancel_transfer(transfer);
@@ -1306,50 +1375,51 @@ static int func_cancel_xact_request(URBDRC_PLUGIN* urbdrc, wHashTable* queue, ui
 
 	return 0;
 }
+
 static void libusb_udev_cancel_all_transfer_request(IUDEVICE* idev)
 {
 	UDEVICE* pdev = (UDEVICE*)idev;
-	ULONG_PTR* keys;
 	int count, x;
 
 	if (!pdev || !pdev->request_queue || !pdev->urbdrc)
 		return;
 
-	count = HashTable_GetKeys(pdev->request_queue, &keys);
+	ArrayList_Lock(pdev->request_queue);
+	count = ArrayList_Count(pdev->request_queue);
 
 	for (x = 0; x < count; x++)
 	{
-		struct libusb_transfer* transfer =
+		struct libusb_transfer* transfer = ArrayList_GetItem(pdev->request_queue, x);
-		    HashTable_GetItemValue(pdev->request_queue, (void*)keys[x]);
+		func_cancel_xact_request(pdev->urbdrc, transfer);
-		func_cancel_xact_request(pdev->urbdrc, pdev->request_queue, (uint32_t)keys[x], transfer);
 	}
 
-	free(keys);
+	ArrayList_Unlock(pdev->request_queue);
 }
 
 static int libusb_udev_cancel_transfer_request(IUDEVICE* idev, UINT32 RequestId)
 {
+	int rc = -1;
 	UDEVICE* pdev = (UDEVICE*)idev;
 	struct libusb_transfer* transfer;
-	URBDRC_PLUGIN* urbdrc;
-	BOOL id1;
-	uint32_t cancelID;
 	uint32_t cancelID1 = 0x40000000 | RequestId;
 	uint32_t cancelID2 = 0x80000000 | RequestId;
 
 	if (!idev || !pdev->urbdrc || !pdev->request_queue)
 		return -1;
 
-	id1 = HashTable_Contains(pdev->request_queue, (void*)(size_t)cancelID1);
+	ArrayList_Lock(pdev->request_queue);
+	transfer = list_contains(pdev->request_queue, cancelID1);
+	if (!transfer)
+		transfer = list_contains(pdev->request_queue, cancelID2);
 
-	if (!id1)
+	if (transfer)
-		return -1;
+	{
+		URBDRC_PLUGIN* urbdrc = (URBDRC_PLUGIN*)pdev->urbdrc;
 
-	urbdrc = (URBDRC_PLUGIN*)pdev->urbdrc;
+		rc = func_cancel_xact_request(urbdrc, transfer);
-	cancelID = (id1) ? cancelID1 : cancelID2;
+	}
-
+	ArrayList_Unlock(pdev->request_queue);
-	transfer = HashTable_GetItemValue(pdev->request_queue, (void*)(size_t)cancelID);
+	return rc;
-	return func_cancel_xact_request(urbdrc, pdev->request_queue, cancelID, transfer);
 }
 
 BASIC_STATE_FUNC_DEFINED(channelManager, IWTSVirtualChannelManager*)
@@ -1404,7 +1474,7 @@ static void udev_free(IUDEVICE* idev)
 
 	/* release all interface and  attach kernel driver */
 	udev->iface.attach_kernel_driver(idev);
-	HashTable_Free(udev->request_queue);
+	ArrayList_Free(udev->request_queue);
 	/* free the config descriptor that send from windows */
 	msusb_msconfig_free(udev->MsConfig);
 	libusb_close(udev->libusb_handle);
@@ -1434,6 +1504,7 @@ static void udev_load_interface(UDEVICE* pdev)
 	pdev->iface.isChannelClosed = libusb_udev_is_channel_closed;
 	pdev->iface.setAlreadySend = libusb_udev_set_already_send;
 	pdev->iface.setChannelClosed = libusb_udev_channel_closed;
+	pdev->iface.markChannelClosed = libusb_udev_mark_channel_closed;
 	pdev->iface.getPath = libusb_udev_get_path;
 	/* Transfer */
 	pdev->iface.isoch_transfer = libusb_udev_isoch_transfer;
@@ -1541,6 +1612,7 @@ static void request_free(void* value)
 
 	user_data = (ASYNC_TRANSFER_USER_DATA*)transfer->user_data;
 	async_transfer_user_data_free(user_data);
+	transfer->user_data = NULL;
 }
 
 static IUDEVICE* udev_init(URBDRC_PLUGIN* urbdrc, libusb_context* context, LIBUSB_DEVICE* device,
@@ -1639,12 +1711,12 @@ static IUDEVICE* udev_init(URBDRC_PLUGIN* urbdrc, libusb_context* context, LIBUS
 	/* initialize pdev */
 	pdev->bus_number = bus_number;
 	pdev->dev_number = dev_number;
-	pdev->request_queue = HashTable_New(TRUE);
+	pdev->request_queue = ArrayList_New(TRUE);
 
 	if (!pdev->request_queue)
 		goto fail;
 
-	pdev->request_queue->valueFree = request_free;
+	ArrayList_Object(pdev->request_queue)->fnObjectFree = request_free;
 
 	/* set config of windows */
 	pdev->MsConfig = msusb_msconfig_new();

channels/urbdrc/client/libusb/libusb_udevice.h

@@ -63,7 +63,7 @@ struct _UDEVICE
 	MSUSB_CONFIG_DESCRIPTOR* MsConfig;
 	LIBUSB_CONFIG_DESCRIPTOR* LibusbConfig;
 
-	wHashTable* request_queue;
+	wArrayList* request_queue;
 
 	URBDRC_PLUGIN* urbdrc;
 };

channels/urbdrc/client/libusb/libusb_udevman.c

@@ -399,6 +399,36 @@ static IUDEVICE* udevman_get_udevice_by_UsbDevice(IUDEVMAN* idevman, UINT32 UsbD
 	return NULL;
 }
 
+static IUDEVICE* udevman_get_udevice_by_ChannelID(IUDEVMAN* idevman, UINT32 channelID)
+{
+	UDEVICE* pdev;
+	URBDRC_PLUGIN* urbdrc;
+
+	if (!idevman || !idevman->plugin)
+		return NULL;
+
+	/* Mask highest 2 bits, must be ignored */
+	urbdrc = (URBDRC_PLUGIN*)idevman->plugin;
+	idevman->loading_lock(idevman);
+	idevman->rewind(idevman);
+
+	while (idevman->has_next(idevman))
+	{
+		pdev = (UDEVICE*)idevman->get_next(idevman);
+
+		if (pdev->channelID == channelID)
+		{
+			idevman->loading_unlock(idevman);
+			return (IUDEVICE*)pdev;
+		}
+	}
+
+	idevman->loading_unlock(idevman);
+	WLog_Print(urbdrc->log, WLOG_WARN, "Failed to find a USB device mapped to channelID=%08" PRIx32,
+	           channelID);
+	return NULL;
+}
+
 static void udevman_loading_lock(IUDEVMAN* idevman)
 {
 	UDEVMAN* udevman = (UDEVMAN*)idevman;
@@ -786,6 +816,7 @@ static void udevman_load_interface(UDEVMAN* udevman)
 	udevman->iface.register_udevice = udevman_register_udevice;
 	udevman->iface.unregister_udevice = udevman_unregister_udevice;
 	udevman->iface.get_udevice_by_UsbDevice = udevman_get_udevice_by_UsbDevice;
+	udevman->iface.get_udevice_by_ChannelID = udevman_get_udevice_by_ChannelID;
 	/* Extension */
 	udevman->iface.isAutoAdd = udevman_is_auto_add;
 	/* Basic state */

channels/urbdrc/client/urbdrc_main.c

@@ -621,6 +621,12 @@ static UINT urbdrc_on_close(IWTSVirtualChannelCallback* pChannelCallback)
 				UINT32 control = callback->channel_mgr->GetChannelId(callback->channel);
 				if (udevman->controlChannelId == control)
 					udevman->status |= URBDRC_DEVICE_CHANNEL_CLOSED;
+				else
+				{ /* Need to notify the local backend the device is gone */
+					IUDEVICE* pdev = udevman->get_udevice_by_ChannelID(udevman, control);
+					if (pdev)
+						pdev->markChannelClosed(pdev);
+				}
 			}
 		}
 	}

channels/urbdrc/client/urbdrc_main.h

@@ -174,6 +174,7 @@ struct _IUDEVICE
 
 	void (*setAlreadySend)(IUDEVICE* idev);
 	void (*setChannelClosed)(IUDEVICE* idev);
+	void (*markChannelClosed)(IUDEVICE* idev);
 	char* (*getPath)(IUDEVICE* idev);
 
 	void (*free)(IUDEVICE* idev);
@@ -205,6 +206,7 @@ struct _IUDEVMAN
 	                           UINT16 idProduct, UINT32 flag);
 	IUDEVICE* (*get_next)(IUDEVMAN* idevman);
 	IUDEVICE* (*get_udevice_by_UsbDevice)(IUDEVMAN* idevman, UINT32 UsbDevice);
+	IUDEVICE* (*get_udevice_by_ChannelID)(IUDEVMAN* idevman, UINT32 channelID);
 
 	/* Extension */
 	int (*isAutoAdd)(IUDEVMAN* idevman);

channels/urbdrc/common/urbdrc_helpers.c

@@ -391,7 +391,7 @@ void urbdrc_dump_message(wLog* log, BOOL client, BOOL write, wStream* s)
 	pos = Stream_GetPosition(s);
 	if (write)
 	{
-		length = Stream_GetPosition(s);
+		length = pos;
 		Stream_SetPosition(s, 0);
 	}
 	else
@@ -407,7 +407,7 @@ void urbdrc_dump_message(wLog* log, BOOL client, BOOL write, wStream* s)
 
 	WLog_Print(log, WLOG_DEBUG,
 	           "[%-5s] %s [%08" PRIx32 "] InterfaceId=%08" PRIx32 ", MessageId=%08" PRIx32
-	           ", FunctionId=%08" PRIx32 ", length=%" PRIdz,
+	           ", FunctionId=%08" PRIx32 ", length=%" PRIuz,
 	           type, call_to_string(client, InterfaceId, FunctionId), FunctionId, InterfaceId,
 	           MessageId, FunctionId, length);
 #if defined(WITH_DEBUG_URBDRC)