libutil: fix startProgram Linux definition and descriptor handling by awsmadi · Pull Request #16454 · NixOS/nix

awsmadi · 2026-09-11T13:02:10Z

Where this comes from

The first commit here is @lisanna-dettwyler's, taken from #15959 and rebased onto master. Master is
596 commits past that branch point, but the two libutil files apply with git apply --3way and no
conflicts. The pasta feature itself is not included, only the startProgram split. I am not trying
to pre-empt #15959; the refactor is independently useful and I found four defects while trying to use
it.

The four defects

startProgram is declared unconditionally but defined only inside #ifndef __linux__, so it does
not link on Linux. Its caps block sits #ifdef __linux__ nested inside that #ifndef __linux__, so
it never compiles, meaning pasta's CAP_NET_ADMIN is never granted, and pasta is Linux-only.
linux/processes.cc handles neither caps nor redirections. And closeExtraFDs() runs after the
dup2 loop, closing every descriptor Redirection exists to wire.

Fixes

startProgram implemented natively on the Linux side, the caps blocks un-nested, and the descriptor
sweep taught to spare redirection targets via a new closeExtraFDs(std::span<const Descriptor>).
validateRedirections rejects fd 3 on either end, since Linux relocates its error pipe there and
calls closefrom(4).

One change to your struct that needs your agreement, @lisanna-dettwyler: the fields are now
sourceFd/targetFd, so from is the source. The original had dup2(to, from) while its own error
message read "dupping fd %i to %i" with the arguments the other way round. There were no callers, so I
matched the names, but it is your design.

Verification

813 to 826 tests. libstore builds against in-tree libutil. fds 4 and 5 deliver on the fixed tree and
deliver 0 bytes on the original. One call site converted (ssh.cc startMaster); the other three
need standardInFd and standardErrFd, which do not exist yet.

Extracted from NixOS#15959 (c8f889f "libstore: use pasta for FODs if available") and rebased onto master, which is 596 commits past that branch point. Only the two libutil process files are taken; the pasta feature itself is not included. startProgram returns a Pid and takes redirections through RunOptions::Redirection, so callers that today pass an arbitrary lambda to startProcess can express stdin/stdout/stderr and extra-fd wiring declaratively instead. This is what NixOS#16168 bullet 2 asks for. Both files applied with git apply --3way against master with no conflicts. The Assisted-by trailer is deliberately absent: the code in this commit is Lisanna Dettwyler's, taken unmodified. Only the extraction and the rebase onto master were tool-assisted, and those changed no bytes of the diff.

Seven of RunOptions' fields had no test, so a refactor of the spawn path could drop one and stay green. Adds behavioural tests that observe the child rather than the options struct: - non-zero exit reported via ExecError, asserted through statusOk since the field is a raw wait status rather than an exit code - standardOut captures stdout and not stderr - mergeStderrToStdout captures both - environment replaces the parent environment rather than extending it - environment passes a given variable through - chdir, with a positive control that the test is not already running in the target directory - argv0 (Unix only, where the field exists) Extends the existing self-exec harness in main.cc with five child entry points rather than adding a second mechanism. Assisted-by: Claude Code (claude-opus-5)

…tions startProgram as introduced could not work on any platform. Four separate reasons, none of which surfaced because nothing calls it yet: the symbol appears three times in NixOS#15959 and never outside those two files. 1. It was declared unconditionally in processes.hh but defined only inside the `#ifndef __linux__` region of unix/processes.cc, so every Linux caller was a link error. Linux has its own runProgram2 built on the vfork helper from NixOS#16152, which the split had not touched. 2. The two `#ifdef __linux__` blocks implementing `caps` were nested inside that `#ifndef __linux__`, so they could never compile. Capabilities are a Linux-only feature, so the code was unreachable everywhere. 3. linux/processes.cc mentioned neither `redirections` nor `caps`, so both fields were silently ignored on the one platform that has them. 4. Even off Linux the redirections were undone before taking effect. closeExtraFDs() closes every descriptor above stderr and ran after the dup2 loop and before the exec; since stdin/stdout/stderr have their own RunOptions fields, descriptors above stderr are the only thing Redirection exists to wire. Additionally the dup2 operands were the wrong way round relative to the error message beside them, which read "dupping fd <from> to <to>" while the call duplicated `to` onto `from`. With no callers to be compatible with, the documented reading wins: `from` is the source, `to` is the number it gets in the child. Implemented on Linux inside doExecChild, which is post-vfork and so may not allocate or throw: the params struct carries a pointer and a count rather than containers, capability indices are range-checked in the parent, and the descriptor sweep now spares the redirection targets instead of using a fixed threshold. The same exemption is available to the fork-based path through a new closeExtraFDs(std::span<const Descriptor>) overload. Conflicts a caller could not otherwise detect are rejected in the parent by validateRedirections, because the child applies the duplications in order with no bookkeeping and, on Linux, cannot report a usage error at all: a target that is a standard stream, a target that is another redirection's source, and either end colliding with the descriptor the vfork child reserves for its error pipe. This also discharges the TODO that stood above linux/processes.cc's runProgram2 asking for it to be factored into something that returns a pid. Relevant to NixOS#16168 bullet 2 and to NixOS#16382. Assisted-by: Claude Code (claude-opus-5)

Four tests, in a suite where redirections previously had none. Two are behavioural and observe the child rather than the options struct, because the ordering between wiring the descriptors and sweeping the rest is the entire behaviour and is invisible from the parent: - redirectionsReachTheChild wires two pipes onto fds 4 and 5 specifically, which is what hook-instance.cc does, and reads the markers back out. - redirectionsDoNotKeepUnrelatedFDsOpen checks a descriptor that is neither a source nor a target is still closed, so that exempting the targets cannot quietly become exempting everything above stderr. The other two pin the parent-side rejections, which exist because the child cannot report a usage error after vfork. The sources are relocated to descriptors 30 and 31 before use. Without that the test's outcome depends on what ::pipe happens to return: if it hands back 4 or 5, a source coincides with a target and validateRedirections correctly refuses, turning a real pass into a spurious failure. Assisted-by: Claude Code (claude-opus-5)

The original defect existed because `from`/`to` is ambiguous: the implementation duplicated `to` onto `from` while the error message beside it said "dupping fd <from> to <to>", so the code and its own diagnostic disagreed about which end was the source. Fixing the operand order left the ambiguity that produced it in place. Naming the fields for their roles removes it. There are no callers anywhere in the tree, so this costs nothing now and would cost a rename-with-callers later. Also closes a gap in the redirection test. It previously used targets 4 and 5 with an unrelated descriptor at 25, above both. The child sweeps descriptors in two stages -- a range close above the highest target, then an individual walk below it -- and a spare above the highest target only ever exercises the first, so the walk was covered by nothing. The test now uses targets 4 and 9 with spares at 6 and 25, one inside the target range and one above it. Verified by neutering the walk: the child then fails its fd-6 check specifically, and passes again when it is restored. Assisted-by: Claude Code (claude-opus-5)

…WithParent `startProgram` could only send the child's stdout into a `Sink`, via `standardOut`. Callers that already own the write end of a pipe want the descriptor wired directly, and `Redirection` deliberately refuses to express that because its `targetFd` may not be a standard stream. Such a caller had to fall back to `startProcess` and a hand-written `dup2`. `dieWithParent` was hardcoded to `true` in both implementations. `linux/processes.cc` carried a `TODO` asking for exactly this to be exposed, and `ProcessOptions` has always had the field, so a caller needing a child that outlives it -- an `ssh -M` control master, say -- also had to fall back to `startProcess`. Adds `RunOptions::standardOutFd` and `RunOptions::dieWithParent`, wired in the Linux and non-Linux paths. `validateRedirections` rejects `standardOut` and `standardOutFd` together, since the child has one stdout and honouring either silently would discard the other's intent. Two tests, each shown failing with the mechanism neutered: ignoring `standardOutFd` leaves the pipe empty, and dropping the check throws `ExecError` from the exec instead of `UsageError` from validation. Also reformats these files; `clang-format` was already failing on four lines here before this change. Assisted-by: Claude Code (claude-opus-5)

`SSHMaster::startMaster` spawned `ssh -M -N` through `startProcess` with a lambda that restored the process context, closed the read end of its pipe, duplicated the write end onto stdout, assembled argv, and called `execvpe`. All of it is now declared instead of run by hand. `restoreProcessContext()` is what `startProgram`'s own child already does, `close(out.readSide)` is subsumed by its descriptor sweep, the stdout duplication is `standardOutFd`, and `dieWithParent = false` replaces the `ProcessOptions` field. This is the shape NixOS#16168 asks for in its second bullet, and the reason NixOS#16382 wants a spawn that is not lambda-based. `createSSHEnv` is split so the environment is available as a map for `RunOptions::environment` as well as flattened for the remaining `execvpe` caller; the flattening now goes through the map, so there is one source of truth. Assisted-by: Claude Code (claude-opus-5)

xokdvium · 2026-09-11T14:54:13Z

-/* 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 startProgram(const RunOptions & options, std::shared_ptr<Pipe> out)


startProgram really shouldn't take a RunOptions. A subset - yes, but some fields are meaningless there.

For example, isInteractive is meaningless there. I had a split commit Spawn/RunOptions which made it less insane.

xokdvium · 2026-09-11T14:57:23Z

+    int maxKeep = MAX_KEPT_FD;
+    for (auto fd : keep)
+        maxKeep = std::max(maxKeep, static_cast<int>(fd));


ranges::max_element or something?

xokdvium · 2026-09-11T14:58:36Z

    if (const auto & errorContent = childErrorSink.s; errorContent.size()) {
        int status = pid.wait();

        auto execErr = ExecError(status, "could not start program %1%", PathFmt(options.program));


Now this should no longer be an ExecError, but a ProcessSpawnError or something. And to keep back-compat that could be wrapped in runProgram2 with a FIXME to fix the callers.

xokdvium · 2026-09-11T14:58:48Z

+        return;
+    }
+
+    auto kept = [&](int fd) { return std::find(keep.begin(), keep.end(), fd) != keep.end(); };


ranges::contains?

xokdvium · 2026-09-11T15:00:12Z

+        /* Kept in step with `relocatedErrorPipeFD` in `linux/processes.cc`. The
+           vfork child moves its error pipe there before touching anything else,
+           so that number is unusable as either end of a redirection: as a target
+           it would overwrite the pipe, and as a source it would itself be
+           overwritten by the relocation. */
+        if (redirection.targetFd == STDERR_FILENO + 1 || redirection.sourceFd == STDERR_FILENO + 1)
+            throw UsageError(
+                "fd %i cannot take part in a redirection; it is reserved for the child's error-reporting pipe",
+                STDERR_FILENO + 1);


The more sane approach would be to relocate the error pipe instead of applying an arbitrary restriction here.

xokdvium · 2026-09-11T22:33:08Z

libutil: fix startProgram Linux definition and descriptor handling

Also description seems very wrong, seeing how nothing on master is broken.

xokdvium · 2026-09-21T23:50:37Z

I've implemented something along these lines in #16504. Hopefully that will unblock more things. I haven't yet migrated call-sites in there but it should be pretty trivial now.

lisanna-dettwyler and others added 7 commits September 2, 2026 18:26

awsmadi requested review from Ericson2314 and edolstra as code owners September 11, 2026 13:02

xokdvium reviewed Sep 11, 2026

View reviewed changes

xokdvium mentioned this pull request Sep 21, 2026

libutil: Expose spawnProgram and implement fd redirections #16504

Merged

Navigation Menu

Repository navigation

libutil: fix startProgram Linux definition and descriptor handling - #16454

libutil: fix startProgram Linux definition and descriptor handling#16454
awsmadi wants to merge 7 commits into
NixOS:masterfrom
awsmadi:feat/start-program-callsites-2

awsmadi commented Sep 11, 2026

Uh oh!

xokdvium Sep 11, 2026

Uh oh!

xokdvium Sep 11, 2026

Uh oh!

xokdvium Sep 11, 2026

Uh oh!

xokdvium Sep 11, 2026

Uh oh!

xokdvium Sep 11, 2026

Uh oh!

xokdvium Sep 11, 2026

Uh oh!

xokdvium commented Sep 11, 2026

Uh oh!

xokdvium commented Sep 21, 2026

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

awsmadi commented Sep 11, 2026

Where this comes from

The four defects

Fixes

Verification

Uh oh!

xokdvium Sep 11, 2026

Choose a reason for hiding this comment

Uh oh!

xokdvium Sep 11, 2026

Choose a reason for hiding this comment

Uh oh!

xokdvium Sep 11, 2026

Choose a reason for hiding this comment

Uh oh!

xokdvium Sep 11, 2026

Choose a reason for hiding this comment

Uh oh!

xokdvium Sep 11, 2026

Choose a reason for hiding this comment

Uh oh!

xokdvium Sep 11, 2026

Choose a reason for hiding this comment

Uh oh!

xokdvium commented Sep 11, 2026

Uh oh!

xokdvium commented Sep 21, 2026

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants