Pin engine-teardown signalling with a pidfd, re-verify before kill (refs #298) - #303
Merged
Conversation
…efs #298) engine/reap.go signalOwned scanned /proc for processes whose command line names this run's socket, then syscall.Kill(pid) each by number. Foreign-pid case: between the scan and the kill the number can be reaped and handed to an unrelated process, which snug then SIGKILLs -- the reuse TOCTOU #294 removed from the orphan sweep, still live here. signalOwned now pins each discovered pid with unix.PidfdOpen, re-reads the cmdline through the pinned number to confirm it still names the engine, and only then unix.PidfdSendSignal. The pidfd pins the task the number named at open time, so the signal can never land on a later reuse; the re-read drops a number the scan matched but that no longer names us (recycled, or an engine that exited between scan and signal). Returns the pids actually signalled. The pids are HOST-namespace: ownedPIDs reads the host /proc and matches the socket path in a host cmdline, so pidfd_open (caller's namespace) refers to the same task. It never sees the engine's own pids, numbered in its own namespace (#167). cmdlineNamesPath factored out and shared with ownedPIDs; reading /proc/<pid>/cmdline for DATA by number stays -- the rule is about signalling. TestSignalOwnedReverifiesThroughAPidfdBeforeKilling: a live process not naming the socket, handed to signalPinned by pid, must survive (the reuse case); mutation-checked (dropping the re-verify kills it). Positive control: a process that names the socket is signalled and dies from SIGKILL. go-implementer.md gets the standing rule: pin with a pidfd to signal or track a process; a bare numeric pid is only safe for your own not-yet-waited child, and only ever safe to READ /proc for data. Namespace caveat (#167) noted. Own-child cmd.Process.Kill() sites left as-is.
This was referenced Aug 22, 2026
vyskocilm
added a commit
that referenced
this pull request
Aug 22, 2026
…reen (refs #345, refs #348, refs #31, refs #344) (#362) * Re-derive three teardown justifications, and sanitize a cmdline on screen (refs #345, refs #348, refs #31, refs #344) Four comments that describe something other than what the code does, plus the half of #344 that needs no decision. #344's MATCHER is NOT fixed here — see below. #345 — orphansweep.go claimed "while the fd is held the kernel cannot recycle that pid number, so there is no residual pid-reuse window at all". MEASURED false on this kernel, in a fresh user+pid namespace writing ns_last_pid: pinned pid = 7 (reaped, pidfd still open) fdinfo says = Pid: -1 NSpid: -1 next child got pid = 7 (collision=true) signal through pin = no such process new occupant alive = true The NUMBER recycles. What the pin guarantees is the other half and it is the half the sweep needs: the pidfd refers to the task it opened, dies with it, and returns ESRCH rather than reaching the new occupant (#303, confirmed by the same run). That is what makes the kill fail CLOSED. The comment now says so, and says what follows: anything reading /proc/<pid> BY NUMBER after the open may be describing a different task, which is what the starttime and namespace-inode checks are for. The false version was the licence a later edit needs to act on the number inside that window. #348 — reap.go justified identity-by-path with the WRAPPER case: a podman forwarding to the host, engine parented to the host's systemd. Closed since Tier C, two independent ways. The argv names guest paths only — Spec resolves --root, --runroot and the socket through guestPath, which refuses a path no graft exposes — and those resolve only inside the derived mount namespace, so an engine exec'd on the host cannot open its store or bind its socket. Preflight P1 refuses the shim before that, by name, measured on this host. Path identity stays, for the reason that holds NOW: it verifies the cascade, and a recorded pid is not an option because libpod's are numbered in the engine's own namespace (#167). A wrapper re-execing INSIDE the derived view is still possible and is not that case — it is a descendant in the engine's pid namespace, so the cascade covers it. TestEngineArgvNamesOnlyGuestPaths makes the first half checkable: every path-shaped argv element is under /snug, and spec.Sock is still the HOST path because P1's waitForSocket and the proxy dial it. Positive control asserts the argv carries at least three paths, so it cannot pass on a Spec that stopped resolving. #31 — inengine.go described step 4 as "a plain private COPY of the host tree, deliberately NOT derived from the resolved Policy — that is Tier C's later pieces", contradicted by its own code 210 lines below: setns into the sandbox's mount namespace, unshare, MS_REC|MS_PRIVATE, then move_mount every graft. Step 4 is now the real four calls in order (4a-4d), states that p.Grafts is their author (invariant 6), and keeps the distinction the stale text got right: the view is not the container bind filter, and neither may be read as the other. Same statement at enginefork.go's #146 inventory — "no derived mount VIEW, no grafts — those are issue #125's later pieces" — now says C0 is the PRECONDITION for the view rather than a piece that shipped without it. #344, PARTIAL. describe() rendered a matched process's command line with only NUL replaced. A command line is not snug's text, so an ESC there erases the line snug printed above it; it goes through policy.VisibleText now. TestNoSnugScreenEmitsARawControlCharacter drives the --dry-run screen and does not reach internal/engine, so TestDescribeSanitisesACommandLine does, with a live decoy. Mutation observed: without the sanitiser, "describe emitted '\x1b' raw". That test's own two false starts are worth carrying, both being harness traps #344 names: `/bin/sleep "30<ESC>..."` rejects the duration and the decoy becomes a ZOMBIE, whose cmdline reads back empty; and a single-command `sh -c` is exec-optimised and loses the crafted argv. The working decoy is `sh -c 'sleep 30; :' <argv>`, and the empty-cmdline window (#318) is polled past rather than raced. NOT FIXED: paths() still returns the host socket while the engine's cmdline names the guest one, so stopLocked's step 3 remains a check that cannot fail. Handed back — correcting the matcher changes what step 3 verifies and when, because stopLocked runs while the engine is deliberately ALIVE and exclude holds no engine pid. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Fix reapidentity_test's Spec call for #360's fifth parameter (refs #345, refs #307) PR #360 (7d4a351) gave Engine.Spec a fifth parameter *SignaturePolicy after this branch merged main. New test reapidentity_test.go called the old signature: vet: internal/engine/reapidentity_test.go:34:54: not enough arguments in call to e.Spec have (*policy.Policy, string, []string, bool) want (*policy.Policy, string, []string, bool, *SignaturePolicy) Passes noSignaturePolicy(t) — "this host configured none" — same as every other Spec call in the package. NOT nil: Spec refuses nil outright as a caller that skipped ProjectHostSignaturePolicy, which is #307's no-fallback clause. The value does not change what TestEngineArgvNamesOnlyGuestPaths measures. The signature policy reaches the engine as a generated $HOME/.config/containers/policy.json (writeEngineHome), and podman 5.8.4 has no --signature-policy flag at all (engine.go:1068), so no value of it puts a path in the argv this test sweeps. Comment states that at the call site. make gate green. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Gap
internal/engine/reap.gosignalOwnedscanned/procfor processes whose command line names this run's engine socket, thensyscall.Kill(pid, sig)each BY NUMBER. Its own comment (waitQuiet) says these are "not our children, so there is no wait(2) to call on them" — exactly the foreign-pid case. Between the scan and the kill the number can be reaped and recycled to an unrelated process, which snug then SIGKILLs. Same reuse TOCTOU #294 removed from the orphan sweep; this was the live instance the #298 audit named.Fix
signalOwnednow, per discovered pid:unix.PidfdOpen(pid)→ re-read the cmdline through the number to confirm the pinned process still names the engine →unix.PidfdSendSignal. The pidfd pins the task the number named AT OPEN TIME, so the signal can never land on a later reuse; the re-read drops a number the scan matched but that no longer names us (recycled, or an engine that exited between scan and signal). Returns the pids actually signalled (was: all discovered; the single caller inengine.godiscards it).cmdlineNamesPathfactored out ofownedPIDsand shared.Namespace correctness (#167)
The pids are HOST-namespace:
ownedPIDsreads the host/procand matches the socket path in a HOST command line, sopidfd_open— which takes a pid in the caller's namespace — refers to the same task. This path never sees the engine's OWN pids, which are numbered in the engine's namespace and are not what it matches.Verified
make gategreen (internal/engineunit tests included; the new test needs no sandbox).TestSignalOwnedReverifiesThroughAPidfdBeforeKilling: a live process NOT naming the socket, handed tosignalPinnedby pid, must survive — the reuse case. Mutation-checked: dropping the re-verify guard kills it (signalPinned killed pid … which does not name …). Positive control: a process that names the socket is signalled and dies from SIGKILL, so the negative cannot pass on a signal path that kills nothing.Scope / not in scope
Own-child
cmd.Process.Kill()sites (reaper.go,stage/serve.go,stage/stage.go,sandbox/netns.gopasta,cli/attach.go) are reuse-SAFE — the unreaped child holds its number — and left as-is. Reading/proc/<pid>/…for DATA by number stays; the rule is about signalling and liveness-identity..claude/agents/go-implementer.mdgets the standing rule (pin with a pidfd to signal/track; bare numeric pid only safe for your own not-yet-waited child; namespace caveat #167).Not a hole-adding change — it removes a TOCTOU on a teardown kill path — so no new mount/seccomp/
@netsurface; the regression test is the deliverable.refs #298