From aebe9742e078b61bf5fbeda148c7d8bb751b3b92 Mon Sep 17 00:00:00 2001 From: David Fort Date: Tue, 20 Feb 2024 23:59:33 +0100 Subject: [PATCH] [client,win32] Child session fixes It seems like WaitFor[Single|Multiple]Object calls aren't reliable on pipes, especially on the pipe opened for childSession access. The object can be marked as signaled even if no data is available, making the connection laggy and unresponsive (nearly unusable in some cases). This patch works around that by using ReadFileEx() with overlapped instead of simple ReadFile() and use asynchronous reads. --- client/Windows/wf_client.c | 6 +- client/common/cmdline.c | 5 +- libfreerdp/core/childsession.c | 245 +++++++++++++++++++++++++++++---- libfreerdp/core/info.c | 8 +- libfreerdp/crypto/tls.c | 1 + 5 files changed, 234 insertions(+), 31 deletions(-) diff --git a/client/Windows/wf_client.c b/client/Windows/wf_client.c index 8a86f1ba6..b23fa1f05 100644 --- a/client/Windows/wf_client.c +++ b/client/Windows/wf_client.c @@ -1022,7 +1022,7 @@ static DWORD WINAPI wf_client_thread(LPVOID lpParam) rdpSettings* settings = context->settings; WINPR_ASSERT(settings); - while (1) + while (!freerdp_shall_disconnect_context(instance->context)) { HANDLE handles[MAXIMUM_WAIT_OBJECTS] = { 0 }; DWORD nCount = 0; @@ -1045,7 +1045,9 @@ static DWORD WINAPI wf_client_thread(LPVOID lpParam) nCount += tmp; } - if (MsgWaitForMultipleObjects(nCount, handles, FALSE, 1000, QS_ALLINPUT) == WAIT_FAILED) + DWORD status = MsgWaitForMultipleObjectsEx(nCount, handles, 5 * 1000, QS_ALLINPUT, + MWMO_ALERTABLE | MWMO_INPUTAVAILABLE); + if (status == WAIT_FAILED) { WLog_ERR(TAG, "wfreerdp_run: WaitForMultipleObjects failed: 0x%08lX", GetLastError()); break; diff --git a/client/common/cmdline.c b/client/common/cmdline.c index 24e50e5ff..16d397c4b 100644 --- a/client/common/cmdline.c +++ b/client/common/cmdline.c @@ -4753,11 +4753,14 @@ static int freerdp_client_settings_parse_command_line_arguments_int( "vs-debug") || !freerdp_settings_set_string(settings, FreeRDP_ServerHostname, "localhost") || !freerdp_settings_set_string(settings, FreeRDP_AuthenticationPackageList, "ntlm") || + !freerdp_settings_set_string(settings, FreeRDP_ClientAddress, "0.0.0.0") || !freerdp_settings_set_bool(settings, FreeRDP_NegotiateSecurityLayer, FALSE) || !freerdp_settings_set_bool(settings, FreeRDP_VmConnectMode, TRUE) || !freerdp_settings_set_bool(settings, FreeRDP_ConnectChildSession, TRUE) || !freerdp_settings_set_bool(settings, FreeRDP_NlaSecurity, TRUE) || - !freerdp_settings_set_uint32(settings, FreeRDP_AuthenticationLevel, 0)) + !freerdp_settings_set_uint32(settings, FreeRDP_AuthenticationLevel, 0) || + !freerdp_settings_set_bool(settings, FreeRDP_NetworkAutoDetect, TRUE) || + !freerdp_settings_set_uint32(settings, FreeRDP_ConnectionType, CONNECTION_TYPE_LAN)) return COMMAND_LINE_ERROR_MEMORY; } #endif diff --git a/libfreerdp/core/childsession.c b/libfreerdp/core/childsession.c index 3bed26269..8bde8ec28 100644 --- a/libfreerdp/core/childsession.c +++ b/libfreerdp/core/childsession.c @@ -2,7 +2,7 @@ * FreeRDP: A Remote Desktop Protocol Implementation * Named pipe transport * - * Copyright 2023 David Fort + * Copyright 2023-2024 David Fort * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -18,15 +18,29 @@ */ #include "tcp.h" + #include #include +#include +#include + +#include + #include "childsession.h" #define TAG FREERDP_TAG("childsession") typedef struct { + OVERLAPPED readOverlapped; HANDLE hFile; + BOOL opInProgress; + BOOL lastOpClosed; + RingBuffer readBuffer; + BOOL blocking; + BYTE tmpReadBuffer[4096]; + + HANDLE readEvent; } WINPR_BIO_NAMED; static int transport_bio_named_uninit(BIO* bio); @@ -44,47 +58,189 @@ static int transport_bio_named_write(BIO* bio, const char* buf, int size) BIO_clear_flags(bio, BIO_FLAGS_WRITE); DWORD written = 0; + UINT64 start = GetTickCount64(); BOOL ret = WriteFile(ptr->hFile, buf, size, &written, NULL); - WLog_VRB(TAG, "transport_bio_named_write(%d)=%d written=%d", size, ret, written); + // winpr_HexDump(TAG, WLOG_DEBUG, buf, size); if (!ret) - return -1; + { + WLog_VRB(TAG, "error or deferred"); + return 0; + } + + WLog_VRB(TAG, "(%d)=%d written=%d duration=%d", size, ret, written, GetTickCount64() - start); if (written == 0) - return -1; + { + WLog_VRB(TAG, "closed on write"); + return 0; + } return written; } +static BOOL treatReadResult(WINPR_BIO_NAMED* ptr, DWORD readBytes) +{ + WLog_VRB(TAG, "treatReadResult(readBytes=%" PRIu32 ")", readBytes); + ptr->opInProgress = FALSE; + if (readBytes == 0) + { + WLog_VRB(TAG, "readBytes == 0"); + return TRUE; + } + + if (!ringbuffer_write(&ptr->readBuffer, ptr->tmpReadBuffer, readBytes)) + { + WLog_VRB(TAG, "ringbuffer_write()"); + return FALSE; + } + + return SetEvent(ptr->readEvent); +} + +static BOOL doReadOp(WINPR_BIO_NAMED* ptr) +{ + DWORD readBytes; + + if (!ResetEvent(ptr->readEvent)) + return FALSE; + + ptr->opInProgress = TRUE; + if (!ReadFile(ptr->hFile, ptr->tmpReadBuffer, sizeof(ptr->tmpReadBuffer), &readBytes, + &ptr->readOverlapped)) + { + DWORD error = GetLastError(); + switch (error) + { + case ERROR_NO_DATA: + WLog_VRB(TAG, "No Data, unexpected"); + return TRUE; + case ERROR_IO_PENDING: + WLog_VRB(TAG, "ERROR_IO_PENDING"); + return TRUE; + case ERROR_BROKEN_PIPE: + WLog_VRB(TAG, "broken pipe"); + ptr->lastOpClosed = TRUE; + return TRUE; + default: + return FALSE; + } + } + + return treatReadResult(ptr, readBytes); +} + static int transport_bio_named_read(BIO* bio, char* buf, int size) { WINPR_ASSERT(bio); WINPR_ASSERT(buf); WINPR_BIO_NAMED* ptr = (WINPR_BIO_NAMED*)BIO_get_data(bio); - if (!buf) return 0; - BIO_clear_flags(bio, BIO_FLAGS_READ); + BIO_clear_flags(bio, BIO_FLAGS_SHOULD_RETRY | BIO_FLAGS_READ); - DWORD readBytes = 0; - BOOL ret = ReadFile(ptr->hFile, buf, size, &readBytes, NULL); - WLog_VRB(TAG, "transport_bio_named_read(%d)=%d read=%d", size, ret, readBytes); - if (!ret) + if (ptr->blocking) { - if (GetLastError() == ERROR_NO_DATA) - BIO_set_flags(bio, (BIO_FLAGS_SHOULD_RETRY | BIO_FLAGS_READ)); - return -1; + while (!ringbuffer_used(&ptr->readBuffer)) + { + if (ptr->lastOpClosed) + return 0; + + if (ptr->opInProgress) + { + DWORD status = WaitForSingleObjectEx(ptr->readEvent, 500, TRUE); + switch (status) + { + case WAIT_TIMEOUT: + case WAIT_IO_COMPLETION: + continue; + case WAIT_OBJECT_0: + break; + default: + return -1; + } + + DWORD readBytes; + if (!GetOverlappedResult(ptr->hFile, &ptr->readOverlapped, &readBytes, FALSE)) + { + WLog_ERR(TAG, "GetOverlappedResult blocking(lastError=%" PRIu32 ")", + GetLastError()); + return -1; + } + + if (!treatReadResult(ptr, readBytes)) + { + WLog_ERR(TAG, "treatReadResult blocking"); + return -1; + } + } + } + } + else + { + if (ptr->opInProgress) + { + DWORD status = WaitForSingleObject(ptr->readEvent, 0); + switch (status) + { + case WAIT_OBJECT_0: + break; + default: + WLog_ERR(TAG, "error WaitForSingleObject(readEvent)=0x%" PRIu32 "", status); + return -1; + } + + DWORD readBytes; + if (!GetOverlappedResult(ptr->hFile, &ptr->readOverlapped, &readBytes, FALSE)) + { + WLog_ERR(TAG, "GetOverlappedResult non blocking(lastError=%" PRIu32 ")", + GetLastError()); + return -1; + } + + if (!treatReadResult(ptr, readBytes)) + { + WLog_ERR(TAG, "error treatReadResult non blocking"); + return -1; + } + } } - if (readBytes == 0) + int ret = MIN(size, ringbuffer_used(&ptr->readBuffer)); + if (ret) { - BIO_set_flags(bio, (BIO_FLAGS_READ | BIO_FLAGS_SHOULD_RETRY)); - return 0; + DataChunk chunks[2]; + int nchunks = ringbuffer_peek(&ptr->readBuffer, chunks, ret); + for (int i = 0; i < nchunks; i++) + { + memcpy(buf, chunks[i].data, chunks[i].size); + buf += chunks[i].size; + } + + ringbuffer_commit_read_bytes(&ptr->readBuffer, ret); + + WLog_VRB(TAG, "(%d)=%d nchunks=%d", size, ret, nchunks); + } + else + { + ret = -1; } - return readBytes; + if (!ringbuffer_used(&ptr->readBuffer)) + { + if (!ptr->opInProgress && !doReadOp(ptr)) + { + WLog_ERR(TAG, "error rearming read"); + return -1; + } + } + + if (ret <= 0) + BIO_set_flags(bio, (BIO_FLAGS_SHOULD_RETRY | BIO_FLAGS_READ)); + + return ret; } static int transport_bio_named_puts(BIO* bio, const char* str) @@ -100,7 +256,7 @@ static int transport_bio_named_gets(BIO* bio, char* str, int size) WINPR_ASSERT(bio); WINPR_ASSERT(str); - return transport_bio_named_write(bio, str, size); + return transport_bio_named_read(bio, str, size); } static long transport_bio_named_ctrl(BIO* bio, int cmd, long arg1, void* arg2) @@ -119,7 +275,7 @@ static long transport_bio_named_ctrl(BIO* bio, int cmd, long arg1, void* arg2) if (!BIO_get_init(bio) || !arg2) return 0; - *((HANDLE*)arg2) = ptr->hFile; + *((HANDLE*)arg2) = ptr->readEvent; return 1; case BIO_C_SET_HANDLE: BIO_set_init(bio, 1); @@ -127,18 +283,25 @@ static long transport_bio_named_ctrl(BIO* bio, int cmd, long arg1, void* arg2) return 0; ptr->hFile = (HANDLE)arg2; + ptr->blocking = TRUE; + if (!doReadOp(ptr)) + return -1; return 1; case BIO_C_SET_NONBLOCK: { + WLog_DBG(TAG, "BIO_C_SET_NONBLOCK"); + ptr->blocking = FALSE; return 1; } case BIO_C_WAIT_READ: { + WLog_DBG(TAG, "BIO_C_WAIT_READ"); return 1; } case BIO_C_WAIT_WRITE: { + WLog_DBG(TAG, "BIO_C_WAIT_WRITE"); return 1; } @@ -173,16 +336,33 @@ static long transport_bio_named_ctrl(BIO* bio, int cmd, long arg1, void* arg2) return status; } +static void BIO_NAMED_free(WINPR_BIO_NAMED* ptr) +{ + if (!ptr) + return; + + if (ptr->hFile) + { + CloseHandle(ptr->hFile); + ptr->hFile = NULL; + } + + if (ptr->readEvent) + { + CloseHandle(ptr->readEvent); + ptr->readEvent = NULL; + } + + ringbuffer_destroy(&ptr->readBuffer); + free(ptr); +} + static int transport_bio_named_uninit(BIO* bio) { WINPR_ASSERT(bio); WINPR_BIO_NAMED* ptr = (WINPR_BIO_NAMED*)BIO_get_data(bio); - if (ptr && ptr->hFile) - { - CloseHandle(ptr->hFile); - ptr->hFile = NULL; - } + BIO_NAMED_free(ptr); BIO_set_init(bio, 0); BIO_set_flags(bio, 0); @@ -192,15 +372,27 @@ static int transport_bio_named_uninit(BIO* bio) static int transport_bio_named_new(BIO* bio) { WINPR_ASSERT(bio); - BIO_set_flags(bio, BIO_FLAGS_SHOULD_RETRY); WINPR_BIO_NAMED* ptr = (WINPR_BIO_NAMED*)calloc(1, sizeof(WINPR_BIO_NAMED)); if (!ptr) return 0; + if (!ringbuffer_init(&ptr->readBuffer, 0xfffff)) + goto error; + + ptr->readEvent = CreateEventA(NULL, TRUE, FALSE, NULL); + if (!ptr->readEvent || ptr->readEvent == INVALID_HANDLE_VALUE) + goto error; + + ptr->readOverlapped.hEvent = ptr->readEvent; + BIO_set_data(bio, ptr); BIO_set_flags(bio, BIO_FLAGS_SHOULD_RETRY); return 1; + +error: + BIO_NAMED_free(ptr); + return 0; } static int transport_bio_named_free(BIO* bio) @@ -295,7 +487,8 @@ static BOOL createChildSessionTransport(HANDLE* pFile) ConvertWCharNToUtf8(pipePath, 0x80, pipePathA, sizeof(pipePathA)); WLog_DBG(TAG, "child session is at '%s'", pipePathA); - HANDLE f = CreateFileW(pipePath, GENERIC_READ | GENERIC_WRITE, 0, NULL, OPEN_EXISTING, 0, NULL); + HANDLE f = CreateFileW(pipePath, GENERIC_READ | GENERIC_WRITE, 0, NULL, OPEN_EXISTING, + FILE_FLAG_OVERLAPPED, NULL); if (f == INVALID_HANDLE_VALUE) { WLog_ERR(TAG, "error when connecting to local named pipe"); diff --git a/libfreerdp/core/info.c b/libfreerdp/core/info.c index 0d8e90ee2..7d6eec137 100644 --- a/libfreerdp/core/info.c +++ b/libfreerdp/core/info.c @@ -469,8 +469,12 @@ static BOOL rdp_write_extended_info_packet(rdpRdp* rdp, wStream* s) rdpSettings* settings = rdp->settings; WINPR_ASSERT(settings); - const UINT16 clientAddressFamily = - settings->IPv6Enabled ? ADDRESS_FAMILY_INET6 : ADDRESS_FAMILY_INET; + UINT16 clientAddressFamily = ADDRESS_FAMILY_INET; + if (settings->ConnectChildSession) + clientAddressFamily = 0x0000; + else if (settings->IPv6Enabled) + clientAddressFamily = ADDRESS_FAMILY_INET6; + WCHAR* clientAddress = ConvertUtf8ToWCharAlloc(settings->ClientAddress, &cbClientAddress); if (cbClientAddress > (UINT16_MAX / sizeof(WCHAR))) diff --git a/libfreerdp/crypto/tls.c b/libfreerdp/crypto/tls.c index 24c84c908..2d400389b 100644 --- a/libfreerdp/crypto/tls.c +++ b/libfreerdp/crypto/tls.c @@ -985,6 +985,7 @@ static int pollAndHandshake(rdpTls* tls) case WAIT_OBJECT_0: break; case WAIT_TIMEOUT: + case WAIT_IO_COMPLETION: continue; default: WLog_ERR(TAG, "error during WaitForSingleObject(): 0x%08" PRIX32 "", status);