Merge pull request #6762 from symless/issue-6567-zombie-processes

SYNERGY-92 Fixed deamon leaving zombie process hanging about feeding on memory

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

src/lib/platform/MSWindowsWatchdog.cpp

@@ -37,6 +37,7 @@
 #include <UserEnv.h>
 #include <Shellapi.h>
 
+#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();
+}

src/lib/platform/MSWindowsWatchdog.h

@@ -26,6 +26,7 @@
 #include <Windows.h>
 #include <string>
 #include <list>
+#include <map>
 
 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              duplicateProcessToken(HANDLE process, LPSECURITY_ATTRIBUTES security);
-    HANDLE                getUserToken(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_stdOutWrite;
-    HANDLE                m_stdOutRead;
+    HANDLE              m_stdOutRead;
-    Thread*                m_outputThread;
+    Thread*             m_outputThread;
-    IpcServer&            m_ipcServer;
+    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<unsigned long, PROCESS_INFORMATION>   m_children;
 };
 
 //! Relauncher error