Browse Source

Fixed broken environment on restart.

We were calling duplicate_environment() to copy the service's initial
environment prior to restarting it.  However the duplicate_environment()
function modified its input by replacing = characters with NULLs.  That
is not allowed; the memory returned by GetEnvironmentStrings() must be
treated as readonly.

After two restarts the application would run with a null environment and
probably crash.

We now copy the initial environment on to the heap and operate on the
copy, ensuring that the corruption does not occur.

Thanks Alessandro Gherardi.
Iain Patterson 7 years ago
parent
commit
620fc9f569
4 changed files with 38 additions and 2 deletions
  1. 2 0
      README.txt
  2. 32 0
      env.cpp
  3. 2 0
      env.h
  4. 2 2
      service.cpp

+ 2 - 0
README.txt

@@ -679,6 +679,8 @@ Thanks to Andrew RedzMax for suggesting an unconditional restart delay.
 Thanks to Bryan Senseman for noticing that applications with redirected stdout
 and/or stderr which attempt to read from stdin would fail.
 Thanks to Czenda Czendov for help with Visual Studio 2013 and Server 2012R2.
+Thanks to Alessandro Gherardi for reporting and draft fix of the bug whereby
+the second restart of the application would have a corrupted environment.
 
 Licence
 -------

+ 32 - 0
env.cpp

@@ -1,5 +1,23 @@
 #include "nssm.h"
 
+/* Copy an environment block. */
+TCHAR *copy_environment_block(TCHAR *env) {
+  unsigned long len;
+
+  if (! env) return 0;
+  for (len = 0; env[len]; len++) while (env[len]) len++;
+  if (! len++) return 0;
+
+  TCHAR *newenv = (TCHAR *) HeapAlloc(GetProcessHeap(), 0, len * sizeof(TCHAR));
+  if (! newenv) {
+    log_event(EVENTLOG_ERROR_TYPE, NSSM_EVENT_OUT_OF_MEMORY, _T("environment"), _T("copy_environment_block()"), 0);
+    return 0;
+  }
+
+  memmove(newenv, env, len * sizeof(TCHAR));
+  return newenv;
+}
+
 /*
   The environment block starts with variables of the form
   =C:=C:\Windows\System32 which we ignore.
@@ -139,3 +157,17 @@ int test_environment(TCHAR *env) {
 
   return 0;
 }
+
+/*
+  Duplicate an environment block returned by GetEnvironmentStrings().
+  Since such a block is by definition readonly, and duplicate_environment()
+  modifies its inputs, this function takes a copy of the input and operates
+  on that.
+*/
+void duplicate_environment_strings(TCHAR *env) {
+  TCHAR *newenv = copy_environment_block(env);
+  if (! newenv) return;
+
+  duplicate_environment(newenv);
+  HeapFree(GetProcessHeap(), 0, newenv);
+}

+ 2 - 0
env.h

@@ -1,11 +1,13 @@
 #ifndef ENV_H
 #define ENV_H
 
+TCHAR *copy_environment_block(TCHAR *);
 TCHAR *useful_environment(TCHAR *);
 TCHAR *expand_environment_string(TCHAR *);
 int set_environment_block(TCHAR *);
 int clear_environment();
 int duplicate_environment(TCHAR *);
 int test_environment(TCHAR *);
+void duplicate_environment_strings(TCHAR *);
 
 #endif

+ 2 - 2
service.cpp

@@ -1623,7 +1623,7 @@ int start_service(nssm_service_t *service) {
     unsigned long error = GetLastError();
     log_event(EVENTLOG_ERROR_TYPE, NSSM_EVENT_CREATEPROCESS_FAILED, service->name, service->exe, error_string(error), 0);
     close_output_handles(&si);
-    duplicate_environment(service->initial_env);
+    duplicate_environment_strings(service->initial_env);
     return stop_service(service, exitcode, true, true);
   }
   service->process_handle = pi.hProcess;
@@ -1636,7 +1636,7 @@ int start_service(nssm_service_t *service) {
   if (! service->no_console) FreeConsole();
 
   /* Restore our environment. */
-  duplicate_environment(service->initial_env);
+  duplicate_environment_strings(service->initial_env);
 
   if (service->affinity) {
     /*