From ec102dce28306077265c454a5922afed6a8a27c6 Mon Sep 17 00:00:00 2001 From: Bryan Berns Date: Wed, 23 May 2018 19:53:36 -0400 Subject: [PATCH] Alternate Thread Creation API To Avoid Memory Leaks (#306) * Alternate Thread Creation API To Avoid Memory Leaks - Switched from CreateThread() to _beginthreadex() and ExitThread() to _endthreadex() in order to avoid potential leaks when linking with static CRT library. - Addressed a variety of warnings that were being detected with static code analysis. * Addressed Type Cast Warning - Added explicit cast to the output of _beginthreadex() to avoid a compiler warning. * Indentation Fix --- contrib/win32/win32compat/Debug.h | 2 +- contrib/win32/win32compat/fileio.c | 8 +++++++- contrib/win32/win32compat/inc/process.h | 6 ------ contrib/win32/win32compat/misc.c | 12 ++++++------ contrib/win32/win32compat/shell-host.c | 14 +++++++------- contrib/win32/win32compat/signal_internal.h | 1 + contrib/win32/win32compat/signal_wait.c | 8 ++++---- contrib/win32/win32compat/termio.c | 13 +++++++------ contrib/win32/win32compat/w32fd.c | 4 ++-- contrib/win32/win32compat/w32log.c | 2 +- regress/unittests/win32compat/signal_tests.c | 8 ++++---- 11 files changed, 40 insertions(+), 38 deletions(-) delete mode 100644 contrib/win32/win32compat/inc/process.h diff --git a/contrib/win32/win32compat/Debug.h b/contrib/win32/win32compat/Debug.h index a9902308c..69a3af49e 100644 --- a/contrib/win32/win32compat/Debug.h +++ b/contrib/win32/win32compat/Debug.h @@ -1,6 +1,6 @@ #pragma once -void fatal(const char *, ...); +void __declspec(noreturn) fatal(const char *, ...); void error(const char *, ...); void verbose(const char *, ...); void debug(const char *, ...); diff --git a/contrib/win32/win32compat/fileio.c b/contrib/win32/win32compat/fileio.c index 9ba4fab0e..d78234b29 100644 --- a/contrib/win32/win32compat/fileio.c +++ b/contrib/win32/win32compat/fileio.c @@ -1072,8 +1072,14 @@ fileio_readlink(const char *path, char *buf, size_t bufsiz) goto cleanup; } + /* allocate the maximum possible size the reparse buffer size could be */ + reparse_buffer = (PREPARSE_DATA_BUFFER_SYMLINK)malloc(MAXIMUM_REPARSE_DATA_BUFFER_SIZE); + if (reparse_buffer == NULL) { + errno = ENOMEM; + goto cleanup; + } + /* send a request to the file system to get the real path */ - reparse_buffer = (PREPARSE_DATA_BUFFER_SYMLINK) malloc(MAXIMUM_REPARSE_DATA_BUFFER_SIZE); DWORD dwBytesReturned = 0; if (DeviceIoControl(handle, FSCTL_GET_REPARSE_POINT, NULL, 0, (LPVOID) reparse_buffer, MAXIMUM_REPARSE_DATA_BUFFER_SIZE, &dwBytesReturned, 0) == 0) { diff --git a/contrib/win32/win32compat/inc/process.h b/contrib/win32/win32compat/inc/process.h deleted file mode 100644 index 691a96b98..000000000 --- a/contrib/win32/win32compat/inc/process.h +++ /dev/null @@ -1,6 +0,0 @@ -#ifndef Process_H -#define Process_H - -/* Compatibility header to avoid lots of #ifdef _WIN32's in includes.h */ - -#endif diff --git a/contrib/win32/win32compat/misc.c b/contrib/win32/win32compat/misc.c index 5bd32870d..ec63fcc8d 100644 --- a/contrib/win32/win32compat/misc.c +++ b/contrib/win32/win32compat/misc.c @@ -1034,8 +1034,8 @@ resolved_path_utf16(const char *input_path) else resolved_path = utf8_to_utf16(input_path); - if (resolved_path == NULL) { - errno = ENOMEM; + if (resolved_path == NULL) { + errno = ENOMEM; return NULL; } @@ -1052,10 +1052,10 @@ resolved_path_utf16(const char *input_path) if (changed_req > 0) { wchar_t * resolved_path_new = realloc(resolved_path, (resolved_len + changed_req + 1) * sizeof(wchar_t)); - if (resolved_path == NULL) { - debug3("%s: memory allocation failed.", __FUNCTION__); - free(resolved_path); - errno = ENOMEM; + if (resolved_path_new == NULL) { + debug3("%s: memory allocation failed.", __FUNCTION__); + free(resolved_path); + errno = ENOMEM; return NULL; } else resolved_path = resolved_path_new; diff --git a/contrib/win32/win32compat/shell-host.c b/contrib/win32/win32compat/shell-host.c index 8ea9a59d7..02a85e8a6 100644 --- a/contrib/win32/win32compat/shell-host.c +++ b/contrib/win32/win32compat/shell-host.c @@ -38,6 +38,7 @@ #include #include #include +#include #include "misc_internal.h" #include "inc\utf.h" @@ -790,7 +791,7 @@ SizeWindow(HANDLE hInput) bSuccess = GetConsoleScreenBufferInfoEx(hInput, &consoleInfo); } -DWORD WINAPI +unsigned __stdcall MonitorChild(_In_ LPVOID lpParameter) { WaitForSingleObject(child, INFINITE); @@ -1009,7 +1010,7 @@ ProcessEvent(void *p) return ERROR_SUCCESS; } -DWORD WINAPI +unsigned __stdcall ProcessEventQueue(LPVOID p) { while (1) { @@ -1107,7 +1108,7 @@ void FreeQueueEvent() LeaveCriticalSection(&criticalSection); } -DWORD WINAPI +unsigned __stdcall ProcessPipes(LPVOID p) { BOOL ret; @@ -1293,7 +1294,7 @@ start_with_pty(wchar_t *command) /* monitor child exist */ child = pi.hProcess; - monitor_thread = CreateThread(NULL, 0, MonitorChild, NULL, 0, NULL); + monitor_thread = (HANDLE) _beginthreadex(NULL, 0, MonitorChild, NULL, 0, NULL); if (IS_INVALID_HANDLE(monitor_thread)) goto cleanup; @@ -1302,11 +1303,11 @@ start_with_pty(wchar_t *command) initialize_keylen(); - io_thread = CreateThread(NULL, 0, ProcessPipes, NULL, 0, NULL); + io_thread = (HANDLE) _beginthreadex(NULL, 0, ProcessPipes, NULL, 0, NULL); if (IS_INVALID_HANDLE(io_thread)) goto cleanup; - ux_thread = CreateThread(NULL, 0, ProcessEventQueue, NULL, 0, NULL); + ux_thread = (HANDLE) _beginthreadex(NULL, 0, ProcessEventQueue, NULL, 0, NULL); if (IS_INVALID_HANDLE(ux_thread)) goto cleanup; @@ -1348,7 +1349,6 @@ cleanup: return child_exit_code; } -/* shellhost.exe */ int wmain(int ac, wchar_t **av) { diff --git a/contrib/win32/win32compat/signal_internal.h b/contrib/win32/win32compat/signal_internal.h index 715151c80..ac319f6ab 100644 --- a/contrib/win32/win32compat/signal_internal.h +++ b/contrib/win32/win32compat/signal_internal.h @@ -1,4 +1,5 @@ #include +#include /* child processes */ #define MAX_CHILDREN 512 diff --git a/contrib/win32/win32compat/signal_wait.c b/contrib/win32/win32compat/signal_wait.c index 6a767c379..e45a7b3c3 100644 --- a/contrib/win32/win32compat/signal_wait.c +++ b/contrib/win32/win32compat/signal_wait.c @@ -43,7 +43,7 @@ typedef struct _wait_for_multiple_objects_struct { } wait_for_multiple_objects_struct; -DWORD WINAPI +unsigned __stdcall wait_for_multiple_objects_thread(LPVOID lpParam) { wait_for_multiple_objects_struct *waitstruct = @@ -66,7 +66,7 @@ wait_for_multiple_objects_interrupter(_In_ ULONG_PTR dwParam) * the alert prior to the thread running in which case it is acknowledged when * the threads starts running instead of when it is waiting at * WaitForMultipleObjectsEx */ - ExitThread(0); + _endthreadex(0); } DWORD @@ -144,8 +144,8 @@ wait_for_multiple_objects_enhanced(_In_ DWORD nCount, _In_ const HANDLE *lpHand wait_bins[bin].num_handles = min(nCount - handles_processed, bin_size); /* create a thread for this bin */ - if ((wait_bins[bin].thread_handle = CreateThread(NULL, 2048, - wait_for_multiple_objects_thread, + if ((wait_bins[bin].thread_handle = (HANDLE) _beginthreadex(NULL, + 2048, wait_for_multiple_objects_thread, (LPVOID) &(wait_bins[bin]), 0, NULL)) == NULL) { goto cleanup; } diff --git a/contrib/win32/win32compat/termio.c b/contrib/win32/win32compat/termio.c index fa662149e..54bbe02e8 100644 --- a/contrib/win32/win32compat/termio.c +++ b/contrib/win32/win32compat/termio.c @@ -40,6 +40,7 @@ */ #include +#include #include "w32fd.h" #include "tncon.h" #include "inc\utf.h" @@ -69,7 +70,7 @@ ReadAPCProc(_In_ ULONG_PTR dwParam) } /* Read worker thread */ -static DWORD WINAPI +static unsigned __stdcall ReadThread(_In_ LPVOID lpParameter) { int nBytesReturned = 0; @@ -154,10 +155,10 @@ syncio_initiate_read(struct w32_io* pio) pio->read_details.buf_size = TERM_IO_BUF_SIZE; } - read_thread = CreateThread(NULL, 0, ReadThread, pio, 0, NULL); + read_thread = (HANDLE) _beginthreadex(NULL, 0, ReadThread, pio, 0, NULL); if (read_thread == NULL) { errno = errno_from_Win32LastError(); - debug3("TermRead initiate - ERROR CreateThread %d, io:%p", GetLastError(), pio); + debug3("TermRead initiate - ERROR _beginthreadex %d, io:%p", GetLastError(), pio); return -1; } @@ -185,7 +186,7 @@ WriteAPCProc(_In_ ULONG_PTR dwParam) /* Write worker thread */ -static DWORD WINAPI +static unsigned __stdcall WriteThread(_In_ LPVOID lpParameter) { struct w32_io* pio = (struct w32_io*)lpParameter; @@ -231,10 +232,10 @@ syncio_initiate_write(struct w32_io* pio, DWORD num_bytes) debug5("syncio_initiate_write initiate io:%p", pio); memset(&(pio->sync_write_status), 0, sizeof(pio->sync_write_status)); pio->sync_write_status.to_transfer = num_bytes; - write_thread = CreateThread(NULL, 0, WriteThread, pio, 0, NULL); + write_thread = (HANDLE)_beginthreadex(NULL, 0, WriteThread, pio, 0, NULL); if (write_thread == NULL) { errno = errno_from_Win32LastError(); - debug3("syncio_initiate_write initiate - ERROR CreateThread %d, io:%p", GetLastError(), pio); + debug3("syncio_initiate_write initiate - ERROR _beginthreadex %d, io:%p", GetLastError(), pio); return -1; } diff --git a/contrib/win32/win32compat/w32fd.c b/contrib/win32/win32compat/w32fd.c index d836090fe..2888e12e5 100644 --- a/contrib/win32/win32compat/w32fd.c +++ b/contrib/win32/win32compat/w32fd.c @@ -1126,7 +1126,7 @@ fd_encode_state(const posix_spawn_file_actions_t *file_actions, HANDLE aux_h[]) char *buf, *encoded; struct std_fd_state *std_fd_state; struct inh_fd_state *c; - DWORD len_req; + DWORD len_req = 0; BOOL b; int i; int fd_in = file_actions->stdio_redirect[STDIN_FILENO]; @@ -1174,7 +1174,7 @@ static void fd_decode_state(char* enc_buf) { char* buf; - DWORD req, skipped, out_flags; + DWORD req = 0, skipped, out_flags; struct std_fd_state *std_fd_state; struct inh_fd_state *c; int num_inherited = 0; diff --git a/contrib/win32/win32compat/w32log.c b/contrib/win32/win32compat/w32log.c index a45aad5f9..6d64bc5bf 100644 --- a/contrib/win32/win32compat/w32log.c +++ b/contrib/win32/win32compat/w32log.c @@ -142,7 +142,7 @@ syslog_file(int priority, const char *format, const char *formatBuffer) GetCurrentProcessId(), st.wYear, st.wMonth, st.wDay, st.wHour, st.wMinute, st.wSecond, st.wMilliseconds, formatBuffer); if (r == -1) { - _write(logfd, "_snprintf_s failed.", 30); + _write(logfd, "_snprintf_s failed.", 20); return; } msgbufTimestamp[strnlen(msgbufTimestamp, MSGBUFSIZ)] = '\0'; diff --git a/regress/unittests/win32compat/signal_tests.c b/regress/unittests/win32compat/signal_tests.c index 1b5a3f7e6..d05dd8c97 100644 --- a/regress/unittests/win32compat/signal_tests.c +++ b/regress/unittests/win32compat/signal_tests.c @@ -23,7 +23,7 @@ signal_test_send_apc(LPVOID lpParam) return TRUE; } -DWORD WINAPI +unsigned __stdcall signal_test_set_event(LPVOID lpParam) { HANDLE hevent = (HANDLE)lpParam; @@ -32,7 +32,7 @@ signal_test_set_event(LPVOID lpParam) return TRUE; } -DWORD WINAPI +unsigned __stdcall signal_create_abandoned_object(LPVOID lpParam) { *((HANDLE *)lpParam) = CreateMutex(NULL, TRUE, 0); @@ -65,7 +65,7 @@ signal_test_wait_for_multiple_objects() /* create abandoned mutex */ HANDLE abandoned_mutux = NULL; - HANDLE mutex_thread = CreateThread(NULL, 0, signal_create_abandoned_object, &abandoned_mutux, 0, NULL); + HANDLE mutex_thread = (HANDLE) _beginthreadex(NULL, 0, signal_create_abandoned_object, &abandoned_mutux, 0, NULL); WaitForSingleObject(mutex_thread, INFINITE); CloseHandle(mutex_thread); @@ -137,7 +137,7 @@ signal_test_wait_for_multiple_objects() for (int i = 0; i < objects_size; i++) ResetEvent(hObjects[i]); for (int i = 0; i < objects_size; i++) { - CloseHandle(CreateThread(NULL, 0, signal_test_set_event, hObjects[i], 0, NULL)); + CloseHandle((HANDLE) _beginthreadex(NULL, 0, signal_test_set_event, hObjects[i], 0, NULL)); DWORD ret = wait_for_multiple_objects_enhanced(objects_size, hObjects, 10000, FALSE); ASSERT_INT_EQ(ret, i + WAIT_OBJECT_0_ENHANCED); ResetEvent(hObjects[i]);