From: Iain Patterson <me@iain.cx>
Date: Tue, 1 Apr 2014 21:20:21 +0000 (+0100)
Subject: Fixed broken environment on restart.
X-Git-Tag: v2.22~3
X-Git-Url: http://git.iain.cx/?p=nssm.git;a=commitdiff_plain;h=620fc9f56975c9319a1b0e0be556e264ad1d485d

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
--- 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
 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
 -------
diff --git a/env.cpp b/env.cpp
index 76c187e..063b60f 100644
--- 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
--- 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
--- a/service.cpp
+++ b/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) {
     /*