From 1f52bc5f6f1ab70460c4ab61500f7bcb0719dd61 Mon Sep 17 00:00:00 2001 From: Viorel Munteanu Date: Sat, 5 Sep 2026 07:20:51 +0000 Subject: [PATCH] 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). --- CLAUDE.md | 95 ++++++++++++++- TODO.md | 80 ++++++++---- meson.build | 1 + tests/integration/test_session_cleanup.cpp | 134 +++++++++++++++++++++ 4 files changed, 279 insertions(+), 31 deletions(-) create mode 100644 tests/integration/test_session_cleanup.cpp diff --git a/CLAUDE.md b/CLAUDE.md index 40f7f9f..3b125bf 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -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 diff --git a/TODO.md b/TODO.md index 8cfec4f..e687ef7 100644 --- a/TODO.md +++ b/TODO.md @@ -1,35 +1,63 @@ # TODO -## Ctrl-C/SIGTERM on a foreground session doesn't reach everything --kill does +## Resolved: Ctrl-C/SIGTERM (or a normal exit) could leave session stragglers running -Reported by the user while device-testing the signal-exit-logging fix (not -yet reproduced/confirmed with a specific repro, just the architectural gap): -`forward_signal_to_foreground_child()` (`process.cpp`, installed as the -SIGINT/SIGTERM handler around `run_process_foreground()`'s own `waitpid()`) -does a single, simple `kill(g_foreground_child_pid, sig)` -- only the one -pid `run_bwrap()` originally tracked (bwrap's own outer process). If -anything inside the sandbox daemonizes/double-forks and calls `setsid()` +Reported by the user while device-testing the signal-exit-logging fix, then +fixed on request: `forward_signal_to_foreground_child()` (`process.cpp`, +installed as the SIGINT/SIGTERM handler around `run_process_foreground()`'s +own `waitpid()`) only ever does a single, simple `kill(g_foreground_child_pid, +sig)` -- just the one pid `run_bwrap()` tracks (bwrap's own outer process). +If anything inside the sandbox daemonizes/double-forks and calls `setsid()` (escaping into a new session, the same shape `--kill`'s own investigation found for a real `php-fpm --daemonize` + `exec caddy` init script -- see -`kill_session.{h,cpp}`'s own three-strategy design in `CLAUDE.md`), it -escapes this simple forward entirely and is left running after Ctrl-C ends -the foreground session, even though `--kill` against the exact same session -would reach it (via `kill_via_cgroup()`, or `kill_via_pid_namespace()` where -supported). +`kill_session.{h,cpp}`'s own three-strategy design in `CLAUDE.md`), that +signal forward alone could never reach it -- and, on reflection, a *normal* +exit (or `-D/--daemonize`) had exactly the same gap, since nothing swept the +session's own leftovers in any of those cases either. -**Likely fix, not yet designed in detail**: reuse `kill_session()`'s own -strategy selection (cgroup-first, falling back to pid-namespace or the -tracked-pid-only approach per host capability) from -`forward_signal_to_foreground_child()` too, instead of a bare `kill()` -- -the session's own cgroup (`session_cgroup.h`) and tracked pid are already -known by the time `run_process_foreground()` is running, so the pieces -`kill_session()` needs should already be available without major -restructuring. Signal-safety needs care: `kill_session()` as it exists -today does blocking work (polling, `waitpid()`s) that isn't safe to call -directly from a signal handler -- likely needs the handler to just set a -flag/write to a self-pipe and have the actual `kill_session()`-style sweep -happen back in `run_process_foreground()`'s own normal control flow once -`waitpid()` returns, rather than doing it inside the handler itself. +**Fix, per the user's own suggested direction**: rather than teaching the +signal handler itself to do `kill_session()`-style work (genuinely awkward -- +`kill_session()` does blocking polling/`waitpid()`s, unsafe to call directly +from a signal handler, and re-derives the session via `list_sessions()`, +which would see `run_bwrap()`'s own still-open `SessionLock` fd as "still +running" since `flock()` ownership is per open file description, not per +process), `kill_via_cgroup()` (`kill_session.cpp`'s own cgroup-based +strategy) was exported and is now called directly from `run_bwrap()` itself, +right after `run_process_foreground()` returns and before the session +cgroup is torn down -- unconditionally, regardless of *why* it returned +(normal exit, a forwarded SIGINT/SIGTERM), so a container that daemonizes +and exits successfully can no longer leave a background process running +either. `-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. See `bwrap.{h,cpp}`'s and +`kill_session.{h,cpp}`'s own entries in `CLAUDE.md` for the full detail. + +**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's exit -- the exact escape shape this fix targets -- and a live +`-r/--run` session backgrounding a long-running process and exiting +normally left no leftover process and no leftover cgroup directory +afterward. This dev machine's own kernel supports pid namespaces, though, +so the *default* case (`--unshare-pid` requested) already gets this for +free from the kernel itself (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); the scenario this fix specifically targets -- +`--unshare-pid` unavailable or disabled, so a daemonizing process reparents +completely outside any namespace -- couldn't be exercised end-to-end +locally (no CLI-level way to force that off for a single run; it's a +config-file-only `NamespaceConfig` field), and needs on-device +confirmation, same as everything else in this project that depends on the +real target's own kernel capabilities. Whether the real device's cgroup v2 +is actually writable as root (the case that matters there) also hasn't +been specifically re-verified yet. + +**Known residual limitation**: this only helps when `create_session_cgroup()` +actually succeeded (cgroup v2 mounted and writable). A kernel with *neither* +cgroup v2 nor pid namespace support still has no way to reach a reparented +straggler automatically -- the same fundamental gap `kill_via_tracked_pid()` +(the weakest of `--kill`'s own three strategies) already represents. ## Resolved: config/state paths sometimes resolved relative to cwd diff --git a/meson.build b/meson.build index 7c03f4b..96bf0ca 100644 --- a/meson.build +++ b/meson.build @@ -31,6 +31,7 @@ if get_option('enable_tests') 'tests/integration/test_config_bwrap_chain.cpp', 'tests/integration/test_rootless_run.cpp', 'tests/integration/test_root_networking.cpp', + 'tests/integration/test_session_cleanup.cpp', ] endif diff --git a/tests/integration/test_session_cleanup.cpp b/tests/integration/test_session_cleanup.cpp new file mode 100644 index 0000000..19d1d7c --- /dev/null +++ b/tests/integration/test_session_cleanup.cpp @@ -0,0 +1,134 @@ +// Copyright (C) 2026 Viorel Munteanu +// +// This program is free software; you can redistribute it and/or modify +// it under the terms of the GNU General Public License as published by +// the Free Software Foundation; either version 2 of the License, or +// (at your option) any later version. +// +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. +// +// You should have received a copy of the GNU General Public License along +// with this program; if not, write to the Free Software Foundation, Inc., +// 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. + +// [integration][root]: regression test for run_bwrap()'s own automatic +// post-exit straggler sweep (bwrap.cpp) -- exercises the exported +// kill_via_cgroup() (kill_session.h) directly against a real cgroup, rather +// than going through the full mount/bwrap pipeline. That's a deliberate +// choice, not a shortcut: the actual bug this targets is a process that +// daemonizes/double-forks and setsid()'s away, reparenting outside the +// tracked process's own tree (e.g. no pid namespace support at all, the +// real target device's own kernel) -- reproducing that specific escape +// through a real bwrap sandbox would need to fake the kernel's own pid +// namespace absence (not controllable from the CLI at all -- it's a +// config-file-only NamespaceConfig field, see bwrap.h), whereas the actual +// mechanism under test (cgroup membership surviving reparenting, and +// kill_via_cgroup() reaping it) is fully exercised with two plain forked +// processes and no container/image/bwrap involved at all. + +#include + +#include +#include + +#include + +#include + +#include "kill_session.h" +#include "session_cgroup.h" + +TEST_CASE("session cgroup sweep: kill_via_cgroup reaps a reparented straggler", "[integration][root]") { + if (geteuid() != 0) { + SKIP("requires root"); + } + + const std::string container_name = "selftest-cgroup-sweep"; + + // sync_pipe makes the child wait until it's actually been moved into the + // session cgroup before it forks its own straggler -- otherwise the + // straggler could be forked (and thus inherit cgroup membership) before + // create_session_cgroup() below has run, racing the exact same way + // run_bwrap()'s own on_start callback races against bwrap forking its + // own children. + int sync_pipe[2]; + REQUIRE(pipe(sync_pipe) == 0); + + pid_t tracked_pid = fork(); + REQUIRE(tracked_pid >= 0); + if (tracked_pid == 0) { + close(sync_pipe[1]); + char buf = 0; + ssize_t unused = read(sync_pipe[0], &buf, 1); + (void)unused; + close(sync_pipe[0]); + + // Reset to the default disposition before forking: the straggler + // below is expected to receive a real SIGTERM later (from + // kill_via_cgroup()) and terminate via it -- Catch2 installs its own + // fatal-signal handler around a running TEST_CASE, which a forked + // child inherits (only the disposition *at fork time* matters), so + // without this reset the straggler's own ordinary shutdown signal + // gets caught by that inherited handler instead, producing a + // spurious "FAILED ... due to a fatal error condition: SIGTERM" + // report interleaved into this test's real output -- the exact same + // lesson test_root_networking.cpp's tap-relay test already learned + // for its own forked relay child. Confirmed cosmetic only when hit: + // the assertions below still ran and passed correctly either way. + signal(SIGTERM, SIG_DFL); + + // Mirrors a container process that daemonizes and exits successfully + // (or is signaled and dies) while a backgrounded child of its own + // keeps running, detached into a new session -- the exact shape + // that used to escape bwrap's own pid tree with nothing left to + // reap it. + pid_t straggler = fork(); + if (straggler == 0) { + setsid(); + pause(); + _exit(0); + } + _exit(0); + } + close(sync_pipe[0]); + + // Mirrors create_session_cgroup() being called from run_bwrap()'s own + // on_start callback, the instant the tracked pid is known -- before it's + // had any real chance to fork further. + auto cgroup = create_session_cgroup(container_name, tracked_pid); + if (!cgroup) { + char go = 1; + ssize_t unused = write(sync_pipe[1], &go, 1); + (void)unused; + close(sync_pipe[1]); + int status = 0; + waitpid(tracked_pid, &status, 0); + SKIP("could not create a session cgroup on this host (see session_cgroup.h)"); + } + + char go = 1; + REQUIRE(write(sync_pipe[1], &go, 1) == 1); + close(sync_pipe[1]); + + int status = 0; + // Mirrors run_process_foreground()'s own waitpid() on the tracked pid -- + // by the time this returns, `tracked_pid` is already gone from + // cgroup.procs (a dead process is removed from its cgroup + // automatically), leaving only the straggler behind, exactly like a + // real bwrap exit would. + waitpid(tracked_pid, &status, 0); + + auto before = session_cgroup_pids(*cgroup); + // Confirms the straggler really did escape into the cgroup in the first + // place -- otherwise the kill_via_cgroup() check below would trivially + // "pass" against an already-empty cgroup and prove nothing. + REQUIRE_FALSE(before.empty()); + + CHECK(kill_via_cgroup(*cgroup, /*grace_period_seconds=*/2)); + CHECK(session_cgroup_pids(*cgroup).empty()); + + remove_session_cgroup(*cgroup); +}