From 2aa71ea7e405fa6f582560aed73519de2bcd4b36 Mon Sep 17 00:00:00 2001 From: David Fort Date: Mon, 6 Nov 2017 21:58:10 +0100 Subject: [PATCH 1/3] Added some checks for the serial redirection channel Sponsored by: Rangee GmbH (http://www.rangee.de) --- channels/serial/client/serial_main.c | 63 +++++++++++++++++++--------- 1 file changed, 44 insertions(+), 19 deletions(-) diff --git a/channels/serial/client/serial_main.c b/channels/serial/client/serial_main.c index cfbfe1565..05c61ddf7 100644 --- a/channels/serial/client/serial_main.c +++ b/channels/serial/client/serial_main.c @@ -124,20 +124,27 @@ static UINT32 _GetLastErrorToIoStatus(SERIAL_DEVICE* serial) return STATUS_UNSUCCESSFUL; } -static void serial_process_irp_create(SERIAL_DEVICE* serial, IRP* irp) +static UINT serial_process_irp_create(SERIAL_DEVICE* serial, IRP* irp) { DWORD DesiredAccess; DWORD SharedAccess; DWORD CreateDisposition; UINT32 PathLength; + + if (Stream_GetRemainingLength(irp->input) < 32) + return ERROR_INVALID_DATA; + Stream_Read_UINT32(irp->input, DesiredAccess); /* DesiredAccess (4 bytes) */ Stream_Seek_UINT64(irp->input); /* AllocationSize (8 bytes) */ Stream_Seek_UINT32(irp->input); /* FileAttributes (4 bytes) */ Stream_Read_UINT32(irp->input, SharedAccess); /* SharedAccess (4 bytes) */ - Stream_Read_UINT32(irp->input, - CreateDisposition); /* CreateDisposition (4 bytes) */ + Stream_Read_UINT32(irp->input, CreateDisposition); /* CreateDisposition (4 bytes) */ Stream_Seek_UINT32(irp->input); /* CreateOptions (4 bytes) */ Stream_Read_UINT32(irp->input, PathLength); /* PathLength (4 bytes) */ + + if (Stream_GetRemainingLength(irp->input) < PathLength) + return ERROR_INVALID_DATA; + Stream_Seek(irp->input, PathLength); /* Path (variable) */ assert(PathLength == 0); /* MS-RDPESP 2.2.2.2 */ #ifndef _WIN32 @@ -191,18 +198,21 @@ static void serial_process_irp_create(SERIAL_DEVICE* serial, IRP* irp) /* dcb.fBinary = TRUE; */ /* SetCommState(serial->hComm, &dcb); */ assert(irp->FileId == 0); - irp->FileId = - irp->devman->id_sequence++; /* FIXME: why not ((WINPR_COMM*)hComm)->fd? */ + irp->FileId = irp->devman->id_sequence++; /* FIXME: why not ((WINPR_COMM*)hComm)->fd? */ irp->IoStatus = STATUS_SUCCESS; WLog_Print(serial->log, WLOG_DEBUG, "%s (DeviceId: %"PRIu32", FileId: %"PRIu32") created.", serial->device.name, irp->device->id, irp->FileId); error_handle: Stream_Write_UINT32(irp->output, irp->FileId); /* FileId (4 bytes) */ Stream_Write_UINT8(irp->output, 0); /* Information (1 byte) */ + return CHANNEL_RC_OK; } -static void serial_process_irp_close(SERIAL_DEVICE* serial, IRP* irp) +static UINT serial_process_irp_close(SERIAL_DEVICE* serial, IRP* irp) { + if (Stream_GetRemainingLength(irp->input) < 32) + return ERROR_INVALID_DATA; + Stream_Seek(irp->input, 32); /* Padding (32 bytes) */ if (!CloseHandle(serial->hComm)) @@ -219,6 +229,7 @@ static void serial_process_irp_close(SERIAL_DEVICE* serial, IRP* irp) irp->IoStatus = STATUS_SUCCESS; error_handle: Stream_Zero(irp->output, 5); /* Padding (5 bytes) */ + return CHANNEL_RC_OK; } /** @@ -232,11 +243,15 @@ static UINT serial_process_irp_read(SERIAL_DEVICE* serial, IRP* irp) UINT64 Offset; BYTE* buffer = NULL; DWORD nbRead = 0; + + if (Stream_GetRemainingLength(irp->input) < 32) + return ERROR_INVALID_DATA; + Stream_Read_UINT32(irp->input, Length); /* Length (4 bytes) */ Stream_Read_UINT64(irp->input, Offset); /* Offset (8 bytes) */ Stream_Seek(irp->input, 20); /* Padding (20 bytes) */ - buffer = (BYTE*)calloc(Length, sizeof(BYTE)); + buffer = (BYTE*)calloc(Length, sizeof(BYTE)); if (buffer == NULL) { irp->IoStatus = STATUS_NO_MEMORY; @@ -283,11 +298,15 @@ error_handle: return CHANNEL_RC_OK; } -static void serial_process_irp_write(SERIAL_DEVICE* serial, IRP* irp) +static UINT serial_process_irp_write(SERIAL_DEVICE* serial, IRP* irp) { UINT32 Length; UINT64 Offset; DWORD nbWritten = 0; + + if (Stream_GetRemainingLength(irp->input) < 32) + return ERROR_INVALID_DATA; + Stream_Read_UINT32(irp->input, Length); /* Length (4 bytes) */ Stream_Read_UINT64(irp->input, Offset); /* Offset (8 bytes) */ Stream_Seek(irp->input, 20); /* Padding (20 bytes) */ @@ -318,6 +337,8 @@ static void serial_process_irp_write(SERIAL_DEVICE* serial, IRP* irp) serial->device.name); Stream_Write_UINT32(irp->output, nbWritten); /* Length (4 bytes) */ Stream_Write_UINT8(irp->output, 0); /* Padding (1 byte) */ + + return CHANNEL_RC_OK; } @@ -334,14 +355,20 @@ static UINT serial_process_irp_device_control(SERIAL_DEVICE* serial, IRP* irp) UINT32 OutputBufferLength; BYTE* OutputBuffer = NULL; DWORD BytesReturned = 0; - Stream_Read_UINT32(irp->input, - OutputBufferLength); /* OutputBufferLength (4 bytes) */ - Stream_Read_UINT32(irp->input, - InputBufferLength); /* InputBufferLength (4 bytes) */ + + if (Stream_GetRemainingLength(irp->input) < 32) + return ERROR_INVALID_DATA; + + Stream_Read_UINT32(irp->input, OutputBufferLength); /* OutputBufferLength (4 bytes) */ + Stream_Read_UINT32(irp->input, InputBufferLength); /* InputBufferLength (4 bytes) */ + Stream_Read_UINT32(irp->input, IoControlCode); /* IoControlCode (4 bytes) */ Stream_Seek(irp->input, 20); /* Padding (20 bytes) */ - OutputBuffer = (BYTE*)calloc(OutputBufferLength, sizeof(BYTE)); + if (Stream_GetRemainingLength(irp->input) < InputBufferLength) + return ERROR_INVALID_DATA; + + OutputBuffer = (BYTE*)calloc(OutputBufferLength, sizeof(BYTE)); if (OutputBuffer == NULL) { irp->IoStatus = STATUS_NO_MEMORY; @@ -349,7 +376,6 @@ static UINT serial_process_irp_device_control(SERIAL_DEVICE* serial, IRP* irp) } InputBuffer = (BYTE*)calloc(InputBufferLength, sizeof(BYTE)); - if (InputBuffer == NULL) { irp->IoStatus = STATUS_NO_MEMORY; @@ -381,8 +407,7 @@ error_handle: * BytesReturned == OutputBufferLength when * CommDeviceIoControl returns FALSE */ assert(OutputBufferLength == BytesReturned); - Stream_Write_UINT32(irp->output, - BytesReturned); /* OutputBufferLength (4 bytes) */ + Stream_Write_UINT32(irp->output, BytesReturned); /* OutputBufferLength (4 bytes) */ if (BytesReturned > 0) { @@ -425,11 +450,11 @@ static UINT serial_process_irp(SERIAL_DEVICE* serial, IRP* irp) switch (irp->MajorFunction) { case IRP_MJ_CREATE: - serial_process_irp_create(serial, irp); + error = serial_process_irp_create(serial, irp); break; case IRP_MJ_CLOSE: - serial_process_irp_close(serial, irp); + error = serial_process_irp_close(serial, irp); break; case IRP_MJ_READ: @@ -439,7 +464,7 @@ static UINT serial_process_irp(SERIAL_DEVICE* serial, IRP* irp) break; case IRP_MJ_WRITE: - serial_process_irp_write(serial, irp); + error = serial_process_irp_write(serial, irp); break; case IRP_MJ_DEVICE_CONTROL: From ff8f7e94749fc8cdd03c4b9d64bfe9deae4fd170 Mon Sep 17 00:00:00 2001 From: David Fort Date: Mon, 6 Nov 2017 22:09:01 +0100 Subject: [PATCH 2/3] Disambiguate USB error messages and fix a typo --- channels/urbdrc/client/libusb/libusb_udevice.c | 2 +- channels/urbdrc/client/urbdrc_main.c | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/channels/urbdrc/client/libusb/libusb_udevice.c b/channels/urbdrc/client/libusb/libusb_udevice.c index 62ddb786a..55bd5431a 100644 --- a/channels/urbdrc/client/libusb/libusb_udevice.c +++ b/channels/urbdrc/client/libusb/libusb_udevice.c @@ -1797,7 +1797,7 @@ static IUDEVICE* udev_init(UDEVICE* pdev, UINT16 bus_number, UINT16 dev_number) /* get the first interface and first altsetting */ interface_temp = config_temp->interface[0].altsetting[0]; - WLog_DBG(TAG,"Regist Device: Vid: 0x%04"PRIX16" Pid: 0x%04"PRIX16"" + WLog_DBG(TAG,"Registered Device: Vid: 0x%04"PRIX16" Pid: 0x%04"PRIX16"" " InterfaceClass = 0x%02"PRIX8"", pdev->devDescriptor->idVendor, pdev->devDescriptor->idProduct, diff --git a/channels/urbdrc/client/urbdrc_main.c b/channels/urbdrc/client/urbdrc_main.c index 5410488b0..23f7f2ce1 100644 --- a/channels/urbdrc/client/urbdrc_main.c +++ b/channels/urbdrc/client/urbdrc_main.c @@ -471,7 +471,7 @@ static UINT urbdrc_exchange_capabilities(URBDRC_CHANNEL_CALLBACK* callback, char break; default: - WLog_ERR(TAG, "unknown FunctionId 0x%"PRIX32"", FunctionId); + WLog_ERR(TAG, "%s: unknown FunctionId 0x%"PRIX32"", __FUNCTION__, FunctionId); error = ERROR_NOT_FOUND; break; } @@ -1184,7 +1184,7 @@ static UINT urbdrc_process_channel_notification(URBDRC_CHANNEL_CALLBACK* callbac break; default: - WLog_VRB(TAG, "unknown FunctionId 0x%"PRIX32"", FunctionId); + WLog_VRB(TAG, "%s: unknown FunctionId 0x%"PRIX32"", __FUNCTION__, FunctionId); error = 1; break; } From de7d7e43c9ce7543aa6bff4958dfc2602359fa17 Mon Sep 17 00:00:00 2001 From: David Fort Date: Mon, 6 Nov 2017 22:23:07 +0100 Subject: [PATCH 3/3] serial redirection: implement event char The signotec signature device requires the eventChar support to work properly in serial redirection mode. This implementation is basic but does the job for this device. Sponsored by: Rangee GmbH (http://www.rangee.de) --- winpr/libwinpr/comm/comm.c | 6 ++---- winpr/libwinpr/comm/comm.h | 1 + winpr/libwinpr/comm/comm_io.c | 14 ++++++++++++-- winpr/libwinpr/comm/comm_serial_sys.c | 14 ++++++-------- 4 files changed, 21 insertions(+), 14 deletions(-) diff --git a/winpr/libwinpr/comm/comm.c b/winpr/libwinpr/comm/comm.c index 126cc9f2d..746ce7b0a 100644 --- a/winpr/libwinpr/comm/comm.c +++ b/winpr/libwinpr/comm/comm.c @@ -425,10 +425,8 @@ BOOL GetCommState(HANDLE hFile, LPDCB lpDCB) lpLocalDcb->fDtrControl = DTR_CONTROL_DISABLE; } - lpLocalDcb->fDsrSensitivity = (handflow.ControlHandShake & - SERIAL_DSR_SENSITIVITY) != 0; - lpLocalDcb->fTXContinueOnXoff = (handflow.FlowReplace & SERIAL_XOFF_CONTINUE) != - 0; + lpLocalDcb->fDsrSensitivity = (handflow.ControlHandShake & SERIAL_DSR_SENSITIVITY) != 0; + lpLocalDcb->fTXContinueOnXoff = (handflow.FlowReplace & SERIAL_XOFF_CONTINUE) != 0; lpLocalDcb->fOutX = (handflow.FlowReplace & SERIAL_AUTO_TRANSMIT) != 0; lpLocalDcb->fInX = (handflow.FlowReplace & SERIAL_AUTO_RECEIVE) != 0; lpLocalDcb->fErrorChar = (handflow.FlowReplace & SERIAL_ERROR_CHAR) != 0; diff --git a/winpr/libwinpr/comm/comm.h b/winpr/libwinpr/comm/comm.h index 8bc5fddba..8dbceaea3 100644 --- a/winpr/libwinpr/comm/comm.h +++ b/winpr/libwinpr/comm/comm.h @@ -69,6 +69,7 @@ struct winpr_comm ULONG WaitEventMask; ULONG PendingEvents; + char eventChar; /* NB: CloseHandle() has to free resources */ }; diff --git a/winpr/libwinpr/comm/comm_io.c b/winpr/libwinpr/comm/comm_io.c index d3eb2c147..e401ea27e 100644 --- a/winpr/libwinpr/comm/comm_io.c +++ b/winpr/libwinpr/comm/comm_io.c @@ -146,7 +146,7 @@ BOOL CommReadFile(HANDLE hDevice, LPVOID lpBuffer, DWORD nNumberOfBytesToRead, * * ReadIntervalTimeout | ReadTotalTimeoutMultiplier | ReadTotalTimeoutConstant | VMIN | VTIME | TMAX | * 0 | 0 | 0 | N | 0 | INDEF | Blocks for N bytes available. - * 0< Ti EventsLock); + if (pComm->PendingEvents & SERIAL_EV_FREERDP_WAITING) + { + if (pComm->eventChar != '\0' && memchr(lpBuffer, pComm->eventChar, nbRead)) + pComm->PendingEvents |= SERIAL_EV_RXCHAR; + } + LeaveCriticalSection(&pComm->EventsLock); goto return_true; } @@ -546,13 +554,15 @@ BOOL CommWriteFile(HANDLE hDevice, LPCVOID lpBuffer, * printer. Its driver was expecting the modem line status * SERIAL_MSR_DSR true after the sending which was never * happenning otherwise. A purge was also done before each - * Write operation. The serial port was oppened with: + * Write operation. The serial port was opened with: * DesiredAccess=0x0012019F. The printer worked fine with * mstsc. */ tcdrain(pComm->fd_write); + return_true: LeaveCriticalSection(&pComm->WriteLock); return TRUE; + return_false: LeaveCriticalSection(&pComm->WriteLock); return FALSE; diff --git a/winpr/libwinpr/comm/comm_serial_sys.c b/winpr/libwinpr/comm/comm_serial_sys.c index c6b3ad7ec..1ba0abdc6 100644 --- a/winpr/libwinpr/comm/comm_serial_sys.c +++ b/winpr/libwinpr/comm/comm_serial_sys.c @@ -316,7 +316,7 @@ static BOOL _set_serial_chars(WINPR_COMM *pComm, const SERIAL_CHARS *pSerialChar if (pSerialChars->XonChar == pSerialChars->XoffChar) { - /* http://msdn.microsoft.com/en-us/library/windows/hardware/ff546688%28v=vs.85%29.aspx */ + /* https://msdn.microsoft.com/en-us/library/windows/hardware/ff546688?v=vs.85.aspx */ SetLastError(ERROR_INVALID_PARAMETER); return FALSE; } @@ -360,12 +360,9 @@ static BOOL _set_serial_chars(WINPR_COMM *pComm, const SERIAL_CHARS *pSerialChar result = FALSE; /* but keep on */ } - /* FIXME: could be implemented during read/write I/O. What about ISIG? */ if (pSerialChars->EventChar != '\0') { - CommLog_Print(WLOG_WARN, "EventChar 0x%02"PRIX8" ('%c') cannot be set\n", pSerialChars->EventChar, (char) pSerialChars->EventChar); - SetLastError(ERROR_NOT_SUPPORTED); - result = FALSE; /* but keep on */ + pComm->eventChar = pSerialChars->EventChar; } upcomingTermios.c_cc[VSTART] = pSerialChars->XonChar; @@ -1076,7 +1073,8 @@ static BOOL _set_wait_mask(WINPR_COMM *pComm, const ULONG *pWaitMask) if (possibleMask != *pWaitMask) { - CommLog_Print(WLOG_WARN, "Not all wait events supported (Serial.sys), requested events= 0x%08"PRIX32", possible events= 0x%08"PRIX32"", *pWaitMask, possibleMask); + CommLog_Print(WLOG_WARN, "Not all wait events supported (Serial.sys), requested events= 0x%08"PRIX32", possible events= 0x%08"PRIX32"", + *pWaitMask, possibleMask); /* FIXME: shall we really set the possibleMask and return FALSE? */ pComm->WaitEventMask = possibleMask; @@ -1310,7 +1308,7 @@ static BOOL _get_commstatus(WINPR_COMM *pComm, SERIAL_STATUS *pCommstatus) if (currentCounters.rx != pComm->counters.rx) { - pComm->PendingEvents |= SERIAL_EV_RXCHAR; + pComm->PendingEvents |= SERIAL_EV_RXFLAG; } if ((currentCounters.tx != pComm->counters.tx) && /* at least a transmission occurred AND ...*/ @@ -1458,7 +1456,7 @@ static BOOL _wait_on_mask(WINPR_COMM *pComm, ULONG *pOutputMask) * * NOTE: previously used a semaphore but used * sem_timedwait() anyway. Finally preferred a simpler - * solution with Sleep() whithout the burden of the + * solution with Sleep() without the burden of the * semaphore initialization and destroying. */