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.

diff --git a/README.txt b/README.txt
index 32051ab..b0d32a7 100644 (file)
--- a/README.txt
+++ b/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\r
 and/or stderr which attempt to read from stdin would fail.\r
 Thanks to Czenda Czendov for help with Visual Studio 2013 and Server 2012R2.\r
+Thanks to Alessandro Gherardi for reporting and draft fix of the bug whereby\r
+the second restart of the application would have a corrupted environment.\r
 \r
 Licence\r
 -------\r

diff --git a/env.cpp b/env.cpp
index 76c187e..063b60f 100644 (file)
--- a/env.cpp
+++ b/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);
+}

diff --git a/env.h b/env.h
index 24d8987..021ff4f 100644 (file)
--- a/env.h
+++ b/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

diff --git a/service.cpp b/service.cpp
index 5b10233..8d1c0fe 100644 (file)
--- a/service.cpp
+++ b/service.cpp
@@ -1623,7 +1623,7 @@ int start_service(nssm_service_t *service) {
     unsigned long error = GetLastError();\r
     log_event(EVENTLOG_ERROR_TYPE, NSSM_EVENT_CREATEPROCESS_FAILED, service->name, service->exe, error_string(error), 0);\r
     close_output_handles(&si);\r
-    duplicate_environment(service->initial_env);\r
+    duplicate_environment_strings(service->initial_env);\r
     return stop_service(service, exitcode, true, true);\r
   }\r
   service->process_handle = pi.hProcess;\r
@@ -1636,7 +1636,7 @@ int start_service(nssm_service_t *service) {
   if (! service->no_console) FreeConsole();\r
 \r
   /* Restore our environment. */\r
-  duplicate_environment(service->initial_env);\r
+  duplicate_environment_strings(service->initial_env);\r
 \r
   if (service->affinity) {\r
     /*\r