diff --git a/ChangeLog b/ChangeLog index c0c2e2529..f805a4b48 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 - #6755 Fixed Vulnerability in vreadf function detected by Static analysis - #6758 Account name with space on windows causes synergy to error when starting - #6760 Synergy loses license when creating a System scope config diff --git a/src/lib/platform/MSWindowsWatchdog.cpp b/src/lib/platform/MSWindowsWatchdog.cpp index eff124a14..96448d4d4 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); @@ -564,6 +574,7 @@ MSWindowsWatchdog::shutdownExistingProcesses() HANDLE handle = OpenProcess(PROCESS_ALL_ACCESS, FALSE, entry.th32ProcessID); shutdownProcess(handle, entry.th32ProcessID, 10); + CloseHandle(handle); } } @@ -581,6 +592,7 @@ MSWindowsWatchdog::shutdownExistingProcesses() } } + clearAllChildren(); CloseHandle(snapshot); m_processRunning = false; } @@ -601,7 +613,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 +634,7 @@ MSWindowsWatchdog::getActiveDesktop(LPSECURITY_ATTRIBUTES security) } m_ready = false; ARCH->unlockMutex(m_mutex); + closeProcessHandles(pid); } } @@ -644,3 +657,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