Revamped environment variables again.

A previous commit changed how environment variables were managed.
Initially we were calling SetEnvironmentVariable() on each variable
defined in AppEnvironmentExtra prior to starting the service.  That
behaviour was changed because it was technically incorrect, potentially
resulting in NSSM's own environment being harmed.

Unfortunately the changed method, creating a new environment block by
calling GetEnvironmentStrings() (or using the block from AppEnvironment
if that and AppEnvironmentExtra were both defined), failed if users did
something like set AppEnvironmentExtra to PATH=C:\bin;%PATH%.  That
would result in the process being started with a block which included
TWO occurrences of the PATH variable; the system-defined one and the
appended one.  The latter would be ignored, possibly causing the
application to fail to start.

For this third attempt we now call GetEnvironmentStrings() once at
startup, storing the block in a new variable which is only freed at
exit.  Immediately prior to calling CreateProcess() to start the service
we use the new environment functions duplicate_environment() (on
AppEnvironment) and set_environment_block() (on AppEnvironmentExtra) to
set the variables.  Then immediately after calling CreateProcess() we
call duplicate_environment() again to restore the original block.

Because they are passed to expand_environment_string(), the environment
blocks are subject to expansion.  That means that setting PATH as
described in the example above would work.  Trying to append items to
the PATH by setting AppEnvironment in a similar way will probably not
work.

Thanks Bryan Senseman.

README.txt

@@ -354,7 +354,33 @@ environment variables which will be added to the service's environment.
 Each entry in the list should be of the form KEY=VALUE.  It is possible to
 omit the VALUE but the = symbol is mandatory.
 
-srvany only supports AppEnvironment.
+Environment variables listed in both AppEnvironment and AppEnvironmentExtra
+are subject to normal expansion, so it is possible, for example, to update the
+system path by setting "PATH=C:\bin;%PATH%" in AppEnvironmentExtra.  Variables
+are expanded in the order in which they appear, so if you want to include the
+value of one variable in another variable you should declare the dependency
+first.
+
+Because variables defined in AppEnvironment override the existing
+environment it is not possible to refer to any variables which were previously
+defined.
+
+For example, the following AppEnvironment block:
+
+      PATH=C:\Windows\System32;C:\Windows
+      PATH=C:\bin;%PATH%
+
+Would result in a PATH of "C:\bin;C:\Windows\System32;C:\Windows" as expected.
+
+Whereas the following AppEnvironment block:
+
+      PATH=C:\bin;%PATH%
+
+Would result in a path containing only C:\bin and probably cause the
+application to fail to start.
+
+Most people will want to use AppEnvironmentExtra exclusively.  srvany only
+supports AppEnvironment.
 
 
 Managing services using the GUI

registry.cpp

@@ -466,67 +466,6 @@ int get_parameters(nssm_service_t *service, STARTUPINFO *si) {
   /* Environment variables to add to existing rather than replace - may fail. */
   get_environment(service->name, key, NSSM_REG_ENV_EXTRA, &service->env_extra, &service->env_extralen);
 
-  if (si) {
-    if (service->env_extra) {
-      TCHAR *env;
-      unsigned long envlen;
-
-      /* Copy our environment for the application. */
-      if (! service->env) {
-        TCHAR *rawenv = GetEnvironmentStrings();
-        env = rawenv;
-        if (env) {
-          /*
-            The environment block starts with variables of the form
-            =C:=C:\Windows\System32 which we ignore.
-          */
-          while (*env == _T('=')) {
-            for ( ; *env; env++);
-            env++;
-          }
-          envlen = 0;
-          if (*env) {
-            while (true) {
-              for ( ; env[envlen]; envlen++);
-              if (! env[++envlen]) break;
-            }
-            envlen++;
-
-            service->envlen = envlen * sizeof(TCHAR);
-            service->env = (TCHAR *) HeapAlloc(GetProcessHeap(), 0, service->envlen);
-            memmove(service->env, env, service->envlen);
-            FreeEnvironmentStrings(rawenv);
-          }
-        }
-      }
-
-      /* Append extra variables to configured variables. */
-      if (service->env) {
-        envlen = service->envlen + service->env_extralen - sizeof(TCHAR)/*?*/;
-        env = (TCHAR *) HeapAlloc(GetProcessHeap(), 0, envlen);
-        if (env) {
-          memmove(env, service->env, service->envlen - sizeof(TCHAR));
-          /* envlen is in bytes but env[i] is in characters. */
-          memmove(env + (service->envlen / sizeof(TCHAR)) - 1, service->env_extra, service->env_extralen);
-
-          HeapFree(GetProcessHeap(), 0, service->env);
-          HeapFree(GetProcessHeap(), 0, service->env_extra);
-          service->env = env;
-          service->envlen = envlen;
-        }
-        else log_event(EVENTLOG_ERROR_TYPE, NSSM_EVENT_OUT_OF_MEMORY, _T("environment"), _T("get_parameters()"), 0);
-      }
-      else {
-        /* Huh?  No environment at all? */
-        service->env = service->env_extra;
-        service->envlen = service->env_extralen;
-      }
-    }
-
-    service->env_extra = 0;
-    service->env_extralen = 0;
-  }
-
   /* Try to get priority - may fail. */
   unsigned long priority;
   if (get_number(key, NSSM_REG_PRIORITY, &priority, false) == 1) {

service.cpp

@@ -681,6 +681,7 @@ void cleanup_nssm_service(nssm_service_t *service) {
   if (service->wait_handle) UnregisterWait(service->process_handle);
   if (service->throttle_section_initialised) DeleteCriticalSection(&service->throttle_section);
   if (service->throttle_timer) CloseHandle(service->throttle_timer);
+  if (service->initial_env) FreeEnvironmentStrings(service->initial_env);
   HeapFree(GetProcessHeap(), 0, service);
 }
 
@@ -1324,6 +1325,9 @@ void WINAPI service_main(unsigned long argc, TCHAR **argv) {
     }
   }
 
+  /* Remember our initial environment. */
+  service->initial_env = GetEnvironmentStrings();
+
   monitor_service(service);
 }
 
@@ -1519,23 +1523,21 @@ int start_service(nssm_service_t *service) {
 
   throttle_restart(service);
 
+  /* Set the environment. */
+  if (service->env) duplicate_environment(service->env);
+  if (service->env_extra) set_environment_block(service->env_extra);
+
   bool inherit_handles = false;
   if (si.dwFlags & STARTF_USESTDHANDLES) inherit_handles = true;
   unsigned long flags = service->priority & priority_mask();
   if (service->stdin_pipe) flags |= DETACHED_PROCESS;
   if (service->affinity) flags |= CREATE_SUSPENDED;
-#ifdef UNICODE
-  flags |= CREATE_UNICODE_ENVIRONMENT;
-#endif
-  if (! CreateProcess(0, cmd, 0, 0, inherit_handles, flags, service->env, service->dir, &si, &pi)) {
+  if (! CreateProcess(0, cmd, 0, 0, inherit_handles, flags, 0, service->dir, &si, &pi)) {
     unsigned long exitcode = 3;
     unsigned long error = GetLastError();
-    if (error == ERROR_INVALID_PARAMETER && service->env) {
-      log_event(EVENTLOG_ERROR_TYPE, NSSM_EVENT_CREATEPROCESS_FAILED_INVALID_ENVIRONMENT, service->name, service->exe, NSSM_REG_ENV, 0);
-      if (test_environment(service->env)) exitcode = 4;
-    }
-    else log_event(EVENTLOG_ERROR_TYPE, NSSM_EVENT_CREATEPROCESS_FAILED, service->name, service->exe, error_string(error), 0);
+    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);
     return stop_service(service, exitcode, true, true);
   }
   service->process_handle = pi.hProcess;
@@ -1545,6 +1547,9 @@ int start_service(nssm_service_t *service) {
 
   close_output_handles(&si);
 
+  /* Restore our environment. */
+  duplicate_environment(service->initial_env);
+
   if (service->affinity) {
     /*
       We are explicitly storing service->affinity as a 64-bit unsigned integer

service.h

@@ -101,6 +101,7 @@ typedef struct {
   LARGE_INTEGER throttle_duetime;
   FILETIME creation_time;
   FILETIME exit_time;
+  TCHAR *initial_env;
 } nssm_service_t;
 
 void WINAPI service_main(unsigned long, TCHAR **);