libutil-tests: Fix crash on Windows

src/libfetchers/mercurial.cc

@@ -1,5 +1,6 @@
 #include "nix/fetchers/fetchers.hh"
 #include "nix/util/processes.hh"
+#include "nix/util/environment-variables.hh"
 #include "nix/util/users.hh"
 #include "nix/fetchers/cache.hh"
 #include "nix/store/globals.hh"
@@ -16,9 +17,9 @@ namespace nix::fetchers {
 
 static RunOptions hgOptions(const Strings & args)
 {
-    auto env = getEnv();
+    auto env = getEnvOs();
     // Set HGPLAIN: this means we get consistent output from hg and avoids leakage from a user or system .hgrc.
-    env["HGPLAIN"] = "";
+    env[OS_STR("HGPLAIN")] = OS_STR("");
 
     return {.program = "hg", .lookupPath = true, .args = args, .environment = env};
 }

src/libmain/shared.cc

@@ -17,9 +17,12 @@
 #include <cstdlib>
 #include <sys/time.h>
 #include <sys/stat.h>
-#include <sys/resource.h>
 #include <unistd.h>
 #include <signal.h>
+
+#ifndef _WIN32
+#  include <sys/resource.h>
+#endif
 #ifdef __linux__
 #  include <features.h>
 #endif

src/libstore/build/derivation-building-goal.cc

@@ -5,6 +5,7 @@
 #  include "nix/store/build/derivation-builder.hh"
 #endif
 #include "nix/util/processes.hh"
+#include "nix/util/environment-variables.hh"
 #include "nix/util/config-global.hh"
 #include "nix/store/build/worker.hh"
 #include "nix/util/util.hh"
@@ -888,11 +889,12 @@ static void runPostBuildHook(
         fmt("running post-build-hook '%s'", workerSettings.postBuildHook),
         Logger::Fields{store.printStorePath(drvPath)});
     PushActivity pact(act.id);
-    StringMap hookEnvironment = getEnv();
+    OsStringMap hookEnvironment = getEnvOs();
 
-    hookEnvironment.emplace("DRV_PATH", store.printStorePath(drvPath));
-    hookEnvironment.emplace("OUT_PATHS", chomp(concatStringsSep(" ", store.printStorePathSet(outputPaths))));
-    hookEnvironment.emplace("NIX_CONFIG", globalConfig.toKeyValue());
+    hookEnvironment.emplace(OS_STR("DRV_PATH"), string_to_os_string(store.printStorePath(drvPath)));
+    hookEnvironment.emplace(
+        OS_STR("OUT_PATHS"), string_to_os_string(chomp(concatStringsSep(" ", store.printStorePathSet(outputPaths)))));
+    hookEnvironment.emplace(OS_STR("NIX_CONFIG"), string_to_os_string(globalConfig.toKeyValue()));
 
     struct LogSink : Sink
     {

src/libutil/environment-variables.cc

@@ -1,8 +1,6 @@
 #include "nix/util/util.hh"
 #include "nix/util/environment-variables.hh"
 
-extern char ** environ __attribute__((weak));
-
 namespace nix {
 
 std::optional<std::string> getEnv(const std::string & key)
@@ -29,24 +27,10 @@ std::optional<OsString> getEnvOsNonEmpty(const OsString & key)
     return value;
 }
 
-StringMap getEnv()
-{
-    StringMap env;
-    for (size_t i = 0; environ[i]; ++i) {
-        auto s = environ[i];
-        auto eq = strchr(s, '=');
-        if (!eq)
-            // invalid env, just keep going
-            continue;
-        env.emplace(std::string(s, eq), std::string(eq + 1));
-    }
-    return env;
-}
-
 void clearEnv()
 {
-    for (auto & name : getEnv())
-        unsetenv(name.first.c_str());
+    for (auto & [name, value] : getEnvOs())
+        unsetEnvOs(name.c_str());
 }
 
 void replaceEnv(const StringMap & newEnv)

src/libutil/include/nix/util/environment-variables.hh

@@ -23,6 +23,11 @@ std::optional<std::string> getEnv(const std::string & key);
  */
 std::optional<OsString> getEnvOs(const OsString & key);
 
+/**
+ * Like `getEnv`, but using `OsString` to avoid coercions.
+ */
+OsStringMap getEnvOs();
+
 /**
  * @return a non empty environment variable. Returns nullopt if the env
  * variable is set to ""
@@ -60,6 +65,11 @@ int setEnv(const char * name, const char * value);
  */
 int setEnvOs(const OsString & name, const OsString & value);
 
+/**
+ * Like `unsetenv`, but using `OsChar` to avoid coercions.
+ */
+int unsetEnvOs(const OsChar * name);
+
 /**
  * Clear the environment.
  */

src/libutil/include/nix/util/os-string.hh

@@ -1,6 +1,7 @@
 #pragma once
 ///@file
 
+#include <map>
 #include <optional>
 #include <string>
 #include <string_view>
@@ -36,6 +37,11 @@ using OsString = std::basic_string<OsChar>;
  */
 using OsStringView = std::basic_string_view<OsChar>;
 
+/**
+ * `nix::StringMap` counterpart for `OsString`
+ */
+using OsStringMap = std::map<OsString, OsString, std::less<>>;
+
 std::string os_string_to_string(OsStringView path);
 
 OsString string_to_os_string(std::string_view s);

src/libutil/include/nix/util/processes.hh

@@ -4,6 +4,7 @@
 #include "nix/util/types.hh"
 #include "nix/util/error.hh"
 #include "nix/util/file-descriptor.hh"
+#include "nix/util/file-path.hh"
 #include "nix/util/logging.hh"
 #include "nix/util/ansicolor.hh"
 
@@ -119,8 +120,8 @@ struct RunOptions
     std::optional<uid_t> uid;
     std::optional<uid_t> gid;
 #endif
-    std::optional<Path> chdir;
-    std::optional<StringMap> environment;
+    std::optional<std::filesystem::path> chdir;
+    std::optional<OsStringMap> environment;
     std::optional<std::string> input;
     Source * standardIn = nullptr;
     Sink * standardOut = nullptr;

src/libutil/unix/environment-variables.cc

@@ -1,7 +1,10 @@
 #include <cstdlib>
+#include <cstring>
 
 #include "nix/util/environment-variables.hh"
 
+extern char ** environ __attribute__((weak));
+
 namespace nix {
 
 int setEnv(const char * name, const char * value)
@@ -14,9 +17,33 @@ std::optional<std::string> getEnvOs(const std::string & key)
     return getEnv(key);
 }
 
+OsStringMap getEnvOs()
+{
+    OsStringMap env;
+    for (size_t i = 0; environ[i]; ++i) {
+        auto s = environ[i];
+        auto eq = strchr(s, '=');
+        if (!eq)
+            // invalid env, just keep going
+            continue;
+        env.emplace(std::string(s, eq), std::string(eq + 1));
+    }
+    return env;
+}
+
+StringMap getEnv()
+{
+    return getEnvOs();
+}
+
 int setEnvOs(const OsString & name, const OsString & value)
 {
     return setEnv(name.c_str(), value.c_str());
 }
 
+int unsetEnvOs(const OsChar * name)
+{
+    return unsetenv(name);
+}
+
 } // namespace nix

src/libutil/windows/environment-variables.cc

@@ -2,6 +2,7 @@
 
 #ifdef _WIN32
 #  include "processenv.h"
+#  include "shlwapi.h"
 
 namespace nix {
 
@@ -30,11 +31,50 @@ std::optional<OsString> getEnvOs(const OsString & key)
     return value;
 }
 
+OsStringMap getEnvOs()
+{
+    OsStringMap env;
+
+    auto freeStrings = [](wchar_t * strings) { FreeEnvironmentStringsW(strings); };
+    auto envStrings = std::unique_ptr<wchar_t, decltype(freeStrings)>(GetEnvironmentStringsW(), freeStrings);
+    auto s = envStrings.get();
+
+    while (true) {
+        auto eq = StrChrW(s, L'=');
+        // Object ends with an empty string, which naturally won't have an =
+        if (eq == nullptr)
+            break;
+
+        auto value_len = lstrlenW(eq + 1);
+
+        env[OsString(s, eq - s)] = OsString(eq + 1, value_len);
+
+        // 1 to skip L'=', then value, then 1 to skip L'\0'
+        s = eq + 1 + value_len + 1;
+    }
+
+    return env;
+}
+
+StringMap getEnv()
+{
+    StringMap env;
+    for (auto & [name, value] : getEnvOs()) {
+        env.emplace(os_string_to_string(name), os_string_to_string(value));
+    }
+    return env;
+}
+
 int unsetenv(const char * name)
 {
     return -SetEnvironmentVariableA(name, nullptr);
 }
 
+int unsetEnvOs(const OsChar * name)
+{
+    return -SetEnvironmentVariableW(name, nullptr);
+}
+
 int setEnv(const char * name, const char * value)
 {
     return -SetEnvironmentVariableA(name, value);

src/libutil/windows/processes.cc

@@ -4,6 +4,7 @@
 #include "nix/util/executable-path.hh"
 #include "nix/util/file-descriptor.hh"
 #include "nix/util/file-path.hh"
+#include "nix/util/os-string.hh"
 #include "nix/util/signals.hh"
 #include "nix/util/processes.hh"
 #include "nix/util/finally.hh"
@@ -56,29 +57,30 @@ void Pid::operator=(AutoCloseFD pid)
     this->pid = std::move(pid);
 }
 
-// TODO: Implement (not needed for process spawning yet)
 int Pid::kill(bool allowInterrupts)
 {
     assert(pid.get() != INVALID_DESCRIPTOR);
 
     debug("killing process %1%", pid.get());
 
-    throw UnimplementedError("Pid::kill unimplemented");
+    if (!TerminateProcess(pid.get(), 1))
+        logError(WinError("terminating process %1%", pid.get()).info());
+
+    return wait(allowInterrupts);
 }
 
+// Note that `allowInterrupts` is ignored for now, but there to match
+// Unix.
 int Pid::wait(bool allowInterrupts)
 {
-    // https://github.com/nix-windows/nix/blob/windows-meson/src/libutil/util.cc#L1938
     assert(pid.get() != INVALID_DESCRIPTOR);
     DWORD status = WaitForSingleObject(pid.get(), INFINITE);
-    if (status != WAIT_OBJECT_0) {
-        debug("WaitForSingleObject returned %1%", status);
-    }
+    if (status != WAIT_OBJECT_0)
+        throw WinError("waiting for process %1%", pid.get());
 
     DWORD exitCode = 0;
-    if (GetExitCodeProcess(pid.get(), &exitCode) == FALSE) {
-        debug("GetExitCodeProcess failed on pid %1%", pid.get());
-    }
+    if (GetExitCodeProcess(pid.get(), &exitCode) == FALSE)
+        throw WinError("getting exit code of process %1%", pid.get());
 
     pid.close();
     return exitCode;
@@ -216,18 +218,20 @@ Pid spawnProcess(const Path & realProgram, const RunOptions & options, Pipe & ou
     startInfo.hStdOutput = out.writeSide.get();
     startInfo.hStdError = out.writeSide.get();
 
-    std::string envline;
-    // Retain the current processes' environment variables.
-    for (const auto & envVar : getEnv()) {
-        envline += (envVar.first + '=' + envVar.second + '\0');
-    }
-    // Also add new ones specified in options.
+    auto env = getEnvOs();
+
     if (options.environment) {
         for (const auto & envVar : *options.environment) {
-            envline += (envVar.first + '=' + envVar.second + '\0');
+            env[envVar.first] = envVar.second;
         }
     }
 
+    OsString envline;
+
+    for (const auto & envVar : env) {
+        envline += (envVar.first + L'=' + envVar.second + L'\0');
+    }
+
     std::string cmdline = windowsEscape(realProgram, false);
     for (const auto & arg : options.args) {
         // TODO: This isn't the right way to escape windows command
@@ -244,8 +248,8 @@ Pid spawnProcess(const Path & realProgram, const RunOptions & options, Pipe & ou
             NULL,
             TRUE,
             CREATE_UNICODE_ENVIRONMENT | CREATE_SUSPENDED,
-            string_to_os_string(envline).data(),
-            options.chdir.has_value() ? string_to_os_string(*options.chdir).data() : NULL,
+            envline.data(),
+            options.chdir.has_value() ? options.chdir->c_str() : NULL,
             &startInfo,
             &procInfo)
         == 0) {

src/nix/repl.cc

@@ -7,14 +7,15 @@
 #include "nix/cmd/installable-value.hh"
 #include "nix/cmd/repl.hh"
 #include "nix/util/processes.hh"
+#include "nix/util/environment-variables.hh"
 #include "self-exe.hh"
 
 namespace nix {
 
 void runNix(const std::string & program, const Strings & args, const std::optional<std::string> & input = {})
 {
-    auto subprocessEnv = getEnv();
-    subprocessEnv["NIX_CONFIG"] = globalConfig.toKeyValue();
+    auto subprocessEnv = getEnvOs();
+    subprocessEnv[OS_STR("NIX_CONFIG")] = string_to_os_string(globalConfig.toKeyValue());
     // isInteractive avoid grabling interactive commands
     runProgram2(
         RunOptions{