channels: patch rdpdr/smartcard valgrind leaks, fix hang on disconnect

channels/rdpdr/client/irp.c

@@ -75,6 +75,10 @@ IRP* irp_new(DEVMAN* devman, wStream* s)
 		return NULL;
 
 	irp = (IRP*) _aligned_malloc(sizeof(IRP), MEMORY_ALLOCATION_ALIGNMENT);
+
+	if (!irp)
+		return NULL;
+
 	ZeroMemory(irp, sizeof(IRP));
 
 	irp->input = s;

channels/rdpdr/client/rdpdr_main.c

@@ -452,7 +452,7 @@ static void* drive_hotplug_thread_func(void* arg)
 
 	if (mfd < 0)
 	{
-		WLog_ERR(TAG,  "ERROR: Unable to open /proc/mounts.");
+		WLog_ERR(TAG, "ERROR: Unable to open /proc/mounts.");
 		return NULL;
 	}
 
@@ -539,8 +539,8 @@ static void rdpdr_send_client_announce_reply(rdpdrPlugin* rdpdr)
 
 	s = Stream_New(NULL, 12);
 
-	Stream_Write_UINT16(s, RDPDR_CTYP_CORE);
-	Stream_Write_UINT16(s, PAKID_CORE_CLIENTID_CONFIRM);
+	Stream_Write_UINT16(s, RDPDR_CTYP_CORE); /* Component (2 bytes) */
+	Stream_Write_UINT16(s, PAKID_CORE_CLIENTID_CONFIRM); /* PacketId (2 bytes) */
 
 	Stream_Write_UINT16(s, rdpdr->versionMajor);
 	Stream_Write_UINT16(s, rdpdr->versionMinor);
@@ -562,8 +562,8 @@ static void rdpdr_send_client_name_request(rdpdrPlugin* rdpdr)
 
 	s = Stream_New(NULL, 16 + computerNameLenW + 2);
 
-	Stream_Write_UINT16(s, RDPDR_CTYP_CORE);
-	Stream_Write_UINT16(s, PAKID_CORE_CLIENT_NAME);
+	Stream_Write_UINT16(s, RDPDR_CTYP_CORE); /* Component (2 bytes) */
+	Stream_Write_UINT16(s, PAKID_CORE_CLIENT_NAME); /* PacketId (2 bytes) */
 
 	Stream_Write_UINT32(s, 1); /* unicodeFlag, 0 for ASCII and 1 for Unicode */
 	Stream_Write_UINT32(s, 0); /* codePage, must be set to zero */
@@ -614,8 +614,8 @@ static void rdpdr_send_device_list_announce_request(rdpdrPlugin* rdpdr, BOOL use
 
 	s = Stream_New(NULL, 256);
 
-	Stream_Write_UINT16(s, RDPDR_CTYP_CORE);
-	Stream_Write_UINT16(s, PAKID_CORE_DEVICELIST_ANNOUNCE);
+	Stream_Write_UINT16(s, RDPDR_CTYP_CORE); /* Component (2 bytes) */
+	Stream_Write_UINT16(s, PAKID_CORE_DEVICELIST_ANNOUNCE); /* PacketId (2 bytes) */
 
 	count_pos = (int) Stream_GetPosition(s);
 	count = 0;
@@ -662,7 +662,7 @@ static void rdpdr_send_device_list_announce_request(rdpdrPlugin* rdpdr, BOOL use
 				Stream_Write(s, Stream_Buffer(device->data), data_len);
 
 			count++;
-			WLog_INFO(TAG,  "registered device #%d: %s (type=%d id=%d)",
+			WLog_INFO(TAG, "registered device #%d: %s (type=%d id=%d)",
 					  count, device->name, device->type, device->id);
 		}
 	}
@@ -716,16 +716,16 @@ static void rdpdr_process_init(rdpdrPlugin* rdpdr)
 static void rdpdr_process_receive(rdpdrPlugin* rdpdr, wStream* s)
 {
 	UINT16 component;
-	UINT16 packetID;
-	UINT32 deviceID;
+	UINT16 packetId;
+	UINT32 deviceId;
 	UINT32 status;
 
-	Stream_Read_UINT16(s, component);
-	Stream_Read_UINT16(s, packetID);
+	Stream_Read_UINT16(s, component); /* Component (2 bytes) */
+	Stream_Read_UINT16(s, packetId); /* PacketId (2 bytes) */
 
 	if (component == RDPDR_CTYP_CORE)
 	{
-		switch (packetID)
+		switch (packetId)
 		{
 			case PAKID_CORE_SERVER_ANNOUNCE:
 				rdpdr_process_server_announce_request(rdpdr, s);
@@ -750,7 +750,7 @@ static void rdpdr_process_receive(rdpdrPlugin* rdpdr, wStream* s)
 
 			case PAKID_CORE_DEVICE_REPLY:
 				/* connect to a specific resource */
-				Stream_Read_UINT32(s, deviceID);
+				Stream_Read_UINT32(s, deviceId);
 				Stream_Read_UINT32(s, status);
 				break;
 
@@ -760,17 +760,19 @@ static void rdpdr_process_receive(rdpdrPlugin* rdpdr, wStream* s)
 				break;
 
 			default:
+				WLog_ERR(TAG, "rdpdr_process_receive: RDPDR_CTYP_CORE unknown PacketId: 0x%04X", packetId);
 				break;
 
 		}
 	}
 	else if (component == RDPDR_CTYP_PRN)
 	{
-
+		WLog_ERR(TAG, "rdpdr_process_receive: RDPDR_CTYP_PRN unknown PacketId: 0x%04X", packetId);
 	}
 	else
 	{
-
+		WLog_ERR(TAG, "rdpdr_process_receive: unknown message: Component: 0x%04X PacketId: 0x%04X",
+				component, packetId);
 	}
 
 	Stream_Free(s, TRUE);
@@ -845,7 +847,7 @@ int rdpdr_send(rdpdrPlugin* rdpdr, wStream* s)
 	if (status != CHANNEL_RC_OK)
 	{
 		Stream_Free(s, TRUE);
-		WLog_ERR(TAG,  "rdpdr_send: VirtualChannelWrite failed %d", status);
+		WLog_ERR(TAG, "rdpdr_send: VirtualChannelWrite failed %d", status);
 	}
 
 	return status;
@@ -869,7 +871,7 @@ static void rdpdr_virtual_channel_event_data_received(rdpdrPlugin* rdpdr,
 
 	if (dataFlags & CHANNEL_FLAG_FIRST)
 	{
-		if (rdpdr->data_in != NULL)
+		if (rdpdr->data_in)
 			Stream_Free(rdpdr->data_in, TRUE);
 
 		rdpdr->data_in = Stream_New(NULL, totalLength);
@@ -883,14 +885,14 @@ static void rdpdr_virtual_channel_event_data_received(rdpdrPlugin* rdpdr,
 	{
 		if (Stream_Capacity(data_in) != Stream_GetPosition(data_in))
 		{
-			WLog_ERR(TAG,  "svc_plugin_process_received: read error\n");
+			WLog_ERR(TAG, "rdpdr_virtual_channel_event_data_received: read error\n");
 		}
 
 		rdpdr->data_in = NULL;
 		Stream_SealLength(data_in);
 		Stream_SetPosition(data_in, 0);
 
-		MessageQueue_Post(rdpdr->MsgPipe->In, NULL, 0, (void*) data_in, NULL);
+		MessageQueue_Post(rdpdr->queue, NULL, 0, (void*) data_in, NULL);
 	}
 }
 
@@ -903,7 +905,7 @@ static VOID VCAPITYPE rdpdr_virtual_channel_open_event(DWORD openHandle, UINT ev
 
 	if (!rdpdr)
 	{
-		WLog_ERR(TAG,  "rdpdr_virtual_channel_open_event: error no match\n");
+		WLog_ERR(TAG, "rdpdr_virtual_channel_open_event: error no match\n");
 		return;
 	}
 
@@ -929,10 +931,10 @@ static void* rdpdr_virtual_channel_client_thread(void* arg)
 
 	while (1)
 	{
-		if (!MessageQueue_Wait(rdpdr->MsgPipe->In))
+		if (!MessageQueue_Wait(rdpdr->queue))
 			break;
 
-		if (MessageQueue_Peek(rdpdr->MsgPipe->In, &message, TRUE))
+		if (MessageQueue_Peek(rdpdr->queue, &message, TRUE))
 		{
 			if (message.id == WMQ_QUIT)
 				break;
@@ -960,25 +962,25 @@ static void rdpdr_virtual_channel_event_connected(rdpdrPlugin* rdpdr, LPVOID pDa
 
 	if (status != CHANNEL_RC_OK)
 	{
-		WLog_ERR(TAG,  "rdpdr_virtual_channel_event_connected: open failed: status: %d\n", status);
+		WLog_ERR(TAG, "rdpdr_virtual_channel_event_connected: open failed: status: %d\n", status);
 		return;
 	}
 
-	rdpdr->MsgPipe = MessagePipe_New();
+	rdpdr->queue = MessageQueue_New(NULL);
 
 	rdpdr->thread = CreateThread(NULL, 0,
 			(LPTHREAD_START_ROUTINE) rdpdr_virtual_channel_client_thread, (void*) rdpdr, 0, NULL);
 }
 
 static void rdpdr_virtual_channel_event_terminated(rdpdrPlugin* rdpdr)
 {
-	if (rdpdr->MsgPipe)
+	if (rdpdr->queue)
 	{
-		MessagePipe_PostQuit(rdpdr->MsgPipe, 0);
+		MessageQueue_PostQuit(rdpdr->queue, 0);
 		WaitForSingleObject(rdpdr->thread, INFINITE);
 
-		MessagePipe_Free(rdpdr->MsgPipe);
-		rdpdr->MsgPipe = NULL;
+		MessageQueue_Free(rdpdr->queue);
+		rdpdr->queue = NULL;
 
 		CloseHandle(rdpdr->thread);
 		rdpdr->thread = NULL;

channels/rdpdr/client/rdpdr_main.h

@@ -47,7 +47,7 @@ struct rdpdr_plugin
 	wStream* data_in;
 	void* InitHandle;
 	DWORD OpenHandle;
-	wMessagePipe* MsgPipe;
+	wMessageQueue* queue;
 
 	DEVMAN* devman;
 

channels/smartcard/client/smartcard_main.c

@@ -105,6 +105,9 @@ void smartcard_context_free(SMARTCARD_CONTEXT* pContext)
 	if (!pContext)
 		return;
 
+	/* cancel blocking calls like SCardGetStatusChange */
+	SCardCancel(pContext->hContext);
+
 	MessageQueue_PostQuit(pContext->IrpQueue, 0);
 	WaitForSingleObject(pContext->thread, INFINITE);
 	CloseHandle(pContext->thread);
@@ -118,14 +121,24 @@ static void smartcard_free(DEVICE* device)
 {
 	SMARTCARD_DEVICE* smartcard = (SMARTCARD_DEVICE*) device;
 
-	MessageQueue_PostQuit(smartcard->IrpQueue, 0);
-	WaitForSingleObject(smartcard->thread, INFINITE);
+	if (smartcard->IrpQueue)
+	{
+		MessageQueue_PostQuit(smartcard->IrpQueue, 0);
+		WaitForSingleObject(smartcard->thread, INFINITE);
+
+		MessageQueue_Free(smartcard->IrpQueue);
+		smartcard->IrpQueue = NULL;
 
-	CloseHandle(smartcard->thread);
+		CloseHandle(smartcard->thread);
+		smartcard->thread = NULL;
+	}
 
-	Stream_Free(smartcard->device.data, TRUE);
+	if (smartcard->device.data)
+	{
+		Stream_Free(smartcard->device.data, TRUE);
+		smartcard->device.data = NULL;
+	}
 
-	MessageQueue_Free(smartcard->IrpQueue);
 	ListDictionary_Free(smartcard->rgSCardContextList);
 	ListDictionary_Free(smartcard->rgOutstandingMessages);
 	Queue_Free(smartcard->CompletedIrpQueue);
@@ -414,6 +427,7 @@ static void* smartcard_thread_func(void* arg)
 						{
 							WaitForSingleObject(irp->thread, INFINITE);
 							CloseHandle(irp->thread);
+							irp->thread = NULL;
 						}
 
 						smartcard_complete_irp(smartcard, irp);
@@ -441,6 +455,7 @@ static void* smartcard_thread_func(void* arg)
 				{
 					WaitForSingleObject(irp->thread, INFINITE);
 					CloseHandle(irp->thread);
+					irp->thread = NULL;
 				}
 
 				smartcard_complete_irp(smartcard, irp);
@@ -509,9 +524,15 @@ int DeviceServiceEntry(PDEVICE_SERVICE_ENTRY_POINTS pEntryPoints)
 	smartcard->log = WLog_Get("com.freerdp.channel.smartcard.client");
 
 	smartcard->IrpQueue = MessageQueue_New(NULL);
+
+	smartcard->CompletedIrpQueue = Queue_New(TRUE, -1, -1);
+
 	smartcard->rgSCardContextList = ListDictionary_New(TRUE);
+
+	ListDictionary_ValueObject(smartcard->rgSCardContextList)->fnObjectFree =
+			(OBJECT_FREE_FN) smartcard_context_free;
+
 	smartcard->rgOutstandingMessages = ListDictionary_New(TRUE);
-	smartcard->CompletedIrpQueue = Queue_New(TRUE, -1, -1);
 
 	smartcard->thread = CreateThread(NULL, 0, (LPTHREAD_START_ROUTINE) smartcard_thread_func,
 			smartcard, CREATE_SUSPENDED, NULL);

channels/smartcard/client/smartcard_operations.c

@@ -555,8 +555,8 @@ static UINT32 smartcard_ConnectA_Decode(SMARTCARD_DEVICE* smartcard, SMARTCARD_O
 static UINT32 smartcard_ConnectA_Call(SMARTCARD_DEVICE* smartcard, SMARTCARD_OPERATION* operation, ConnectA_Call* call)
 {
 	LONG status;
-	SCARDHANDLE hCard;
-	Connect_Return ret;
+	SCARDHANDLE hCard = 0;
+	Connect_Return ret = { 0 };
 	IRP* irp = operation->irp;
 
 	if ((call->Common.dwPreferredProtocols == SCARD_PROTOCOL_UNDEFINED) &&
@@ -604,8 +604,8 @@ static UINT32 smartcard_ConnectW_Decode(SMARTCARD_DEVICE* smartcard, SMARTCARD_O
 static UINT32 smartcard_ConnectW_Call(SMARTCARD_DEVICE* smartcard, SMARTCARD_OPERATION* operation, ConnectW_Call* call)
 {
 	LONG status;
-	SCARDHANDLE hCard;
-	Connect_Return ret;
+	SCARDHANDLE hCard = 0;
+	Connect_Return ret = { 0 };
 	IRP* irp = operation->irp;
 
 	if ((call->Common.dwPreferredProtocols == SCARD_PROTOCOL_UNDEFINED) &&
@@ -1020,9 +1020,11 @@ static UINT32 smartcard_GetAttrib_Call(SMARTCARD_DEVICE* smartcard, SMARTCARD_OP
 	if (ret.ReturnCode)
 	{
 		WLog_Print(smartcard->log, WLOG_WARN,
-				   "SCardGetAttrib: %s (0x%08X) cbAttrLen: %d\n",
+				   "SCardGetAttrib: %s (0x%08X) cbAttrLen: %d",
 				   SCardGetAttributeString(call->dwAttrId), call->dwAttrId, call->cbAttrLen);
 		Stream_Zero(irp->output, 256);
+
+		free(ret.pbAttr);
 		return ret.ReturnCode;
 	}
 
@@ -1198,15 +1200,15 @@ UINT32 smartcard_irp_device_control_decode(SMARTCARD_DEVICE* smartcard, SMARTCAR
 	if (Stream_Length(irp->input) != (Stream_GetPosition(irp->input) + inputBufferLength))
 	{
 		WLog_Print(smartcard->log, WLOG_WARN,
-				   "InputBufferLength mismatch: Actual: %d Expected: %d\n",
+				   "InputBufferLength mismatch: Actual: %d Expected: %d",
 				   Stream_Length(irp->input), Stream_GetPosition(irp->input) + inputBufferLength);
 		return SCARD_F_INTERNAL_ERROR;
 	}
 
 	WLog_Print(smartcard->log, WLOG_DEBUG, "%s (0x%08X) FileId: %d CompletionId: %d",
 			   smartcard_get_ioctl_string(ioControlCode, TRUE), ioControlCode, irp->FileId, irp->CompletionId);
 #if 0
-	WLog_DBG(TAG, "%s (0x%08X) FileId: %d CompletionId: %d\n",
+	WLog_DBG(TAG, "%s (0x%08X) FileId: %d CompletionId: %d",
 			 smartcard_get_ioctl_string(ioControlCode, TRUE), ioControlCode, irp->FileId, irp->CompletionId);
 #endif
 
@@ -1727,8 +1729,7 @@ UINT32 smartcard_irp_device_control_call(SMARTCARD_DEVICE* smartcard, SMARTCARD_
 			(ioControlCode != SCARD_IOCTL_RELEASESTARTEDEVENT))
 	{
 		offset = (RDPDR_DEVICE_IO_RESPONSE_LENGTH + RDPDR_DEVICE_IO_CONTROL_RSP_HDR_LENGTH);
-		smartcard_pack_write_size_align(smartcard, irp->output,
-										Stream_GetPosition(irp->output) - offset, 8);
+		smartcard_pack_write_size_align(smartcard, irp->output, Stream_GetPosition(irp->output) - offset, 8);
 	}
 
 	if ((result != SCARD_S_SUCCESS) && (result != SCARD_E_TIMEOUT) &&
@@ -1756,12 +1757,14 @@ UINT32 smartcard_irp_device_control_call(SMARTCARD_DEVICE* smartcard, SMARTCARD_
 	outputBufferLength = Stream_Length(irp->output) - RDPDR_DEVICE_IO_RESPONSE_LENGTH - 4;
 	objectBufferLength = outputBufferLength - RDPDR_DEVICE_IO_RESPONSE_LENGTH;
 	Stream_SetPosition(irp->output, RDPDR_DEVICE_IO_RESPONSE_LENGTH);
+
 	/* Device Control Response */
 	Stream_Write_UINT32(irp->output, outputBufferLength); /* OutputBufferLength (4 bytes) */
 	smartcard_pack_common_type_header(smartcard, irp->output); /* CommonTypeHeader (8 bytes) */
 	smartcard_pack_private_type_header(smartcard, irp->output, objectBufferLength); /* PrivateTypeHeader (8 bytes) */
 	Stream_Write_UINT32(irp->output, result); /* Result (4 bytes) */
 	Stream_SetPosition(irp->output, Stream_Length(irp->output));
+
 	return SCARD_S_SUCCESS;
 }
 

docs/valgrind.supp

@@ -12,6 +12,16 @@
    fun:getaddrinfo
 }
 
+{
+   ignore pcsc-lite SCardConnect
+   Memcheck:Param
+   socketcall.sendto(msg)
+   fun:send
+   fun:MessageSend
+   fun:MessageSendWithHeader
+   fun:SCardConnect
+}
+
 {
   ignore openssl malloc
   Memcheck:Leak