From f25947e68b8f53affa9f3c172de5470247a43f78 Mon Sep 17 00:00:00 2001 From: Jamie Newbon Date: Tue, 8 Sep 2020 12:15:06 +0100 Subject: [PATCH 1/3] #6567 Fixed memory leak causing zombie handles to created in the deamon on windows Capture all process information as they are created to properly kill off handles when process are stopped, also added in a kill everything when the program cleans up all synergy(s|c) processes --- src/lib/platform/MSWindowsWatchdog.cpp | 37 +++++++++++++++++++++-- src/lib/platform/MSWindowsWatchdog.h | 41 ++++++++++++++++++-------- 2 files changed, 63 insertions(+), 15 deletions(-) diff --git a/src/lib/platform/MSWindowsWatchdog.cpp b/src/lib/platform/MSWindowsWatchdog.cpp index eff124a14..3a8b2dbd9 100644 --- a/src/lib/platform/MSWindowsWatchdog.cpp +++ b/src/lib/platform/MSWindowsWatchdog.cpp @@ -37,6 +37,7 @@ #include #include +#define CURRENT_PROCESS_ID 0 #define MAXIMUM_WAIT_TIME 3 enum { kOutputBufferSize = 4096 @@ -338,6 +339,7 @@ MSWindowsWatchdog::startProcess() // wait for program to fail. ARCH->sleep(1); if (!isProcessActive()) { + closeProcessHandles(m_processInfo.dwProcessId); throw XMSWindowsWatchdogError("process immediately stopped"); } @@ -376,9 +378,13 @@ MSWindowsWatchdog::startProcessInForeground(String& command) si.dwFlags |= STARTF_USESHOWWINDOW; si.wShowWindow = SW_MINIMIZE; - return CreateProcess( + BOOL result = CreateProcess( NULL, LPSTR(command.c_str()), NULL, NULL, TRUE, 0, NULL, NULL, &si, &m_processInfo); + + m_children.insert(std::make_pair(m_processInfo.dwProcessId, m_processInfo)); + + return result; } BOOL @@ -409,6 +415,8 @@ MSWindowsWatchdog::startProcessAsUser(String& command, HANDLE userToken, LPSECUR sa, NULL, TRUE, creationFlags, environment, NULL, &si, &m_processInfo); + m_children.insert(std::make_pair(m_processInfo.dwProcessId, m_processInfo)); + DestroyEnvironmentBlock(environment); CloseHandle(userToken); @@ -529,13 +537,15 @@ MSWindowsWatchdog::shutdownProcess(HANDLE handle, DWORD pid, int timeout) ARCH->sleep(1); } } + + closeProcessHandles(pid); } void MSWindowsWatchdog::shutdownExistingProcesses() { // first we need to take a snapshot of the running processes - HANDLE snapshot = CreateToolhelp32Snapshot(TH32CS_SNAPPROCESS, 0); + HANDLE snapshot = CreateToolhelp32Snapshot(TH32CS_SNAPPROCESS, CURRENT_PROCESS_ID); if (snapshot == INVALID_HANDLE_VALUE) { LOG((CLOG_ERR "could not get process snapshot")); throw XArch(new XArchEvalWindows); @@ -581,6 +591,7 @@ MSWindowsWatchdog::shutdownExistingProcesses() } } + clearAllChildren(); CloseHandle(snapshot); m_processRunning = false; } @@ -601,7 +612,7 @@ MSWindowsWatchdog::getActiveDesktop(LPSECURITY_ATTRIBUTES security) m_elevateProcess = elevateProcess; BOOL createRet = startProcessAsUser(syntoolCommand, userToken, security); - + auto pid = m_processInfo.dwProcessId; if (!createRet) { DWORD rc = GetLastError(); RevertToSelf(); @@ -622,6 +633,7 @@ MSWindowsWatchdog::getActiveDesktop(LPSECURITY_ATTRIBUTES security) } m_ready = false; ARCH->unlockMutex(m_mutex); + closeProcessHandles(pid); } } @@ -644,3 +656,22 @@ MSWindowsWatchdog::testOutput(String buffer) ARCH->unlockMutex(m_mutex); } } + +void MSWindowsWatchdog::closeProcessHandles(unsigned long pid, bool removeFromMap) { + auto processInfo = m_children.find(pid); + if (processInfo != m_children.end()) { + CloseHandle(processInfo->second.hProcess); + CloseHandle(processInfo->second.hThread); + if(removeFromMap) { + m_children.erase(processInfo); + } + } +} + +void MSWindowsWatchdog::clearAllChildren() { + for (auto it = m_children.begin(); it != m_children.end(); ++it) + { + closeProcessHandles(it->second.dwThreadId, false); + } + m_children.clear(); +} diff --git a/src/lib/platform/MSWindowsWatchdog.h b/src/lib/platform/MSWindowsWatchdog.h index f1feedb4f..6a1d38adf 100644 --- a/src/lib/platform/MSWindowsWatchdog.h +++ b/src/lib/platform/MSWindowsWatchdog.h @@ -26,6 +26,7 @@ #include #include #include +#include class Thread; class IpcLogOutputter; @@ -42,7 +43,7 @@ public: virtual ~MSWindowsWatchdog(); void startAsync(); - std::string getCommand() const; + std::string getCommand() const; void setCommand(const std::string& command, bool elevate); void stop(); bool isProcessActive(); @@ -53,8 +54,8 @@ private: void outputLoop(void*); void shutdownProcess(HANDLE handle, DWORD pid, int timeout); void shutdownExistingProcesses(); - HANDLE duplicateProcessToken(HANDLE process, LPSECURITY_ATTRIBUTES security); - HANDLE getUserToken(LPSECURITY_ATTRIBUTES security); + HANDLE duplicateProcessToken(HANDLE process, LPSECURITY_ATTRIBUTES security); + HANDLE getUserToken(LPSECURITY_ATTRIBUTES security); void startProcess(); BOOL startProcessAsUser(String& command, HANDLE userToken, LPSECURITY_ATTRIBUTES sa); BOOL startProcessInForeground(String& command); @@ -62,29 +63,45 @@ private: void getActiveDesktop(LPSECURITY_ATTRIBUTES security); void testOutput(String buffer); void setStartupInfo(STARTUPINFO& si); + void checkChildren(); + /** + * @brief This closes the handles held to a child thread + * @param pid the ID of the process to kill, will do nothing if PID is not a valid child + * @param removeFromMap should the function remove the item from the children map + */ + void closeProcessHandles(unsigned long pid, bool removeFromMap = true); + /** + * @brief This kills off all children's handles created by this process + */ + void clearAllChildren(); private: - Thread* m_thread; + Thread* m_thread; bool m_autoDetectCommand; - std::string m_command; + std::string m_command; bool m_monitoring; bool m_commandChanged; - HANDLE m_stdOutWrite; - HANDLE m_stdOutRead; - Thread* m_outputThread; - IpcServer& m_ipcServer; + HANDLE m_stdOutWrite; + HANDLE m_stdOutRead; + Thread* m_outputThread; + IpcServer& m_ipcServer; IpcLogOutputter& m_ipcLogOutputter; bool m_elevateProcess; MSWindowsSession m_session; PROCESS_INFORMATION m_processInfo; - int m_processFailures; + int m_processFailures; bool m_processRunning; - FileLogOutputter* m_fileLogOutputter; + FileLogOutputter* m_fileLogOutputter; bool m_autoElevated; - ArchMutex m_mutex; + ArchMutex m_mutex; ArchCond m_condVar; bool m_ready; bool m_foreground; + + /// @brief Save the info of all process made + /// We will use this to track all processes we make and + /// kill off handels and children that we no longer need + std::map m_children; }; //! Relauncher error From 307128900f033a109e3f9fc1858d2ab02eb6fba2 Mon Sep 17 00:00:00 2001 From: Jamie Newbon Date: Tue, 8 Sep 2020 12:18:59 +0100 Subject: [PATCH 2/3] #6567 Updated ChangeLog --- ChangeLog | 1 + 1 file changed, 1 insertion(+) diff --git a/ChangeLog b/ChangeLog index 81a0ca3c2..7195c921b 100644 --- a/ChangeLog +++ b/ChangeLog @@ -2,6 +2,7 @@ v1.12.x-snapshot =========== Bug fixes: - #6753 Fixed a number of vulnerabilities detected by static analysis +- #6567 Fixed Synergy spawning hundreds of zombie processes Enhancements: - #6750 Integrate SonarCloud for static analysis and test coverage From d191e4681834950b9b82239bdf9e209099606d49 Mon Sep 17 00:00:00 2001 From: Jamie Newbon Date: Tue, 15 Sep 2020 09:18:39 +0100 Subject: [PATCH 3/3] #6567 Closed open handle created when shutting a process down --- src/lib/platform/MSWindowsWatchdog.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/lib/platform/MSWindowsWatchdog.cpp b/src/lib/platform/MSWindowsWatchdog.cpp index 3a8b2dbd9..96448d4d4 100644 --- a/src/lib/platform/MSWindowsWatchdog.cpp +++ b/src/lib/platform/MSWindowsWatchdog.cpp @@ -574,6 +574,7 @@ MSWindowsWatchdog::shutdownExistingProcesses() HANDLE handle = OpenProcess(PROCESS_ALL_ACCESS, FALSE, entry.th32ProcessID); shutdownProcess(handle, entry.th32ProcessID, 10); + CloseHandle(handle); } }