From c9abefbc304a1b3bacdacd4a74beeb78d4db89a5 Mon Sep 17 00:00:00 2001 From: Artemis Tosini Date: Fri, 13 Feb 2026 15:56:18 -0500 Subject: [PATCH 1/2] libutil-tests: Fix crash on Windows libutil tests were crashing on Windows due to issues finding `environ`. Replace process creation of `getEnv` with a new `getEnvOs` function that uses native windows APIs. Also convert a bunch of `RunOptions` fields to use `OsString` to better reflect the underlying interfaces. Co-authored-by: John Ericson --- src/libfetchers/mercurial.cc | 5 ++- src/libmain/shared.cc | 5 ++- .../build/derivation-building-goal.cc | 10 +++-- src/libutil/environment-variables.cc | 20 +--------- .../include/nix/util/environment-variables.hh | 10 +++++ src/libutil/include/nix/util/os-string.hh | 6 +++ src/libutil/include/nix/util/processes.hh | 5 ++- src/libutil/unix/environment-variables.cc | 27 +++++++++++++ src/libutil/windows/environment-variables.cc | 40 +++++++++++++++++++ src/libutil/windows/processes.cc | 21 +++++----- src/nix/repl.cc | 5 ++- 11 files changed, 116 insertions(+), 38 deletions(-) diff --git a/src/libfetchers/mercurial.cc b/src/libfetchers/mercurial.cc index dd31e224f6c..005409176cd 100644 --- a/src/libfetchers/mercurial.cc +++ b/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}; } diff --git a/src/libmain/shared.cc b/src/libmain/shared.cc index 0efebc9a8c0..44b5914b83c 100644 --- a/src/libmain/shared.cc +++ b/src/libmain/shared.cc @@ -17,9 +17,12 @@ #include #include #include -#include #include #include + +#ifndef _WIN32 +# include +#endif #ifdef __linux__ # include #endif diff --git a/src/libstore/build/derivation-building-goal.cc b/src/libstore/build/derivation-building-goal.cc index bdbdf5ee10e..e19c9cf1f37 100644 --- a/src/libstore/build/derivation-building-goal.cc +++ b/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 { diff --git a/src/libutil/environment-variables.cc b/src/libutil/environment-variables.cc index 14886059c75..9d44ad65d8d 100644 --- a/src/libutil/environment-variables.cc +++ b/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 getEnv(const std::string & key) @@ -29,24 +27,10 @@ std::optional 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) diff --git a/src/libutil/include/nix/util/environment-variables.hh b/src/libutil/include/nix/util/environment-variables.hh index 7f742130f94..5fccf53c841 100644 --- a/src/libutil/include/nix/util/environment-variables.hh +++ b/src/libutil/include/nix/util/environment-variables.hh @@ -23,6 +23,11 @@ std::optional getEnv(const std::string & key); */ std::optional 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. */ diff --git a/src/libutil/include/nix/util/os-string.hh b/src/libutil/include/nix/util/os-string.hh index f0cbcbaba5b..cc96a1fb8d8 100644 --- a/src/libutil/include/nix/util/os-string.hh +++ b/src/libutil/include/nix/util/os-string.hh @@ -1,6 +1,7 @@ #pragma once ///@file +#include #include #include #include @@ -36,6 +37,11 @@ using OsString = std::basic_string; */ using OsStringView = std::basic_string_view; +/** + * `nix::StringMap` counterpart for `OsString` + */ +using OsStringMap = std::map>; + std::string os_string_to_string(OsStringView path); OsString string_to_os_string(std::string_view s); diff --git a/src/libutil/include/nix/util/processes.hh b/src/libutil/include/nix/util/processes.hh index 81e6a2b83cd..44e8c2952d3 100644 --- a/src/libutil/include/nix/util/processes.hh +++ b/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; std::optional gid; #endif - std::optional chdir; - std::optional environment; + std::optional chdir; + std::optional environment; std::optional input; Source * standardIn = nullptr; Sink * standardOut = nullptr; diff --git a/src/libutil/unix/environment-variables.cc b/src/libutil/unix/environment-variables.cc index c68e3bcad0a..2adef0cf5fc 100644 --- a/src/libutil/unix/environment-variables.cc +++ b/src/libutil/unix/environment-variables.cc @@ -1,7 +1,10 @@ #include +#include #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 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 diff --git a/src/libutil/windows/environment-variables.cc b/src/libutil/windows/environment-variables.cc index c76c1234553..51efca91b64 100644 --- a/src/libutil/windows/environment-variables.cc +++ b/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 getEnvOs(const OsString & key) return value; } +OsStringMap getEnvOs() +{ + OsStringMap env; + + auto freeStrings = [](wchar_t * strings) { FreeEnvironmentStringsW(strings); }; + auto envStrings = std::unique_ptr(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); diff --git a/src/libutil/windows/processes.cc b/src/libutil/windows/processes.cc index 135583ad028..d7b24b3f2c6 100644 --- a/src/libutil/windows/processes.cc +++ b/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" @@ -216,18 +217,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 +247,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) { diff --git a/src/nix/repl.cc b/src/nix/repl.cc index 5450001e4e9..1fc30954b72 100644 --- a/src/nix/repl.cc +++ b/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 & 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{ From 36d0e9580f39628414841ed75e94fa4314464116 Mon Sep 17 00:00:00 2001 From: Artemis Tosini Date: Fri, 13 Feb 2026 16:41:34 -0500 Subject: [PATCH 2/2] Implement `Pid::kill` for Windows Co-authored-by: John Ericson --- src/libutil/windows/processes.cc | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/src/libutil/windows/processes.cc b/src/libutil/windows/processes.cc index d7b24b3f2c6..577f94fd050 100644 --- a/src/libutil/windows/processes.cc +++ b/src/libutil/windows/processes.cc @@ -57,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;