From: Iain Patterson <me@iain.cx>
Date: Sun, 2 Dec 2012 02:22:08 +0000 (-0800)
Subject: Fixed crash when stopping the service.
X-Git-Tag: v2.16~1
X-Git-Url: http://git.iain.cx/?a=commitdiff_plain;h=9884e231415258809dfb1ef117b6bc9a5b011514;p=nssm.git

Fixed crash when stopping the service.

NSSM tidies up by looking for processes whose parent PID matches the PID
of the monitored application.  Because process IDs are reused, however,
it's possible for NSSM to kill a process which was not a child of the
monitored application if that process had a now-exited parent process
whose PID was reused.  On UNIX the PPID would show as 1 for an orphaned
process but under Windows, at least when querying a process with the
Toolhelp32 toolkit, we see the original PPID of the process even if the
parent is no longer running.  Thus it is not sufficient to compare
parent PIDs when checking if a process was launched by our application.

In the worst case scenario NSSM could even terminate itself and trigger
a bluescreen.

When we find process with matching PPID we now also check that it was
created after our application was created and before our application
exited.
---

diff --git a/messages.mc b/messages.mc
index 3fc7dff..d8cb74c 100644
--- a/messages.mc
+++ b/messages.mc
@@ -1100,3 +1100,19 @@ Errore in fase di configurazione delle aziondi di fallimento per il servizio %1.
 Chiamata a ChangeServiceConfig2() fallita:
 %2
 .
+
+MessageId = +1
+SymbolicName = NSSM_EVENT_GETPROCESSTIMES_FAILED
+Severity = Error
+Language = English
+GetProcessTimes() failed:
+%1
+.
+Language = French
+Échec de GetProcessTimes():
+%1
+.
+Language = Italian
+Chiamata a GetProcessTimes():
+%1
+.
diff --git a/process.cpp b/process.cpp
index 97b6e38..b63cc41 100644
--- a/process.cpp
+++ b/process.cpp
@@ -1,5 +1,66 @@
 #include "nssm.h"
 
+int get_process_creation_time(HANDLE process_handle, FILETIME *ft) {
+  FILETIME creation_time, exit_time, kernel_time, user_time;
+
+  if (! GetProcessTimes(process_handle, &creation_time, &exit_time, &kernel_time, &user_time)) {
+    log_event(EVENTLOG_ERROR_TYPE, NSSM_EVENT_GETPROCESSTIMES_FAILED, error_string(GetLastError()), 0);
+    return 1;
+  }
+
+  memmove(ft, &creation_time, sizeof(creation_time));
+
+  return 0;
+}
+
+int get_process_exit_time(HANDLE process_handle, FILETIME *ft) {
+  FILETIME creation_time, exit_time, kernel_time, user_time;
+
+  if (! GetProcessTimes(process_handle, &creation_time, &exit_time, &kernel_time, &user_time)) {
+    log_event(EVENTLOG_ERROR_TYPE, NSSM_EVENT_GETPROCESSTIMES_FAILED, error_string(GetLastError()), 0);
+    return 1;
+  }
+
+  memmove(ft, &exit_time, sizeof(exit_time));
+
+  return 0;
+}
+
+int check_parent(char *service_name, PROCESSENTRY32 *pe, unsigned long ppid, FILETIME *pft, FILETIME *exit_time) {
+  /* Check parent process ID matches. */
+  if (pe->th32ParentProcessID != ppid) return 1;
+
+  /*
+    Process IDs can be reused so do a sanity check by making sure the child
+    has been running for less time than the parent.
+    Though unlikely, it's possible that the parent exited and its process ID
+    was already reused, so we'll also compare against its exit time.
+  */
+  HANDLE process_handle = OpenProcess(PROCESS_QUERY_INFORMATION, false, pe->th32ProcessID);
+  if (! process_handle) {
+    char pid_string[16];
+    _snprintf(pid_string, sizeof(pid_string), "%d", pe->th32ProcessID);
+    log_event(EVENTLOG_ERROR_TYPE, NSSM_EVENT_OPENPROCESS_FAILED, pid_string, service_name, error_string(GetLastError()), 0);
+    return 2;
+  }
+
+  FILETIME ft;
+  if (get_process_creation_time(process_handle, &ft)) {
+    CloseHandle(process_handle);
+    return 3;
+  }
+
+  CloseHandle(process_handle);
+
+  /* Verify that the parent's creation time is not later. */
+  if (CompareFileTime(pft, &ft) > 0) return 4;
+
+  /* Verify that the parent's exit time is not earlier. */
+  if (CompareFileTime(exit_time, &ft) < 0) return 5;
+
+  return 0;
+}
+
 /* Send some window messages and hope the window respects one or more. */
 int CALLBACK kill_window(HWND window, LPARAM arg) {
   kill_t *k = (kill_t *) arg;
@@ -43,6 +104,7 @@ int kill_threads(char *service_name, kill_t *k) {
 
   if (! Thread32First(snapshot, &te)) {
     log_event(EVENTLOG_ERROR_TYPE, NSSM_EVENT_THREAD_ENUMERATE_FAILED, service_name, error_string(GetLastError()), 0);
+    CloseHandle(snapshot);
     return 0;
   }
 
@@ -57,6 +119,7 @@ int kill_threads(char *service_name, kill_t *k) {
       unsigned long error = GetLastError();
       if (error == ERROR_NO_MORE_FILES) break;
       log_event(EVENTLOG_ERROR_TYPE, NSSM_EVENT_THREAD_ENUMERATE_FAILED, service_name, error_string(GetLastError()), 0);
+      CloseHandle(snapshot);
       return ret;
     }
 
@@ -65,6 +128,8 @@ int kill_threads(char *service_name, kill_t *k) {
     }
   }
 
+  CloseHandle(snapshot);
+
   return ret;
 }
 
@@ -72,6 +137,12 @@ int kill_threads(char *service_name, kill_t *k) {
 int kill_process(char *service_name, HANDLE process_handle, unsigned long pid, unsigned long exitcode) {
   /* Shouldn't happen. */
   if (! pid) return 1;
+  if (! process_handle) return 1;
+
+  unsigned long ret;
+  if (GetExitCodeProcess(process_handle, &ret)) {
+    if (ret != STILL_ACTIVE) return 1;
+  }
 
   kill_t k = { pid, exitcode, 0 };
 
@@ -98,7 +169,7 @@ int kill_process(char *service_name, HANDLE process_handle, unsigned long pid, u
   return TerminateProcess(process_handle, exitcode);
 }
 
-void kill_process_tree(char *service_name, unsigned long pid, unsigned long exitcode, unsigned long ppid) {
+void kill_process_tree(char *service_name, unsigned long pid, unsigned long exitcode, unsigned long ppid, FILETIME *parent_creation_time, FILETIME *parent_exit_time) {
   /* Shouldn't happen unless the service failed to start. */
   if (! pid) return;
 
@@ -120,11 +191,12 @@ void kill_process_tree(char *service_name, unsigned long pid, unsigned long exit
 
   if (! Process32First(snapshot, &pe)) {
     log_event(EVENTLOG_ERROR_TYPE, NSSM_EVENT_PROCESS_ENUMERATE_FAILED, service_name, error_string(GetLastError()), 0);
+    CloseHandle(snapshot);
     return;
   }
 
   /* This is a child of the doomed process so kill it. */
-  if (pe.th32ParentProcessID == pid) kill_process_tree(service_name, pe.th32ProcessID, exitcode, ppid);
+  if (! check_parent(service_name, &pe, pid, parent_creation_time, parent_exit_time)) kill_process_tree(service_name, pe.th32ProcessID, exitcode, ppid, parent_creation_time, parent_exit_time);
 
   while (true) {
     /* Try to get the next process. */
@@ -132,12 +204,15 @@ void kill_process_tree(char *service_name, unsigned long pid, unsigned long exit
       unsigned long ret = GetLastError();
       if (ret == ERROR_NO_MORE_FILES) break;
       log_event(EVENTLOG_ERROR_TYPE, NSSM_EVENT_PROCESS_ENUMERATE_FAILED, service_name, error_string(GetLastError()), 0);
+      CloseHandle(snapshot);
       return;
     }
 
-    if (pe.th32ParentProcessID == pid) kill_process_tree(service_name, pe.th32ProcessID, exitcode, ppid);
+    if (! check_parent(service_name, &pe, pid, parent_creation_time, parent_exit_time)) kill_process_tree(service_name, pe.th32ProcessID, exitcode, ppid, parent_creation_time, parent_exit_time);
   }
 
+  CloseHandle(snapshot);
+
   /* We will need a process handle in order to call TerminateProcess() later. */
   HANDLE process_handle = OpenProcess(SYNCHRONIZE | PROCESS_QUERY_INFORMATION | PROCESS_VM_READ | PROCESS_TERMINATE, false, pid);
   if (! process_handle) {
@@ -151,7 +226,8 @@ void kill_process_tree(char *service_name, unsigned long pid, unsigned long exit
   if (! kill_process(service_name, process_handle, pid, exitcode)) {
     /* Maybe it already died. */
     unsigned long ret;
-    if (! GetExitCodeProcess(process_handle, &ret)) log_event(EVENTLOG_ERROR_TYPE, NSSM_EVENT_TERMINATEPROCESS_FAILED, pid_string, service_name, error_string(GetLastError()), 0);
-    return;
+    if (! GetExitCodeProcess(process_handle, &ret) || ret == STILL_ACTIVE) log_event(EVENTLOG_ERROR_TYPE, NSSM_EVENT_TERMINATEPROCESS_FAILED, pid_string, service_name, error_string(GetLastError()), 0);
   }
+
+  CloseHandle(process_handle);
 }
diff --git a/process.h b/process.h
index 23a7706..c12d733 100644
--- a/process.h
+++ b/process.h
@@ -9,9 +9,12 @@ typedef struct {
   int signalled;
 } kill_t;
 
+int get_process_creation_time(HANDLE, FILETIME *);
+int get_process_exit_time(HANDLE, FILETIME *);
+int check_parent(char *, PROCESSENTRY32 *, unsigned long, FILETIME *, FILETIME *);
 int CALLBACK kill_window(HWND, LPARAM);
 int kill_threads(char *, kill_t *);
 int kill_process(char *, HANDLE, unsigned long, unsigned long);
-void kill_process_tree(char *, unsigned long, unsigned long, unsigned long);
+void kill_process_tree(char *, unsigned long, unsigned long, unsigned long, FILETIME *, FILETIME *);
 
 #endif
diff --git a/service.cpp b/service.cpp
index 0c2368e..81e46ef 100644
--- a/service.cpp
+++ b/service.cpp
@@ -14,6 +14,7 @@ bool stopping;
 unsigned long throttle_delay;
 HANDLE throttle_timer;
 LARGE_INTEGER throttle_duetime;
+FILETIME creation_time;
 
 static enum { NSSM_EXIT_RESTART, NSSM_EXIT_IGNORE, NSSM_EXIT_REALLY, NSSM_EXIT_UNCLEAN } exit_actions;
 static const char *exit_action_strings[] = { "Restart", "Ignore", "Exit", "Suicide", 0 };
@@ -90,7 +91,7 @@ int install_service(char *name, char *exe, char *flags) {
     print_message(stderr, NSSM_MESSAGE_OPEN_SERVICE_MANAGER_FAILED);
     return 2;
   }
-  
+
   /* Get path of this program */
   char path[MAX_PATH];
   GetModuleFileName(0, path, MAX_PATH);
@@ -149,7 +150,7 @@ int remove_service(char *name) {
     print_message(stderr, NSSM_MESSAGE_OPEN_SERVICE_MANAGER_FAILED);
     return 2;
   }
-  
+
   /* Try to open the service */
   SC_HANDLE service = OpenService(services, name, SC_MANAGER_ALL_ACCESS);
   if (! service) {
@@ -391,6 +392,8 @@ int start_service() {
   process_handle = pi.hProcess;
   pid = pi.dwProcessId;
 
+  if (get_process_creation_time(process_handle, &creation_time)) ZeroMemory(&creation_time, sizeof(creation_time));
+
   /* Signal successful start */
   service_status.dwCurrentState = SERVICE_RUNNING;
   SetServiceStatus(service_handle, &service_status);
@@ -415,12 +418,11 @@ int stop_service(unsigned long exitcode, bool graceful, bool default_action) {
     SetServiceStatus(service_handle, &service_status);
   }
 
-  /* Nothing to do if server isn't running */
+  /* Nothing to do if service isn't running */
   if (pid) {
-    /* Shut down server */
+    /* Shut down service */
     log_event(EVENTLOG_INFORMATION_TYPE, NSSM_EVENT_TERMINATEPROCESS, service_name, exe, 0);
     kill_process(service_name, process_handle, pid, 0);
-    process_handle = 0;
   }
   else log_event(EVENTLOG_INFORMATION_TYPE, NSSM_EVENT_PROCESS_ALREADY_STOPPED, service_name, exe, 0);
 
@@ -454,7 +456,10 @@ void CALLBACK end_service(void *arg, unsigned char why) {
   /* Check exit code */
   unsigned long exitcode = 0;
   char code[16];
+  FILETIME exit_time;
   GetExitCodeProcess(process_handle, &exitcode);
+  if (exitcode == STILL_ACTIVE || get_process_exit_time(process_handle, &exit_time)) GetSystemTimeAsFileTime(&exit_time);
+  CloseHandle(process_handle);
 
   /*
     Log that the service ended BEFORE logging about killing the process
@@ -466,7 +471,7 @@ void CALLBACK end_service(void *arg, unsigned char why) {
   }
 
   /* Clean up. */
-  kill_process_tree(service_name, pid, exitcode, pid);
+  kill_process_tree(service_name, pid, exitcode, pid, &creation_time, &exit_time);
 
   /*
     The why argument is true if our wait timed out or false otherwise.