Add regression test for the session-straggler sweep; resolve TODO entry
test_session_cleanup.cpp exercises kill_via_cgroup() directly against two
plain forked processes (one setsid()-ing away from the other before it
exits), confirming a reparented straggler is actually reaped -- reproducing
the real escape shape (no pid namespace support at all) through a full
mount/bwrap session isn't possible from the CLI on a single run, since
--unshare-pid is a config-file-only setting, not a flag.
Also resolves TODO.md's SIGINT/SIGTERM entry and extends the relevant
CLAUDE.md sections (bwrap.{h,cpp}, session_cgroup.{h,cpp}, kill_session.{h,cpp})
with the fix's rationale and its known residual limitation (a kernel with
neither cgroup v2 nor pid namespace support still can't be reached
automatically).
This commit is contained in:
@@ -405,6 +405,26 @@ Source layout (all under `src/`):
|
||||
matters. No production code changed for this; it's purely an artifact
|
||||
of forking network primitives from within a Catch2-instrumented
|
||||
process.
|
||||
- `tests/integration/test_session_cleanup.cpp` (`[integration][root]`,
|
||||
no `[net]`) — regression test for `run_bwrap()`'s own automatic
|
||||
post-exit straggler sweep (`bwrap.cpp`/`session_cgroup.h`'s own
|
||||
"Resolved" entry above). Deliberately exercises `kill_via_cgroup()`
|
||||
(`kill_session.h`) directly against a real cgroup with two plain forked
|
||||
processes (one standing in for the tracked bwrap pid, `setsid()`-ing
|
||||
away a second before exiting) rather than through the full mount/bwrap
|
||||
pipeline — reproducing the actual escape shape this fix targets (no pid
|
||||
namespace support at all) through a real sandboxed session isn't
|
||||
possible from the CLI on a single run (`--unshare-pid` is a
|
||||
config-file-only `NamespaceConfig` field, not a flag), whereas the
|
||||
mechanism actually under test — cgroup membership surviving
|
||||
reparenting, and `kill_via_cgroup()` reaping it — needs no
|
||||
container/image/bwrap involvement at all. Hit the exact same
|
||||
Catch2-fatal-signal-handler-inheritance issue `test_root_networking.cpp`'s
|
||||
own tap-relay test already found (the straggler process, forked from
|
||||
this same Catch2-instrumented process, would otherwise catch its own
|
||||
expected shutdown `SIGTERM` via the inherited handler and report a
|
||||
spurious failure) — fixed the same way, resetting `SIGTERM` to
|
||||
`SIG_DFL` right before forking the straggler.
|
||||
- `tests/support/fixtures.{h,cpp}` — `find_busybox_fixture()` (searches
|
||||
`images/busybox.tar` relative to cwd, this project's own established
|
||||
manual-testing convention; `nullopt` if absent, so `[net]` tests
|
||||
@@ -543,7 +563,14 @@ Source layout (all under `src/`):
|
||||
see below) the instant the real `bwrap` pid is known, then calls
|
||||
`release_session_lock()` once `run_process_foreground()` returns (covering
|
||||
every exit path — normal, nonzero, or a forwarded-signal exit — since that call
|
||||
always blocks until the child has actually exited).
|
||||
always blocks until the child has actually exited). Between that return and
|
||||
`release_session_lock()`/`remove_session_cgroup()`, `run_bwrap()` also
|
||||
unconditionally sweeps the session's own cgroup (`session_cgroup_pids()`,
|
||||
`kill_via_cgroup()` — `session_cgroup.h`/`kill_session.h`) for any process
|
||||
still left in it, force-stopping it before cleanup proceeds — see
|
||||
`session_cgroup.h`'s own "Resolved" entry for the full detail on why (a
|
||||
daemonized/reparented straggler could otherwise outlive the session
|
||||
regardless of how it ended).
|
||||
`build_bwrap_args()` no longer passes `--clearenv`/`--setenv` to `bwrap` itself;
|
||||
instead, `build_sandbox_env()` builds the sandboxed command's exact environment
|
||||
(`PATH`, `HOME`, `PWD` — hardcoded to `"/"`, matching `--chdir`'s own value; note
|
||||
@@ -1700,16 +1727,69 @@ Source layout (all under `src/`):
|
||||
testing causes on the two environments this project actually runs on).
|
||||
`remove_session_cgroup()` (called from the same post-`run_process_foreground()`
|
||||
spot `release_session_lock()` already is) only succeeds once the cgroup is
|
||||
empty — a straggler process still alive at normal exit (e.g. a daemonized
|
||||
process that outlived the session's own main command, a pre-existing
|
||||
exposure independent of this feature) leaves it in place with a warning, not
|
||||
a fatal error. `session_cgroup_pids()` reads `cgroup.procs` — this is the
|
||||
empty. `session_cgroup_pids()` reads `cgroup.procs` — this is the
|
||||
actual answer to "gather every process running inside the container":
|
||||
unlike anything derived from `/proc` parent-pid chains or pid namespaces,
|
||||
cgroup membership reliably includes every process the session ever started.
|
||||
`session_cgroup_supports_kill()`/`kill_session_cgroup()` wrap the
|
||||
`cgroup.kill` knob (Linux 5.14+): writing `"1"` to it atomically `SIGKILL`s
|
||||
every process currently in the cgroup in one step.
|
||||
|
||||
**Resolved: a daemonized/escaped straggler could survive the session
|
||||
ending, regardless of how it ended.** Reported by the user (Ctrl-C on a
|
||||
foreground session could leave processes running if they'd created a new
|
||||
session of their own — `forward_signal_to_foreground_child()`, `process.cpp`,
|
||||
only ever forwards the signal to the single tracked bwrap pid) — and, on
|
||||
reflection, a *normal* exit (or `-D/--daemonize`) had the exact same gap,
|
||||
since nothing ever swept the session's own cgroup automatically in any of
|
||||
those cases; only an explicit, separate `--kill` did. Fixed not in the
|
||||
signal handler itself (genuinely awkward: `kill_session()`'s own
|
||||
cgroup-first strategy selection does blocking polling/`waitpid()`s, unsafe
|
||||
from a signal handler, and re-deriving the session there via
|
||||
`list_sessions()` would see `run_bwrap()`'s own still-open `SessionLock` fd
|
||||
as "still running", since `flock()` ownership is per open file
|
||||
description, not per process) but in `run_bwrap()` (`bwrap.cpp`) itself:
|
||||
`kill_via_cgroup()` (previously `kill_session.cpp`-local) was exported
|
||||
(`kill_session.h`) and is now called directly, right after
|
||||
`run_process_foreground()` returns and before `remove_session_cgroup()`
|
||||
runs, whenever `session_cgroup_pids()` shows anything still left —
|
||||
unconditionally, regardless of *why* `run_process_foreground()` just
|
||||
returned (normal exit, or bwrap forwarding a caught SIGINT/SIGTERM). Since
|
||||
a dead process is removed from its own cgroup automatically, bwrap's own
|
||||
pid is already gone from `cgroup.procs` by that point, so this sweep only
|
||||
ever finds genuine leftover processes, never bwrap itself. `-D/--daemonize`
|
||||
needed no special-casing at all: it re-enters this exact same `run_bwrap()`
|
||||
call from within its own already-forked/`setsid()`'d child, so the sweep
|
||||
runs there too, for free — there's only ever the one call site. The grace
|
||||
period used here (`straggler_grace_period_seconds`, `bwrap.cpp`, a
|
||||
file-local constant) is deliberately much shorter than `kill_session()`'s
|
||||
own manual `--kill` default (3s vs. 10s): this runs on *every*
|
||||
`run_bwrap()` return, so the overwhelmingly common zero-stragglers case
|
||||
must stay instant (it does — `session_cgroup_pids()` returning empty
|
||||
short-circuits `kill_via_cgroup()`'s own `poll_until()` immediately, no
|
||||
delay at all), while a genuine straggler still gets a brief chance to exit
|
||||
gracefully before being force-killed. **Verified on this dev machine, root,
|
||||
via the scoped `doas` rule**: a new `[integration][root]` regression test
|
||||
(`tests/integration/test_session_cleanup.cpp`) confirms `kill_via_cgroup()`
|
||||
actually reaps a process that forks, `setsid()`s away, and outlives its own
|
||||
parent — deliberately exercised directly against a real cgroup with two
|
||||
plain forked processes rather than through the full mount/bwrap pipeline,
|
||||
since the actual escape shape under test (a kernel with no pid namespace
|
||||
support at all, so a daemonizing process reparents completely outside any
|
||||
namespace) isn't something a single `-r/--run` invocation can force via the
|
||||
CLI — `--unshare-pid` is a config-file-only `NamespaceConfig` field, not a
|
||||
flag. On a kernel that *does* support pid namespaces (this dev machine
|
||||
included), the default case already gets equivalent protection for free
|
||||
straight from the kernel — killing a pid namespace's own pid 1, whether via
|
||||
a normal exit or a forced kill, collapses the whole namespace regardless of
|
||||
this fix — so this sweep's real-world benefit is concentrated on kernels
|
||||
like the real target device's own, which has neither pid namespace nor (as
|
||||
of this writing) confirmed cgroup delegation; on-device re-verification of
|
||||
both is still needed (see `TODO.md`). **Known residual limitation,
|
||||
unchanged**: a kernel with *neither* cgroup v2 nor pid namespace support
|
||||
still has no automatic way to reach a reparented straggler — the same
|
||||
fundamental gap `kill_via_tracked_pid()` (the weakest of `--kill`'s own
|
||||
three strategies, below) already represents.
|
||||
- `sandbox_process.{h,cpp}` — process-tree/namespace-resolution utilities
|
||||
shared by `exec_session.{h,cpp}` and `kill_session.{h,cpp}` (see both
|
||||
below); pulled into their own file (rather than staying private to
|
||||
@@ -1919,6 +1999,11 @@ Source layout (all under `src/`):
|
||||
not the original snapshot, since a process could have forked a new child
|
||||
after the graceful sweep but before dying). The **only** mechanism that
|
||||
reliably reaches every process regardless of pid namespace support.
|
||||
Exported (moved out of this file's own anonymous namespace, declared in
|
||||
`kill_session.h`) since `run_bwrap()` (`bwrap.cpp`) reuses it directly
|
||||
for its own automatic post-exit straggler sweep — see `session_cgroup.h`'s
|
||||
own "Resolved" entry above for why that caller calls this directly
|
||||
rather than going through `kill_session(pid)` itself.
|
||||
**Critical correctness point, caught during design review before this
|
||||
shipped**: the "is it stopped yet" poll must gate on the *cgroup being
|
||||
empty*, not `list_sessions()`'s running flag — that flag only reflects
|
||||
|
||||
Reference in New Issue
Block a user