diff --git a/src/libcmd/repl.cc b/src/libcmd/repl.cc index 9e3d99d75d13..d8a7a0c7a763 100644 --- a/src/libcmd/repl.cc +++ b/src/libcmd/repl.cc @@ -510,9 +510,12 @@ ProcessLineResult NixRepl::processLine(std::string line) // runProgram redirects stdout to a StringSink, // using runProgram2 to allow editors to display their UI runProgram2({ - .program = editor, - .lookupPath = true, - .args = std::move(args), + .spawnOptions = + { + .program = editor, + .lookupPath = true, + .args = std::move(args), + }, .isInteractive = true, }); diff --git a/src/libfetchers/git-lfs-fetch.cc b/src/libfetchers/git-lfs-fetch.cc index c80732a0c21f..91e9e7f38946 100644 --- a/src/libfetchers/git-lfs-fetch.cc +++ b/src/libfetchers/git-lfs-fetch.cc @@ -73,7 +73,7 @@ LfsApiInfo getLfsApi(ParsedURL url) args.push_back(string_to_os_string(url.renderPath(/*encode=*/false))); args.push_back(OS_STR("download")); - auto [status, output] = runProgram({.program = "ssh", .args = args}); + auto [status, output] = runProgram({{.program = "ssh", .args = args}}); if (output.empty()) throw Error( diff --git a/src/libfetchers/git-utils.cc b/src/libfetchers/git-utils.cc index 2c5d6fddb7d0..414c5aca343c 100644 --- a/src/libfetchers/git-utils.cc +++ b/src/libfetchers/git-utils.cc @@ -750,7 +750,14 @@ struct GitRepoImpl : GitRepo, std::enable_shared_from_this gitArgs.push_back(string_to_os_string(url)); gitArgs.push_back(string_to_os_string(refspec)); - auto status = runProgram({.program = GIT_PROGRAM, .args = gitArgs, .isInteractive = true}).first; + auto [status, _] = runProgram({ + .spawnOptions = + { + .program = GIT_PROGRAM, + .args = gitArgs, + }, + .isInteractive = true, + }); if (status > 0) throw Error("Failed to fetch git repository '%s'", url); @@ -791,15 +798,18 @@ struct GitRepoImpl : GitRepo, std::enable_shared_from_this // Run verification command auto [status, output] = runProgram({ - .program = GIT_PROGRAM, - .args{ - OS_STR("-c"), - OS_STR("gpg.ssh.allowedSignersFile=") + allowedSignersFile.native(), - OS_STR("-C"), - path.native(), - OS_STR("verify-commit"), - string_to_os_string(rev.gitRev()), - }, + .spawnOptions = + { + .program = GIT_PROGRAM, + .args{ + OS_STR("-c"), + OS_STR("gpg.ssh.allowedSignersFile=") + allowedSignersFile.native(), + OS_STR("-C"), + path.native(), + OS_STR("verify-commit"), + string_to_os_string(rev.gitRev()), + }, + }, .mergeStderrToStdout = true, }); diff --git a/src/libfetchers/git.cc b/src/libfetchers/git.cc index ff1bec209b0b..3fb77bd3fd91 100644 --- a/src/libfetchers/git.cc +++ b/src/libfetchers/git.cc @@ -51,9 +51,12 @@ std::optional readHead(const std::filesystem::path & path) { auto [status, output] = runProgram( RunOptions{ - .program = GIT_PROGRAM, - // FIXME: use 'HEAD' to avoid returning all refs - .args = {OS_STR("ls-remote"), OS_STR("--symref"), path.native()}, + .spawnOptions = + { + .program = GIT_PROGRAM, + // FIXME: use 'HEAD' to avoid returning all refs + .args = {OS_STR("ls-remote"), OS_STR("--symref"), path.native()}, + }, .isInteractive = true, }); if (status != 0) @@ -501,16 +504,16 @@ struct GitInputScheme : InputScheme auto result = runProgram( RunOptions{ - .program = GIT_PROGRAM, - .args{ - OS_STR("-C"), - repoPath->native(), - OS_STR("--git-dir"), - string_to_os_string(repoInfo.gitDir), - OS_STR("check-ignore"), - OS_STR("--quiet"), - string_to_os_string(std::string(path.rel())), - }, + {.program = GIT_PROGRAM, + .args{ + OS_STR("-C"), + repoPath->native(), + OS_STR("--git-dir"), + string_to_os_string(repoInfo.gitDir), + OS_STR("check-ignore"), + OS_STR("--quiet"), + string_to_os_string(std::string(path.rel())), + }}, }); auto exitCode = #ifndef WIN32 // TODO abstract over exit status handling on Windows diff --git a/src/libfetchers/mercurial.cc b/src/libfetchers/mercurial.cc index dcf9062fe672..dbeaa43709d1 100644 --- a/src/libfetchers/mercurial.cc +++ b/src/libfetchers/mercurial.cc @@ -24,7 +24,12 @@ static RunOptions hgOptions(OsStrings args) // Set HGPLAIN: this means we get consistent output from hg and avoids leakage from a user or system .hgrc. env[OS_STR("HGPLAIN")] = OS_STR(""); - return {.program = HG_PROGRAM, .lookupPath = true, .args = std::move(args), .environment = env}; + return {{ + .program = HG_PROGRAM, + .lookupPath = true, + .args = std::move(args), + .environment = env, + }}; } // runProgram wrapper that uses hgOptions instead of stock RunOptions. diff --git a/src/libmain/shared.cc b/src/libmain/shared.cc index f0e3365c249b..e5afbcd6fbb9 100644 --- a/src/libmain/shared.cc +++ b/src/libmain/shared.cc @@ -381,7 +381,7 @@ RunPager::RunPager() return; /* TODO re-enable on Windows, once a child can be given our pipe as its - stdin: `RunOptions` has no `standardIn`, and `spawnProcess` hardcodes a + stdin: `RunOptions` has no `standardIn`, and `spawnProgram` hardcodes a null one. Printing unpaged beats failing the command. */ #ifdef _WIN32 return; diff --git a/src/libstore/build/derivation-builder-impl.cc b/src/libstore/build/derivation-builder-impl.cc index af399ee4baeb..cfb6a780b824 100644 --- a/src/libstore/build/derivation-builder-impl.cc +++ b/src/libstore/build/derivation-builder-impl.cc @@ -45,14 +45,16 @@ static void handleDiffHook( try { auto diffRes = runProgram( RunOptions{ - .program = diffHook, - .lookupPath = true, - .args = {tryA, tryB, drvPath, tmpDir}, + .spawnOptions = + {.program = diffHook, + .lookupPath = true, + .args = {tryA, tryB, drvPath, tmpDir}, #ifndef _WIN32 - .uid = uid, - .gid = gid, + .uid = uid, + .gid = gid, #endif - .chdir = "/"}); + .chdir = "/"}, + }); if (!statusOk(diffRes.first)) throw ExecError(diffRes.first, "diff-hook program %s %s", PathFmt(diffHook), statusToString(diffRes.first)); diff --git a/src/libstore/build/derivation-building-goal.cc b/src/libstore/build/derivation-building-goal.cc index 80c8f3dc25bf..7c7b1f023899 100644 --- a/src/libstore/build/derivation-building-goal.cc +++ b/src/libstore/build/derivation-building-goal.cc @@ -1085,8 +1085,8 @@ Goal::Co DerivationBuildingGoal::buildLocally( if (worker.settings.postBuildHook.get() != "") { #ifdef _WIN32 /* Nothing here needs `fork`: the child only sets the environment, - redirects stdout/stderr and execs, which `spawnProcess` already - does. What is missing is that `spawnProcess` is not exported, + redirects stdout/stderr and execs, which `spawnProgram` already + does. What is missing is that `spawnProgram` is not exported, and that the worker needs an `AsyncPipe` tied to the completion port rather than the plain `Pipe` this produces. Throw rather than silently skip the hook. */ diff --git a/src/libstore/include/nix/store/ssh.hh b/src/libstore/include/nix/store/ssh.hh index df3e4e77cf5e..d56d1331b566 100644 --- a/src/libstore/include/nix/store/ssh.hh +++ b/src/libstore/include/nix/store/ssh.hh @@ -40,9 +40,7 @@ private: void addCommonSSHOpts(OsStrings & args); bool isMasterRunning(); -#ifndef _WIN32 // TODO re-enable on Windows, once we can fork. std::filesystem::path startMaster(); -#endif public: diff --git a/src/libstore/ssh.cc b/src/libstore/ssh.cc index 0191590a98aa..da56433b3445 100644 --- a/src/libstore/ssh.cc +++ b/src/libstore/ssh.cc @@ -8,6 +8,9 @@ #include "nix/util/exec.hh" #include "nix/util/base-n.hh" +#include +#include + namespace nix { static std::string parsePublicHostKey(std::string_view host, std::string_view sshPublicHostKey) @@ -125,14 +128,18 @@ bool SSHMaster::isMasterRunning() OsStrings args = {OS_STR("-O"), OS_STR("check"), string_to_os_string(hostnameAndUser)}; addCommonSSHOpts(args); - auto res = runProgram(RunOptions{.program = SSH_PROGRAM, .args = std::move(args), .mergeStderrToStdout = true}); + auto res = runProgram( + RunOptions{ + .spawnOptions = {.program = SSH_PROGRAM, .args = std::move(args)}, + .mergeStderrToStdout = true, + }); return res.first == 0; } -Strings createSSHEnv() +static OsStringMap createSSHEnv() { // Copy the environment and set SHELL=/bin/sh - StringMap env = getEnv(); + OsStringMap env = getEnvOs(); // SSH will invoke the "user" shell for -oLocalCommand, but that means // $SHELL. To keep things simple and avoid potential issues with other @@ -140,21 +147,13 @@ Strings createSSHEnv() // Technically, we don't need that, and we could reinvoke ourselves to print // "started". Self-reinvocation is tricky with library consumers, but mostly // solved; refer to the development history of nixExePath in libstore/globals.cc. - env.insert_or_assign("SHELL", "/bin/sh"); - - Strings r; - for (auto & [k, v] : env) { - r.push_back(k + "=" + v); - } + env.insert_or_assign(OS_STR("SHELL"), OS_STR("/bin/sh")); - return r; + return env; } std::unique_ptr SSHMaster::startCommand(OsStrings && command, OsStrings && extraSshArgs) { -#ifdef _WIN32 // TODO re-enable on Windows, once we can fork. - throw UnimplementedError("cannot yet SSH on windows because spawning processes is not yet implemented"); -#else std::filesystem::path socketPath = startMaster(); Pipe in, out; @@ -162,52 +161,52 @@ std::unique_ptr SSHMaster::startCommand(OsStrings && comm out.create(); auto conn = std::make_unique(); - ProcessOptions options; - options.dieWithParent = false; std::unique_ptr loggerSuspension; if (!fakeSSH && !useMaster) { loggerSuspension = std::make_unique(logger->suspend()); } - conn->sshPid = startProcess( - [&]() { - restoreProcessContext(); - - close(in.writeSide.get()); - close(out.readSide.get()); - - if (dup2(in.readSide.get(), STDIN_FILENO) == -1) - throw SysError("duping over stdin"); - if (dup2(out.writeSide.get(), STDOUT_FILENO) == -1) - throw SysError("duping over stdout"); - if (logFD != -1 && dup2(logFD, STDERR_FILENO) == -1) - throw SysError("duping over stderr"); - - OsStrings args; - - if (!fakeSSH) { - args = {SSH_PROGRAM, hostnameAndUser.c_str(), "-x"}; - addCommonSSHOpts(args); - if (!socketPath.empty()) - args.insert(args.end(), {"-S", socketPath.string()}); - if (verbosity >= lvlChatty) - args.push_back("-v"); - args.splice(args.end(), std::move(extraSshArgs)); - args.push_back("--"); - } - - args.splice(args.end(), std::move(command)); - auto env = createSSHEnv(); - nix::execvpe(args.begin()->c_str(), stringsToCharPtrs(args).data(), stringsToCharPtrs(env).data()); - - // could not exec ssh/bash - throw SysError("unable to execute '%s'", args.front()); + OsStrings args; + std::filesystem::path program; + + if (!fakeSSH) { + program = SSH_PROGRAM; + args = {string_to_os_string(hostnameAndUser), OS_STR("-x")}; + addCommonSSHOpts(args); + if (!socketPath.empty()) + args.insert(args.end(), {OS_STR("-S"), socketPath.native()}); + if (verbosity >= lvlChatty) + args.push_back(OS_STR("-v")); + args.splice(args.end(), std::move(extraSshArgs)); + args.push_back(OS_STR("--")); + } else { + program = command.front(); + command.pop_front(); + } + + args.splice(args.end(), std::move(command)); + + std::vector fdr = { + {.from = in.readSide.get(), .to = FdRedirection::stdInput}, + {.from = out.writeSide.get(), .to = FdRedirection::stdOut}, + }; + + if (logFD != INVALID_DESCRIPTOR) + fdr.push_back({.from = logFD, .to = FdRedirection::stdError}); + + conn->sshPid = spawnProgram( + { + .program = std::move(program), + .lookupPath = true, + .args = std::move(args), + .environment = createSSHEnv(), + .dieWithParent = false, }, - options); + fdr); - in.readSide = INVALID_DESCRIPTOR; - out.writeSide = INVALID_DESCRIPTOR; + in.readSide.close(); + out.writeSide.close(); // Wait for the SSH connection to be established, // So that we don't overwrite the password prompt with our progress bar. @@ -228,11 +227,8 @@ std::unique_ptr SSHMaster::startCommand(OsStrings && comm conn->in = std::move(in.writeSide); return conn; -#endif } -#ifndef _WIN32 // TODO re-enable on Windows, once we can fork. - std::filesystem::path SSHMaster::startMaster() { if (!useMaster) @@ -240,7 +236,7 @@ std::filesystem::path SSHMaster::startMaster() auto state(state_.lock()); - if (state->sshMaster != INVALID_DESCRIPTOR) + if (state->sshMaster) return state->socketPath; state->socketPath = tmpDir->path() / "ssh.sock"; @@ -248,35 +244,35 @@ std::filesystem::path SSHMaster::startMaster() Pipe out; out.create(); - ProcessOptions options; - options.dieWithParent = false; - auto suspension = logger->suspend(); if (isMasterRunning()) return state->socketPath; - state->sshMaster = startProcess( - [&]() { - restoreProcessContext(); - - close(out.readSide.get()); - - if (dup2(out.writeSide.get(), STDOUT_FILENO) == -1) - throw SysError("duping over stdout"); - - OsStrings args = {"ssh", hostnameAndUser.c_str(), "-M", "-N", "-S", state->socketPath.string()}; - if (verbosity >= lvlChatty) - args.push_back("-v"); - addCommonSSHOpts(args); - auto env = createSSHEnv(); - nix::execvpe(args.begin()->c_str(), stringsToCharPtrs(args).data(), stringsToCharPtrs(env).data()); + OsStrings args = { + string_to_os_string(hostnameAndUser), + OS_STR("-M"), + OS_STR("-N"), + OS_STR("-S"), + state->socketPath.native(), + }; + if (verbosity >= lvlChatty) + args.push_back(OS_STR("-v")); + addCommonSSHOpts(args); - throw SysError("unable to execute '%s'", args.front()); + state->sshMaster = spawnProgram( + { + .program = SSH_PROGRAM, + .lookupPath = true, + .args = std::move(args), + .environment = createSSHEnv(), + .dieWithParent = false, }, - options); + std::to_array({ + {.from = out.writeSide.get(), .to = FdRedirection::stdOut}, + })); - out.writeSide = INVALID_DESCRIPTOR; + out.writeSide.close(); std::string reply; try { @@ -292,8 +288,6 @@ std::filesystem::path SSHMaster::startMaster() return state->socketPath; } -#endif - void SSHMaster::Connection::trySetBufferSize(size_t size) { #ifdef F_SETPIPE_SZ diff --git a/src/libstore/windows/build/windows-derivation-builder.cc b/src/libstore/windows/build/windows-derivation-builder.cc index f048aad8cbcb..5a4f9da4f725 100644 --- a/src/libstore/windows/build/windows-derivation-builder.cc +++ b/src/libstore/windows/build/windows-derivation-builder.cc @@ -196,7 +196,7 @@ OsString WindowsDerivationBuilderImpl::makeEnvBlock() /* For runtime strings; fixed names use `OS_STR`, which widens at compile time. */ auto os = [](std::string_view s) { return string_to_os_string(s); }; - /* Built from scratch rather than inherited. This is why `spawnProcess` is not + /* Built from scratch rather than inherited. This is why `spawnProgram` is not used below: it merges the parent's environment. */ env[OS_STR("NIX_BUILD_TOP")] = tmpDir.native(); env[OS_STR("TMP")] = tmpDir.native(); diff --git a/src/libutil-tests/main.cc b/src/libutil-tests/main.cc index cb2d3b852f9b..b1ec861bd2fe 100644 --- a/src/libutil-tests/main.cc +++ b/src/libutil-tests/main.cc @@ -18,6 +18,13 @@ static int spawnTrivialMain() return EXIT_SUCCESS; } +static int spawnWriteToStdoutAndStderr() +{ + std::cout << "hi" << std::endl; + std::cerr << "there" << std::endl; + return EXIT_SUCCESS; +} + #ifndef _WIN32 static int spawnTestForLeakedFDsMain() @@ -52,6 +59,8 @@ int main(int argc, char ** argv) if (argv1 == "__util_test_spawn_trivial") { return spawnTrivialMain(); + } else if (argv1 == "__util_test_spawn_write_to_stdout_and_stderr") { + return spawnWriteToStdoutAndStderr(); } else if (argv1 == "__util_test_spawn_leaked_fds") { #ifndef _WIN32 return spawnTestForLeakedFDsMain(); diff --git a/src/libutil-tests/processes.cc b/src/libutil-tests/processes.cc index 29cb4fd2dcfd..e07c9297671a 100644 --- a/src/libutil-tests/processes.cc +++ b/src/libutil-tests/processes.cc @@ -40,6 +40,36 @@ TEST(runProgram, worksTrivial) } } +TEST(runProgram2, mergeStderrToStdout) +{ + auto self = getSelfExe(); + ASSERT_TRUE(self); + + for (bool mergeStdoutToStderr : {false, true}) { + ::testing::internal::CaptureStdout(); + ::testing::internal::CaptureStderr(); + + auto [status, output] = runProgram({ + .spawnOptions = + { + .program = *self, + .args = {OS_STR("__util_test_spawn_write_to_stdout_and_stderr")}, + }, + .mergeStderrToStdout = mergeStdoutToStderr, + }); + + ASSERT_TRUE(statusOk(status)); + ASSERT_THAT(output, ::testing::HasSubstr("hi")); + ASSERT_EQ(::testing::internal::GetCapturedStdout(), ""); + auto capturedErr = ::testing::internal::GetCapturedStderr(); + if (mergeStdoutToStderr) + ASSERT_THAT(output, ::testing::HasSubstr("there")); + else + /* Otherwise stderr is inherited. */ + ASSERT_THAT(capturedErr, ::testing::HasSubstr("there")); + } +} + #ifdef _WIN32 # define NIX_EXECUTABLE_EXTENSION ".exe" /* FIXME: runProgram reports spawn errors as WinError, while other @@ -62,9 +92,9 @@ TEST(runProgram2, nonexistent) { ASSERT_THROW( { - runProgram2({ + runProgram2({{ .program = "/this/path/really/should/not/exist/for/real" NIX_EXECUTABLE_EXTENSION, - }); + }}); }, NIX_SPAWN_EXCEPTION); } @@ -85,12 +115,12 @@ TEST(runProgram2, leakedFDsAreClosed) enough if the read side is assigned to 3 (also the fd of the relocated pipe in the child on linux that gets dup3-ed into). */ ASSERT_NO_THROW(runProgram2({ - .program = *self, - .args = {"__util_test_spawn_leaked_fds"}, - .environment = OsStringMap{{ - "NIX_CHILD_FDS_SHOULD_BE_CLOSED", - fmt("%d,%d", readSide.get(), writeSide.get()), - }}, + {.program = *self, + .args = {"__util_test_spawn_leaked_fds"}, + .environment = OsStringMap{{ + "NIX_CHILD_FDS_SHOULD_BE_CLOSED", + fmt("%d,%d", readSide.get(), writeSide.get()), + }}}, })); } diff --git a/src/libutil-tests/spawn.cc b/src/libutil-tests/spawn.cc index db6502390c6d..51e981f6defc 100644 --- a/src/libutil-tests/spawn.cc +++ b/src/libutil-tests/spawn.cc @@ -6,10 +6,15 @@ namespace nix { +// TODO: Move into libutil-tests/processes.cc + TEST(SpawnTest, spawnEcho) { - auto output = - runProgram(RunOptions{.program = "cmd.exe", .args = {OS_STR("/C"), OS_STR("echo"), OS_STR("hello world")}}); + auto output = runProgram( + RunOptions{{ + .program = "cmd.exe", + .args = {OS_STR("/C"), OS_STR("echo"), OS_STR("hello world")}, + }}); ASSERT_EQ(output.first, 0); ASSERT_EQ(output.second, "\"hello world\"\r\n"); } diff --git a/src/libutil-tests/unix/file-descriptor.cc b/src/libutil-tests/unix/file-descriptor.cc index f969dbd57d9c..5a0d981d1a3b 100644 --- a/src/libutil-tests/unix/file-descriptor.cc +++ b/src/libutil-tests/unix/file-descriptor.cc @@ -5,6 +5,7 @@ #include "nix/util/processes.hh" #include +#include namespace nix { @@ -36,4 +37,33 @@ TEST(closeExtraFDs, works) ASSERT_TRUE(statusOk(pid.wait())); } +TEST(closeExtraFDs, keepExtra) +{ + using namespace nix::unix; + + Pipe pipe1, pipe2; + pipe1.create(); + pipe2.create(); + Pid pid = startProcess([&]() { + closeExtraFDs(std::to_array({pipe1.readSide.get(), pipe2.readSide.get()})); + + /* File descriptors should already be closed by `closeExtraFDs`. */ + for (int fd : {pipe1.writeSide.get(), pipe2.writeSide.get()}) { + if (::close(fd) == 0 || errno != EBADF) + _exit(1); + } + + /* readSide, stdin, stdout and stderr should not be closed. */ + for (int fd : {pipe1.readSide.get(), pipe2.readSide.get(), STDIN_FILENO, STDOUT_FILENO, STDERR_FILENO}) { + if (::close(fd) == -1) + _exit(2); + } + + _exit(0); + }); + + ASSERT_NE(pid_t(pid), -1); + ASSERT_TRUE(statusOk(pid.wait())); +} + } // namespace nix diff --git a/src/libutil/include/nix/util/file-descriptor.hh b/src/libutil/include/nix/util/file-descriptor.hh index e8bb8ff1938d..31edc5def6ed 100644 --- a/src/libutil/include/nix/util/file-descriptor.hh +++ b/src/libutil/include/nix/util/file-descriptor.hh @@ -331,6 +331,13 @@ namespace unix { */ void closeExtraFDs(); +/** + * Close all file descriptors except stdio fds (ie 0, 1, 2) and + * @p keepExtra. + * Good practice in child processes. + */ +void closeExtraFDs(std::span keepExtra); + /** * A useful primitive for asynchronous poll() loops to notify about some work * completing that gets polled alongside other file descriptors. diff --git a/src/libutil/include/nix/util/processes.hh b/src/libutil/include/nix/util/processes.hh index c77b455d708f..2854afa08933 100644 --- a/src/libutil/include/nix/util/processes.hh +++ b/src/libutil/include/nix/util/processes.hh @@ -139,7 +139,10 @@ std::string runProgram( const OsStrings & args = OsStrings(), bool isInteractive = false); -struct RunOptions +/** + * Options for creating a new process running the specified program. + */ +struct SpawnOptions { std::filesystem::path program; bool lookupPath = true; @@ -151,14 +154,58 @@ struct RunOptions #endif std::optional chdir; std::optional environment; + /** + * @brief (Linux-only) whether to have the process die when its parent (us) exists. + * + * Implemented on best-effort basis and ignored if not implemented. + */ + bool dieWithParent = true; +}; + +struct RunOptions +{ + SpawnOptions spawnOptions; Sink * standardOut = nullptr; bool mergeStderrToStdout = false; bool isInteractive = false; }; +struct FdRedirection +{ + Descriptor from, to; + +#ifdef _WIN32 + /* These are just dummy values to represent distinct file descriptors one might want + to redirect. On Windows we'll do equality checks on the "from"/"to" to implement + the same dup housekeeping semantics as on POSIX. */ + static inline const Descriptor stdInput = reinterpret_cast(static_cast(STD_INPUT_HANDLE)); + static inline const Descriptor stdOut = reinterpret_cast(static_cast(STD_OUTPUT_HANDLE)); + static inline const Descriptor stdError = reinterpret_cast(static_cast(STD_ERROR_HANDLE)); +#else + static constexpr Descriptor stdInput = STDIN_FILENO; + static constexpr Descriptor stdOut = STDOUT_FILENO; + static constexpr Descriptor stdError = STDERR_FILENO; +#endif +}; + // Output = error code + "standard out" output stream std::pair runProgram(RunOptions && options); +/** + * @brief Spawn a program as a subprocess. + * + * @return Pid of the spawned process. + * @throws SysError if fork() or vfork() fail in the parent, ExecError if + * housekeeping fails with status code 1. + * @todo Maybe align with posix_spawn and report 127 on housekeeping errors? + * + * @param fdr Unless specified, standard in, out and error are inherited. + * Redirections are applied in the same order in the child housekeeping. + * It's the callers responsibility to ensure that the redirected file descriptors + * are inheritable. + */ +Pid spawnProgram(const SpawnOptions & options, std::span fdr); + void runProgram2(const RunOptions & options); class ExecError final : public CloneableError diff --git a/src/libutil/linux/processes.cc b/src/libutil/linux/processes.cc index cd3f55aad175..67f77f14d4ea 100644 --- a/src/libutil/linux/processes.cc +++ b/src/libutil/linux/processes.cc @@ -40,9 +40,9 @@ struct ExecChildParams const char * chdir; char * const * environment; char * const * args; - bool mergeStderrToStdout; + const FdRedirection * fdr; + size_t fdrSize; bool lookupPath; - Descriptor stdoutFd; Descriptor errorPipe; bool setGid; gid_t gid; @@ -99,14 +99,6 @@ struct ExecChildParams auto dieWithErrno = [&die] [[noreturn]] (const char * msg) { die(errno, msg); }; - /* TODO: Do something with stdin if we don't want to keep it? */ - - if (params.stdoutFd != INVALID_DESCRIPTOR && ::dup2(params.stdoutFd, STDOUT_FILENO) == -1) - dieWithErrno("dupping stdout"); - - if (params.mergeStderrToStdout && ::dup2(STDOUT_FILENO, STDERR_FILENO) == -1) - dieWithErrno("cannot dup stdout into stderr"); - if (params.chdir && ::chdir(params.chdir) == -1) dieWithErrno("chdir failed"); @@ -144,13 +136,51 @@ struct ExecChildParams purposes we should dup (and mark as O_CLOEXEC) the error pipe descriptor. */ - static constexpr int relocatedErrorPipeFD = STDERR_FILENO + 1; + /* First, specify the redirections the caller requested. We need to be careful + not to trample the error pipe while doing so. We do something similar to musl + and just move the pipe out of the way if we happen to collide. We'll mark it as + CLOEXEC at end unconditionally in case we did have to move it. */ + + int maxTo = STDERR_FILENO; + bool movedErrorPipe = false; + + for (size_t i = 0; i < params.fdrSize; ++i) { + const auto & [from, to] = params.fdr[i]; + /* Move the pipe into an unoccupied slot if needed. We are the thread + group leader in this process so racy dup() is perfectly fine. */ + if (to == errorPipe) { + int newErrorPipe = ::dup(errorPipe); + if (newErrorPipe == -1) + dieWithErrno("moving the error pipe fd"); + ::close(errorPipe); + errorPipe = newErrorPipe; + movedErrorPipe = true; + } + + /* dup2 does nothing when from == to, but we still need to clear the CLOEXEC flag. */ + if (from == to) { + if (int prev = ::fcntl(from, F_GETFD); prev == -1 || ::fcntl(from, F_SETFD, prev & ~FD_CLOEXEC) == -1) + dieWithErrno("clearing FD_CLOEXEC"); + } else if (::dup2(from, to) == -1) + dieWithErrno("dup2"); + + /* Everything that's a redirection target needs to be kept. */ + maxTo = maxTo < to ? to : maxTo; + } + + /* Yes, this does mean that's it's not ideal to specify high-value + redirections. Please don't do that. */ + int relocatedErrorPipeFD = maxTo + 1; /* dup3 fails if oldfd == newfd, so skip that case. */ if (errorPipe != relocatedErrorPipeFD) { if (::dup3(errorPipe, relocatedErrorPipeFD, O_CLOEXEC) == -1) dieWithErrno("dupping error pipe"); errorPipe = relocatedErrorPipeFD; + } else if (movedErrorPipe) { + /* Still need to mark it CLOEXEC in case it had to be moved. */ + if (::fcntl(errorPipe, F_SETFD, FD_CLOEXEC) == -1) + dieWithErrno("making relocated error pipe O_CLOEXEC"); } #if HAVE_CLOSEFROM @@ -165,6 +195,23 @@ struct ExecChildParams ::syscall(SYS_close_range, relocatedErrorPipeFD + 1, ~0u, 0); #endif + /* Close everything in [3, maxTo) range that isn't supposed to be kept. maxTo + 1 + is the error pipe that's CLOEXEC and shouldn't be closed and maxTo is already + known to be kept. And everything > maxTo + 1 is already closed above. */ + for (int fd = STDERR_FILENO + 1; fd < maxTo; ++fd) { + bool kept = false; + /* Cheeky linear search. */ + for (size_t j = 0; j < params.fdrSize; ++j) { + if (params.fdr[j].to == fd) { + kept = true; + break; + } + } + + if (!kept) + ::close(fd); + } + /* Important! Calling syscalls directly and not libc functions because of a discrepancy in POSIX specification (i.e. POSIX setgid/setuid has to apply to all threads, while the syscall only applies to a task). @@ -263,17 +310,8 @@ static Strings prepareEnvironmentStrings(const StringMap & environment) } // namespace -/* TODO: Factor this out into a `launchProgram` that returns a pid. That would be - much more useful in more places. */ -void runProgram2(const RunOptions & options) +Pid spawnProgram(const SpawnOptions & options, std::span fdr) { - checkInterrupt(); - - /* Create a pipe. */ - Pipe out; - if (options.standardOut) - out.create(); - /* Pipe that the child reports errors through. */ Pipe childErrorPipe; childErrorPipe.create(); @@ -292,9 +330,9 @@ void runProgram2(const RunOptions & options) .chdir = options.chdir ? options.chdir->c_str() : nullptr, .environment = options.environment ? env.data() : environ, .args = args.data(), - .mergeStderrToStdout = options.mergeStderrToStdout, + .fdr = fdr.data(), + .fdrSize = fdr.size(), .lookupPath = options.lookupPath, - .stdoutFd = options.standardOut ? out.writeSide.get() : INVALID_DESCRIPTOR, .errorPipe = childErrorPipe.writeSide.get(), .setGid = options.gid.has_value(), /* The default is not used, but a bit sketchy to leave zero initialised so "nobody". */ @@ -302,11 +340,9 @@ void runProgram2(const RunOptions & options) .setUid = options.uid.has_value(), /* The default is not used, but a bit sketchy to leave zero initialised so "nobody". */ .uid = options.uid.value_or(65534), - .dieWithParent = true, /* TODO: Maybe we might want to expose this in RunOptions? */ + .dieWithParent = options.dieWithParent, }; - auto suspension = logger->suspendIf(options.isInteractive); - const auto savedErrno = errno; /* Fork. */ @@ -320,7 +356,6 @@ void runProgram2(const RunOptions & options) if (pid == -1) throw SysError(forkErrno, "unable to vfork"); - out.writeSide.close(); childErrorPipe.writeSide.close(); StringSink childErrorSink; @@ -349,13 +384,7 @@ void runProgram2(const RunOptions & options) throw std::move(execErr); } - if (options.standardOut) - drainFD(out.readSide.get(), *options.standardOut); - - /* Wait for the child to finish. */ - int status = pid.wait(); - if (status) - throw ExecError(status, "program %1% %2%", PathFmt(options.program), statusToString(status)); + return pid; } } // namespace nix diff --git a/src/libutil/processes.cc b/src/libutil/processes.cc index 0211557034cf..68c54f9c0744 100644 --- a/src/libutil/processes.cc +++ b/src/libutil/processes.cc @@ -1,5 +1,6 @@ #include "nix/util/processes.hh" #include "nix/util/serialise.hh" +#include "nix/util/signals.hh" namespace nix { @@ -27,4 +28,63 @@ std::pair runProgram(RunOptions && options) return {status, std::move(sink.s)}; } +std::string runProgram(std::filesystem::path program, bool lookupPath, const OsStrings & args, bool isInteractive) +{ + auto res = runProgram( + RunOptions{ + .spawnOptions = + { + .program = program, + .lookupPath = lookupPath, + .args = args, + }, + .isInteractive = isInteractive, + }); + + if (!statusOk(res.first)) + throw ExecError(res.first, "program %s %s", PathFmt(program), statusToString(res.first)); + + return res.second; +} + +void runProgram2(const RunOptions & runOptions) +{ + checkInterrupt(); + + const auto & options = runOptions.spawnOptions; + + /* Create a pipe and set up redirections for the child. Stdin is inherited + always. Unless standardOut is specified stdout is inherited too. stderr + is redirected to stdout if mergeStderrToStdout is specified which might + end up redirected to the stdout pipe. */ + Pipe out; + std::vector fdr; + + if (runOptions.standardOut) { + out.create(); + fdr.push_back({.from = out.writeSide.get(), .to = FdRedirection::stdOut}); +#ifdef _WIN32 + windows::setHandleInheritability(out.readSide.get(), false); +#endif + } + + if (runOptions.mergeStderrToStdout) + fdr.push_back({.from = FdRedirection::stdOut, .to = FdRedirection::stdError}); + + auto suspension = logger->suspendIf(runOptions.isInteractive); + + /* Fork. */ + Pid pid = spawnProgram(options, fdr); + assert(pid); /* spawnProgram forking errors are always exceptions. */ + out.writeSide.close(); + + if (runOptions.standardOut) + drainFD(out.readSide.get(), *runOptions.standardOut); + + /* Wait for the child to finish. */ + int status = pid.wait(); + if (status) + throw ExecError(status, "program %1% %2%", PathFmt(options.program), statusToString(status)); +} + } // namespace nix diff --git a/src/libutil/unix/file-descriptor.cc b/src/libutil/unix/file-descriptor.cc index ac4053a54aa0..523834b4cc7d 100644 --- a/src/libutil/unix/file-descriptor.cc +++ b/src/libutil/unix/file-descriptor.cc @@ -5,6 +5,7 @@ #include #include #include +#include #include #include "util-unix-config-private.hh" @@ -105,16 +106,39 @@ static int unix_close_range(unsigned int first, unsigned int last, int flags) void unix::closeExtraFDs() { - constexpr int MAX_KEPT_FD = 2; - static_assert(std::max({STDIN_FILENO, STDOUT_FILENO, STDERR_FILENO}) == MAX_KEPT_FD); + closeExtraFDs(/*keepExtra=*/{}); +} + +void unix::closeExtraFDs(std::span keepExtra) +{ + /* Just some sanity compile-time checks. */ + static constexpr Descriptor maxKeptStdio = 2; + static_assert(std::max({STDIN_FILENO, STDOUT_FILENO, STDERR_FILENO}) == maxKeptStdio); + + auto maxKeptIt = std::max_element(keepExtra.begin(), keepExtra.end()); + Descriptor maxKept = STDERR_FILENO; + + if (maxKeptIt != keepExtra.end()) + maxKept = std::max(maxKept, *maxKeptIt); + + auto finish = [&] -> void { + /* We closed everything in one go already. */ + if (keepExtra.empty()) + return; + + /* Otherwise, clean up remaining holes. */ + for (Descriptor fd = maxKeptStdio + 1; fd < maxKept; ++fd) + if (!std::ranges::contains(keepExtra, fd)) + ::close(fd); /* ignore result */ + }; #if defined(__linux__) || defined(__FreeBSD__) // first try to close_range everything we don't care about. if this // returns an error with these parameters we're running on a kernel // that does not implement close_range (i.e. pre 5.9) and fall back // to the old method. we should remove that though, in some future. - if (unix_close_range(MAX_KEPT_FD + 1, ~0U, 0) == 0) { - return; + if (unix_close_range(maxKept + 1, ~0U, 0) == 0) { + return finish(); } #endif @@ -123,22 +147,24 @@ void unix::closeExtraFDs() for (auto & s : DirectoryIterator{"/proc/self/fd"}) { checkInterrupt(); auto fd = std::stoi(s.path().filename()); - if (fd > MAX_KEPT_FD) { + if (fd > maxKept) { debug("closing leaked FD %d", fd); close(fd); } } - return; + return finish(); } catch (SysError &) { } #endif - int maxFD = 0; + Descriptor maxFD = 0; #if HAVE_SYSCONF maxFD = sysconf(_SC_OPEN_MAX); #endif - for (int fd = MAX_KEPT_FD + 1; fd < maxFD; ++fd) - close(fd); /* ignore result */ + for (Descriptor fd = maxKept + 1; fd < maxFD; ++fd) + ::close(fd); /* ignore result */ + + finish(); } void closeOnExec(Descriptor fd) diff --git a/src/libutil/unix/processes.cc b/src/libutil/unix/processes.cc index 909f4d8e766d..704e58c3b668 100644 --- a/src/libutil/unix/processes.cc +++ b/src/libutil/unix/processes.cc @@ -296,48 +296,26 @@ pid_t startProcess(fun processMain, const ProcessOptions & options) return pid; } -std::string runProgram(std::filesystem::path program, bool lookupPath, const OsStrings & args, bool isInteractive) -{ - auto res = runProgram( - RunOptions{ - .program = program, - .lookupPath = lookupPath, - .args = args, - .isInteractive = isInteractive, - }); - - if (!statusOk(res.first)) - throw ExecError(res.first, "program %s %s", PathFmt(program), statusToString(res.first)); - - return res.second; -} - #ifndef __linux__ -void runProgram2(const RunOptions & options) +Pid spawnProgram(const SpawnOptions & options, std::span fdr) { - checkInterrupt(); - - /* Create a pipe. */ - Pipe out; - if (options.standardOut) - out.create(); - - ProcessOptions processOptions; - - auto suspension = logger->suspendIf(options.isInteractive); - - /* Fork. */ - Pid pid = startProcess( + return startProcess( [&] { if (options.environment) replaceEnv(*options.environment); - if (options.standardOut && dup2(out.writeSide.get(), STDOUT_FILENO) == -1) - throw SysError("dupping stdout"); - if (options.mergeStderrToStdout) - if (dup2(STDOUT_FILENO, STDERR_FILENO) == -1) - throw SysError("cannot dup stdout into stderr"); + std::vector keepExtraFDs; + for (const auto & [from, to] : fdr) { + /* dup2 does nothing when from == to, but we still need to clear the CLOEXEC flag. */ + if (from == to) { + if (int prev = ::fcntl(from, F_GETFD); + prev == -1 || ::fcntl(from, F_SETFD, prev & ~FD_CLOEXEC) == -1) + throw SysError("clearing FD_CLOEXEC"); + } else if (::dup2(from, to) == -1) + throw SysError("dupping fd %d -> %d", from, to); + keepExtraFDs.push_back(to); + } if (options.chdir && chdir((*options.chdir).c_str()) == -1) throw SysError("chdir failed"); if (options.gid && setgid(*options.gid) == -1) @@ -359,7 +337,7 @@ void runProgram2(const RunOptions & options) the FDs before or after restoreProcessContext(), but on Linux it's crucial that it happens *after* restoreProcessContext() call because that re-enters the saved mountns. */ - unix::closeExtraFDs(); + unix::closeExtraFDs(keepExtraFDs); if (options.lookupPath) execvp(options.program.c_str(), stringsToCharPtrs(args_).data()); @@ -370,20 +348,13 @@ void runProgram2(const RunOptions & options) throw SysError("executing %s", PathFmt(options.program)); }, - processOptions); - - out.writeSide.close(); - - if (options.standardOut) - drainFD(out.readSide.get(), *options.standardOut); - - /* Wait for the child to finish. */ - int status = pid.wait(); - if (status) - throw ExecError(status, "program %1% %2%", PathFmt(options.program), statusToString(status)); + { + /* TODO: This isn't really implemented on non-Linux. */ + .dieWithParent = options.dieWithParent, + }); } -#endif // __linux__ +#endif ////////////////////////////////////////////////////////////////////// diff --git a/src/libutil/windows/processes.cc b/src/libutil/windows/processes.cc index 0ead2c9faf44..9ecee244a74d 100644 --- a/src/libutil/windows/processes.cc +++ b/src/libutil/windows/processes.cc @@ -93,60 +93,6 @@ int Pid::wait(bool allowInterrupts) return exitCode; } -// TODO: Merge this with Unix's runProgram since it's identical logic. -std::string runProgram(std::filesystem::path program, bool lookupPath, const OsStrings & args, bool isInteractive) -{ - auto res = runProgram( - RunOptions{ - .program = program, - .lookupPath = lookupPath, - .args = args, - .isInteractive = isInteractive, - }); - - if (!statusOk(res.first)) - throw ExecError(res.first, "program %s %s", PathFmt(program), statusToString(res.first)); - - return res.second; -} - -std::optional getProgramInterpreter(const std::filesystem::path & program) -{ - // These extensions are automatically handled by Windows and don't require an interpreter. - static constexpr const char * exts[] = {".exe", ".cmd", ".bat"}; - for (const auto ext : exts) { - if (hasSuffix(program.string(), ext)) { - return {}; - } - } - // TODO: Open file and read the shebang - throw UnimplementedError("getProgramInterpreter unimplemented"); -} - -AutoCloseFD nullFD() -{ - using namespace nix::windows; - - // Create null handle to discard reads / writes - // https://stackoverflow.com/a/25609668 - // https://github.com/nix-windows/nix/blob/windows-meson/src/libutil/util.cc#L2228 - AutoCloseFD nul = CreateFileW( - L"NUL", - GENERIC_READ | GENERIC_WRITE, - // We don't care who reads / writes / deletes this file since it's NUL anyways - FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, - NULL, - OPEN_EXISTING, - 0, - NULL); - if (!nul) { - throw WinError("Couldn't open NUL device"); - } - // Let this handle be inheritable by child processes - setHandleInheritability(nul.get(), true); - return nul; -} - // Adapted from // https://blogs.msdn.microsoft.com/twistylittlepassagesallalike/2011/04/23/everyone-quotes-command-line-arguments-the-wrong-way/ OsString windowsEscape(const OsString & str, bool cmd) @@ -193,26 +139,40 @@ OsString windowsEscape(const OsString & str, bool cmd) return buffer + L'"'; } -Pid spawnProcess(const std::filesystem::path & realProgram, const RunOptions & options, Pipe & out) +Pid spawnProgram(const SpawnOptions & options, std::span fdr) { using namespace nix::windows; - // Setup pipes. - if (options.standardOut) { - // Don't inherit the read end of the output pipe - setHandleInheritability(out.readSide.get(), false); - } else { - out.writeSide = nullFD(); - } - - AutoCloseFD in = nullFD(); - STARTUPINFOW startInfo = {0}; startInfo.cb = sizeof(startInfo); startInfo.dwFlags = STARTF_USESTDHANDLES; - startInfo.hStdInput = in.get(); - startInfo.hStdOutput = out.writeSide.get(); - startInfo.hStdError = out.writeSide.get(); + + startInfo.hStdInput = getStandardInput(); + startInfo.hStdOutput = getStandardOutput(); + startInfo.hStdError = getStandardError(); + + /* Redirections are applied in sequence, as one would expect with posix_spawn. + Thus, we need to look up a handle we might have previously overwritten. + A bit ugly, but that's how process spawning works on unix and what callers + expect, so not much we can do about the statefulness. */ + auto lookupHandleHousekeeping = [&](HANDLE handle) -> HANDLE * { + if (handle == FdRedirection::stdInput) + return &startInfo.hStdInput; + if (handle == FdRedirection::stdOut) + return &startInfo.hStdOutput; + if (handle == FdRedirection::stdError) + return &startInfo.hStdError; + return nullptr; + }; + + for (const auto & [from, to] : fdr) { + HANDLE * to2 = lookupHandleHousekeeping(to); + HANDLE * fromPtr = lookupHandleHousekeeping(from); + HANDLE from2 = fromPtr ? *fromPtr : from; + if (!to2) + throw UnimplementedError("redirecting arbitrary handles isn't really possible on windows"); + *to2 = from2; + } auto env = getEnvOs(); @@ -228,7 +188,7 @@ Pid spawnProcess(const std::filesystem::path & realProgram, const RunOptions & o envline += (envVar.first + L'=' + envVar.second + L'\0'); } - OsString cmdline = windowsEscape(realProgram.native(), false); + OsString cmdline = windowsEscape(options.program.native(), false); for (const auto & arg : options.args) { // TODO: This isn't the right way to escape windows command // See https://learn.microsoft.com/en-us/windows/win32/api/shellapi/nf-shellapi-commandlinetoargvw @@ -277,37 +237,6 @@ Pid spawnProcess(const std::filesystem::path & realProgram, const RunOptions & o return process; } -void runProgram2(const RunOptions & options) -{ - checkInterrupt(); - - /* Create a pipe. */ - Pipe out; - // TODO: I copied this from unix but this is handled again in spawnProcess, so might be weird to split it up like - // this - if (options.standardOut) - out.create(); - - std::filesystem::path realProgram = options.program; - // TODO: Implement shebang / program interpreter lookup on Windows - auto interpreter = getProgramInterpreter(realProgram); - - auto suspension = logger->suspendIf(options.isInteractive); - - Pid pid = spawnProcess(interpreter.has_value() ? *interpreter : realProgram, options, out); - - // TODO: This is identical to unix, deduplicate? - out.writeSide.close(); - - if (options.standardOut) - drainFD(out.readSide.get(), *options.standardOut); - - /* Wait for the child to finish. */ - int status = pid.wait(); - if (status) - throw ExecError(status, "program %1% %2%", PathFmt(options.program), statusToString(status)); -} - std::string statusToString(int status) { if (status != 0) diff --git a/src/nix/edit.cc b/src/nix/edit.cc index 94210f30ed9b..eecf739fd30e 100644 --- a/src/nix/edit.cc +++ b/src/nix/edit.cc @@ -50,9 +50,12 @@ struct CmdEdit : InstallableValueCommand runProgram2( RunOptions{ - .program = program, - .lookupPath = true, - .args = std::move(args), + .spawnOptions = + { + .program = program, + .lookupPath = true, + .args = std::move(args), + }, .isInteractive = true, }); } diff --git a/src/nix/self-exe.cc b/src/nix/self-exe.cc index 3506b91b68ea..7067eb5ff6c6 100644 --- a/src/nix/self-exe.cc +++ b/src/nix/self-exe.cc @@ -73,11 +73,14 @@ void runNixBin2( try { runProgram2( RunOptions{ - .program = *selfProgramPath, - .lookupPath = false, - .args = args, - .argv0 = binaryName, - .environment = std::move(environment), + .spawnOptions = + { + .program = *selfProgramPath, + .lookupPath = false, + .args = args, + .argv0 = binaryName, + .environment = std::move(environment), + }, .standardOut = standardOut, .isInteractive = isInteractive, });