From 892cbe32610a965e7e0cdadc02c01e8b34b38c96 Mon Sep 17 00:00:00 2001 From: Ondrej Holy Date: Tue, 27 Apr 2021 10:04:47 +0200 Subject: [PATCH] Fix various memory leaks reported by Coverity Covscan report contains various memory leak defects which were marked as important. I have spent some time analyzing them and although they were marked as important, most of them are in error cases, so probably nothing serious. Let's fix most of them anyway. The rest are false positives, or too complicated to fix, or already fixed in master, or simply I am unsure about them. Relates: https://github.com/FreeRDP/FreeRDP/issues/6981 --- channels/printer/client/printer_main.c | 3 +++ channels/rdp2tcp/client/rdp2tcp_main.c | 3 +++ channels/smartcard/client/smartcard_main.c | 1 + channels/smartcard/client/smartcard_operations.c | 3 +++ channels/smartcard/client/smartcard_pack.c | 4 +++- channels/urbdrc/client/data_transfer.c | 3 +++ channels/urbdrc/common/msusb.c | 2 +- channels/urbdrc/common/msusb.h | 1 + client/Wayland/wlf_cliprdr.c | 3 +++ client/X11/xf_window.c | 3 +++ libfreerdp/common/assistance.c | 2 +- libfreerdp/core/gateway/ncacn_http.c | 2 ++ libfreerdp/core/gateway/rdg.c | 6 ++++++ libfreerdp/core/gateway/rpc_client.c | 2 +- libfreerdp/core/info.c | 2 +- rdtk/librdtk/rdtk_nine_patch.c | 5 +++++ winpr/tools/makecert/makecert.c | 9 ++++++++- 17 files changed, 48 insertions(+), 6 deletions(-) diff --git a/channels/printer/client/printer_main.c b/channels/printer/client/printer_main.c index 744f9d74c..a6af9d8b2 100644 --- a/channels/printer/client/printer_main.c +++ b/channels/printer/client/printer_main.c @@ -109,7 +109,10 @@ static BOOL printer_write_setting(const char* path, prn_conf_t type, const void* char* abs = GetCombinedPath(path, name); if (!abs || (length > INT32_MAX)) + { + free(abs); return FALSE; + } file = CreateFileA(abs, GENERIC_WRITE, 0, NULL, CREATE_ALWAYS, FILE_ATTRIBUTE_NORMAL, NULL); free(abs); diff --git a/channels/rdp2tcp/client/rdp2tcp_main.c b/channels/rdp2tcp/client/rdp2tcp_main.c index 5e750bb7a..27b0b02d4 100644 --- a/channels/rdp2tcp/client/rdp2tcp_main.c +++ b/channels/rdp2tcp/client/rdp2tcp_main.c @@ -327,7 +327,10 @@ BOOL VCAPITYPE VirtualChannelEntryEx(PCHANNEL_ENTRY_POINTS pEntryPoints, PVOID p plugin->channelEntryPoints = *pEntryPointsEx; if (init_external_addin(plugin) < 0) + { + free(plugin); return FALSE; + } strncpy(channelDef.name, RDP2TCP_CHAN_NAME, sizeof(channelDef.name)); channelDef.options = diff --git a/channels/smartcard/client/smartcard_main.c b/channels/smartcard/client/smartcard_main.c index 2df4c14e3..c9eb2bfa6 100644 --- a/channels/smartcard/client/smartcard_main.c +++ b/channels/smartcard/client/smartcard_main.c @@ -477,6 +477,7 @@ static UINT smartcard_process_irp(SMARTCARD_DEVICE* smartcard, IRP* irp) { if ((status = smartcard_irp_device_control_call(smartcard, operation))) { + free(operation); WLog_ERR(TAG, "smartcard_irp_device_control_call failed with error %" PRId32 "!", status); return (UINT32)status; diff --git a/channels/smartcard/client/smartcard_operations.c b/channels/smartcard/client/smartcard_operations.c index 4639b1778..bf368c994 100644 --- a/channels/smartcard/client/smartcard_operations.c +++ b/channels/smartcard/client/smartcard_operations.c @@ -516,7 +516,10 @@ static DWORD filter_device_by_name_w(wLinkedList* list, LPWSTR* mszReaders, DWOR /* When res==0, readers may have been set to NULL by ConvertFromUnicode */ if ((res < 0) || ((DWORD)res != cchReaders) || (readers == 0)) + { + free(readers); return 0; + } free(*mszReaders); *mszReaders = NULL; diff --git a/channels/smartcard/client/smartcard_pack.c b/channels/smartcard/client/smartcard_pack.c index 68c983f85..ebd84ed8f 100644 --- a/channels/smartcard/client/smartcard_pack.c +++ b/channels/smartcard/client/smartcard_pack.c @@ -406,7 +406,9 @@ static char* smartcard_msz_dump_w(const WCHAR* msz, size_t len, char* buffer, si { char* sz = NULL; ConvertFromUnicode(CP_UTF8, 0, msz, (int)len, &sz, 0, NULL, NULL); - return smartcard_msz_dump_a(sz, len, buffer, bufferLen); + smartcard_msz_dump_a(sz, len, buffer, bufferLen); + free(sz); + return buffer; } static char* smartcard_array_dump(const void* pd, size_t len, char* buffer, size_t bufferLen) diff --git a/channels/urbdrc/client/data_transfer.c b/channels/urbdrc/client/data_transfer.c index f33b0f3b0..7f65f601b 100644 --- a/channels/urbdrc/client/data_transfer.c +++ b/channels/urbdrc/client/data_transfer.c @@ -552,7 +552,10 @@ static UINT urb_select_interface(IUDEVICE* pdev, URBDRC_CHANNEL_CALLBACK* callba MsInterface = msusb_msinterface_read(s); if ((Stream_GetRemainingLength(s) < 4) || !MsInterface) + { + msusb_msinterface_free(MsInterface); return ERROR_INVALID_DATA; + } Stream_Read_UINT32(s, OutputBufferSize); pdev->select_interface(pdev, MsInterface->InterfaceNumber, MsInterface->AlternateSetting); diff --git a/channels/urbdrc/common/msusb.c b/channels/urbdrc/common/msusb.c index bb517ce5d..12fceeefc 100644 --- a/channels/urbdrc/common/msusb.c +++ b/channels/urbdrc/common/msusb.c @@ -108,7 +108,7 @@ static MSUSB_INTERFACE_DESCRIPTOR* msusb_msinterface_new() return (MSUSB_INTERFACE_DESCRIPTOR*)calloc(1, sizeof(MSUSB_INTERFACE_DESCRIPTOR)); } -static void msusb_msinterface_free(MSUSB_INTERFACE_DESCRIPTOR* MsInterface) +void msusb_msinterface_free(MSUSB_INTERFACE_DESCRIPTOR* MsInterface) { if (MsInterface) { diff --git a/channels/urbdrc/common/msusb.h b/channels/urbdrc/common/msusb.h index 89f1a2bde..ff17ed1b1 100644 --- a/channels/urbdrc/common/msusb.h +++ b/channels/urbdrc/common/msusb.h @@ -82,6 +82,7 @@ extern "C" MSUSB_INTERFACE_DESCRIPTOR* NewMsInterface); FREERDP_API MSUSB_INTERFACE_DESCRIPTOR* msusb_msinterface_read(wStream* out); FREERDP_API BOOL msusb_msinterface_write(MSUSB_INTERFACE_DESCRIPTOR* MsInterface, wStream* out); + FREERDP_API void msusb_msinterface_free(MSUSB_INTERFACE_DESCRIPTOR* MsInterface); /* MSUSB_CONFIG exported functions */ FREERDP_API MSUSB_CONFIG_DESCRIPTOR* msusb_msconfig_new(void); diff --git a/client/Wayland/wlf_cliprdr.c b/client/Wayland/wlf_cliprdr.c index 0762cfa3d..bbb5e8e59 100644 --- a/client/Wayland/wlf_cliprdr.c +++ b/client/Wayland/wlf_cliprdr.c @@ -649,7 +649,10 @@ wlf_cliprdr_server_format_data_request(CliprdrClientContext* context, } if (rc != CHANNEL_RC_OK) + { + free(data); return rc; + } rc = wlf_cliprdr_send_data_response(clipboard, data, size); free(data); diff --git a/client/X11/xf_window.c b/client/X11/xf_window.c index 55ea71232..f6871f966 100644 --- a/client/X11/xf_window.c +++ b/client/X11/xf_window.c @@ -1119,7 +1119,10 @@ xfAppWindow* xf_AppWindowFromX11Window(xfContext* xfc, Window wnd) appWindow = xf_rail_get_window(xfc, *(UINT64*)pKeys[index]); if (!appWindow) + { + free(pKeys); return NULL; + } if (appWindow->handle == wnd) { diff --git a/libfreerdp/common/assistance.c b/libfreerdp/common/assistance.c index 71f18e617..2dcd0e36d 100644 --- a/libfreerdp/common/assistance.c +++ b/libfreerdp/common/assistance.c @@ -400,7 +400,7 @@ static BOOL freerdp_assistance_parse_connection_string2(rdpAssistanceFile* file) { WLog_ERR(TAG, "Failed to parse ASSISTANCE file: ConnectionString2 invalid field " "order for ID"); - return -1; + goto out_fail; } length = q - p; free(file->RASessionId); diff --git a/libfreerdp/core/gateway/ncacn_http.c b/libfreerdp/core/gateway/ncacn_http.c index cffb378eb..75da83d62 100644 --- a/libfreerdp/core/gateway/ncacn_http.c +++ b/libfreerdp/core/gateway/ncacn_http.c @@ -121,6 +121,7 @@ BOOL rpc_ncacn_http_recv_in_channel_response(RpcChannel* inChannel, HttpResponse if (ntlmTokenData && ntlmTokenLength) return ntlm_client_set_input_buffer(ntlm, FALSE, ntlmTokenData, ntlmTokenLength); + free(ntlmTokenData); return TRUE; } @@ -274,5 +275,6 @@ BOOL rpc_ncacn_http_recv_out_channel_response(RpcChannel* outChannel, HttpRespon if (ntlmTokenData && ntlmTokenLength) return ntlm_client_set_input_buffer(ntlm, FALSE, ntlmTokenData, ntlmTokenLength); + free(ntlmTokenData); return TRUE; } diff --git a/libfreerdp/core/gateway/rdg.c b/libfreerdp/core/gateway/rdg.c index 7a9d5023d..01c863eb7 100644 --- a/libfreerdp/core/gateway/rdg.c +++ b/libfreerdp/core/gateway/rdg.c @@ -350,7 +350,10 @@ static BOOL rdg_write_chunked(BIO* bio, wStream* sPacket) len = Stream_Length(sChunk); if (len > INT_MAX) + { + Stream_Free(sChunk, TRUE); return FALSE; + } status = BIO_write(bio, Stream_Buffer(sChunk), (int)len); Stream_Free(sChunk, TRUE); @@ -2060,7 +2063,10 @@ static int rdg_write_chunked_data_packet(rdpRdg* rdg, const BYTE* buf, int isize len = Stream_Length(sChunk); if (len > INT_MAX) + { + Stream_Free(sChunk, TRUE); return -1; + } status = tls_write_all(rdg->tlsIn, Stream_Buffer(sChunk), (int)len); Stream_Free(sChunk, TRUE); diff --git a/libfreerdp/core/gateway/rpc_client.c b/libfreerdp/core/gateway/rpc_client.c index 845bbe4b8..7fb990bd7 100644 --- a/libfreerdp/core/gateway/rpc_client.c +++ b/libfreerdp/core/gateway/rpc_client.c @@ -692,7 +692,7 @@ static int rpc_client_nondefault_out_channel_recv(rdpRpc* rpc) WLog_ERR(TAG, "rpc_client_nondefault_out_channel_recv: Unexpected message %08" PRIx32, nextOutChannel->State); - return -1; + status = -1; } http_response_free(response); diff --git a/libfreerdp/core/info.c b/libfreerdp/core/info.c index 3f8afaef2..cc3967586 100644 --- a/libfreerdp/core/info.c +++ b/libfreerdp/core/info.c @@ -719,7 +719,7 @@ static BOOL rdp_write_info_packet(rdpRdp* rdp, wStream* s) } ptrconv; if (settings->RedirectionPasswordLength > UINT16_MAX) - return FALSE; + goto fail; usedPasswordCookie = TRUE; ptrconv.bp = settings->RedirectionPassword; diff --git a/rdtk/librdtk/rdtk_nine_patch.c b/rdtk/librdtk/rdtk_nine_patch.c index a189021f1..fb946affb 100644 --- a/rdtk/librdtk/rdtk_nine_patch.c +++ b/rdtk/librdtk/rdtk_nine_patch.c @@ -394,6 +394,8 @@ int rdtk_nine_patch_engine_init(rdtkEngine* engine) else winpr_image_free(image, TRUE); } + else + winpr_image_free(image, TRUE); } if (!engine->textField9patch) @@ -402,6 +404,7 @@ int rdtk_nine_patch_engine_init(rdtkEngine* engine) BYTE* data; status = -1; size = rdtk_get_embedded_resource_file("textfield_default.9.png", &data); + image = NULL; if (size > 0) { @@ -420,6 +423,8 @@ int rdtk_nine_patch_engine_init(rdtkEngine* engine) else winpr_image_free(image, TRUE); } + else + winpr_image_free(image, TRUE); } return 1; diff --git a/winpr/tools/makecert/makecert.c b/winpr/tools/makecert/makecert.c index 2f2300524..c605396df 100644 --- a/winpr/tools/makecert/makecert.c +++ b/winpr/tools/makecert/makecert.c @@ -81,9 +81,16 @@ static char* makecert_read_str(BIO* bio, size_t* pOffset) new_len = length * 2; if (new_len == 0) new_len = 2048; + + if (new_len > INT_MAX) + { + status = -1; + break; + } + new_str = (char*)realloc(x509_str, new_len); - if (!new_str || (new_len > INT_MAX)) + if (!new_str) { status = -1; break;