diff --git a/CLAUDE.md b/CLAUDE.md index c7e20e1..1dfd506 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -458,7 +458,7 @@ Source layout (all under `src/`): own automatic post-exit straggler sweep** (`bwrap.cpp`/`session_cgroup.h`'s "Resolved" entries), reproducing the user's own reported shape end to end with a real container: `nohup sleep 137 & exit` inside the sandbox, - then `any_process_cmdline_contains()`/`process_cmdline_gone_within()` + then `sleep_process_running()`/`sleep_process_gone_within()` (`.cpp`-local, same `/proc`-scanning shape as `bwrap.cpp`'s own `find_fuse_overlayfs_pid()`) confirm the backgrounded process is gone from the *host's* own process table afterward — checked by cmdline @@ -475,6 +475,33 @@ Source layout (all under `src/`): `[integration][root]` test (below) for one that exercises `kill_via_cgroup()` directly, since the real target device's own no-pid-namespace escape shape can't be forced via the CLI at all. + **Named `sleep_process_*` (not the original `any_process_cmdline_*`) since + a real false positive was found via this exact test session**: the + original version searched for the literal text `"sleep 137"` anywhere in + a candidate process's cmdline blob, which matched a manual diagnostic + `pkill -f 'sleep 137'` cleanup command run by hand while investigating a + separate issue — that command's own cmdline literally contains the + search text as a `pkill` pattern argument, despite not being a sleep + process at all. Fixed by requiring an exact match instead: `argv[0]`'s + basename is exactly `sleep` and `argv[1]` exactly equals the expected + duration. **`SKIP()`s when neither a pid namespace nor a working session + cgroup is available** under the current effective config + (`g_test_app_config`, `fixtures.h`) and live kernel capability — checked + via `pid_namespace_would_isolate()` (the same two-gate policy-and-kernel- + support check `build_bwrap_args()` itself applies) and + `session_cgroup_would_work()` (tries `create_directories()` + + `access(..., W_OK)` against a throwaway, never-joined probe path under + `session_cgroup_path()` — tried directly rather than guessed from e.g. + `geteuid()`, since the real failure mode this needs to detect, no + delegated subtree when rootless, is exactly a permissions question those + calls can answer directly; never writes this process's own pid into the + probe directory, so cleanup is just removing the still-empty directory). + In that combination there is genuinely no mechanism left that could reap + a reparented straggler — the documented, known residual limitation + (`session_cgroup.h`'s own entry above), not a regression — added + specifically per the user's own follow-up request after using `-c` to + force exactly that combination and correctly getting a failure instead + of a skip. - `tests/integration/test_root_networking.cpp` (`[integration][root][net]`) — the persistent-netns/tap-relay/dns-resolver tests that originally lived directly in this file, ported to tagged `TEST_CASE`s (every @@ -1901,6 +1928,73 @@ Source layout (all under `src/`): 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. + + **Resolved: a genuine race could leave the sweep unable to reach the + straggler at all, specifically without a pid namespace.** Found by the + user's own explicit follow-up request to test the "pid namespace off, + cgroup on" combination as root (via `-c`/`--config-file`'s now-wired-in + effect on `-t`, see `tests/support/fixtures.h`'s own entry below) — the + nohup-straggler regression test (`test_rootless_run.cpp`) failed + *consistently*, even as root with cgroup v2 genuinely available and + writable. Root-caused directly, not assumed: inspecting + `/sys/fs/cgroup/slocker-lite//cgroup.procs` *while the session was + still running* showed only bwrap's own outer pid — its own child (the + actual sandboxed `sh` process, and the `sleep` it later backgrounded) was + never a member at all. Cause: `create_session_cgroup()` used to be called + from `run_bwrap()`'s `on_start` callback, which runs in the *parent*, + concurrently with the just-`fork()`'d child execing into bwrap and bwrap + then doing its *own* internal `clone()` of the sandboxed target — a genuine + race between the parent's own directory-create-plus-write (several + syscalls) and bwrap's own setup. Without `--unshare-pid`, bwrap has + meaningfully less setup work to do (no new pid namespace to create), making + it reliably fast enough to win that race and clone its target *before* the + parent's own write into `cgroup.procs` ever completed — leaving that + target, and everything it later spawns, permanently outside the tracked + cgroup. This apparently didn't manifest with `--unshare-pid` requested + (bwrap's extra setup work there was consistently slow enough for the + parent to win instead) or on the real target device (confirmed working + there earlier, likely for the same reason, or because cgroup delegation + wasn't actually the mechanism catching it there) — but was never a + guaranteed property either way, just a timing coincidence. + + **Fix**: `run_process_foreground()` (`process.{h,cpp}`) gained a new + `before_exec` parameter — a callback invoked *in the child*, synchronously, + immediately before `execvp()` — alongside the existing `on_start` (which + still runs in the parent, unchanged, for the session lock and + `on_bwrap_pid_known`). `run_bwrap()` now creates the session cgroup from + *inside* `before_exec` (`create_session_cgroup(container_name, getpid())`) + instead of from `on_start` — since the child cannot proceed to `execvp()` + (and thus cannot start any of bwrap's own internal forking) until this + call has already returned, the race is closed structurally, not by timing + luck. The parent's own `on_start` callback still reconstructs the + (fully deterministic) `SessionCgroup{session_cgroup_path(container_name, + pid)}` unconditionally, regardless of whether the child-side creation + actually succeeded — `session_cgroup_pids()`/`remove_session_cgroup()` + already tolerate a nonexistent directory gracefully either way, the same + as they already did for any other `create_session_cgroup()` failure (e.g. + no cgroup v2 delegation when rootless). Ordinary (non-async-signal-safe) + work in a post-`fork()`/pre-`exec()` child is safe here since this project + has no threads — the same reasoning `daemonize()`'s own child branch + already relies on for its own, more extensive pre-exec setup. + + **Verified end-to-end on this dev machine, root, via the scoped `doas` + rule**: reproduced the failure consistently (3/3 trials, each from a + freshly-cleaned state) against a `-c`-supplied config with + `unshare-pid: false` and every other field at its default, using the + live `cgroup.procs` inspection above to confirm the exact mechanism; after + the fix, the identical reproduction passed consistently (3/3 isolated + trials, plus repeated full-category runs). **A second, unrelated bug + surfaced and was fixed while verifying this**: the regression test's own + `any_process_cmdline_contains()` did a substring search across a + candidate process's *entire* cmdline blob, which could false-positive + against an unrelated process that merely *mentions* the marker text + somewhere in its own arguments — confirmed directly: a manual diagnostic + `pkill -f 'sleep 137'` cleanup command run by hand during this same + investigation was itself briefly matched as if it were the sleep process. + Renamed to `sleep_process_running()`/`sleep_process_gone_within()` and + tightened to require `argv[0]`'s basename to be exactly `sleep` and + `argv[1]` to exactly match the expected duration, eliminating that class + of false positive. - `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 diff --git a/TODO.md b/TODO.md index e687ef7..527cf38 100644 --- a/TODO.md +++ b/TODO.md @@ -45,13 +45,23 @@ 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. +completely outside any namespace -- originally couldn't be exercised +end-to-end locally (no CLI-level way to force that off for a single run; +it was a config-file-only `NamespaceConfig` field). + +**Since resolved**: `-c/--config-file` (added later) made this directly +testable locally after all, and doing exactly that (per the user's own +explicit request) found a real, separate bug -- a race between this fix's +own `create_session_cgroup()` call and bwrap's own internal forking, which +could leave the sweep unable to reach the straggler at all specifically +without a pid namespace, even as root with cgroup v2 genuinely available. +See `CLAUDE.md`'s `session_cgroup.{h,cpp}` entry for the full root-cause +and fix (`process.h`'s new `before_exec` hook). Re-verified end-to-end +(root, this dev machine, `-c `) after the fix: 3/3 +isolated trials and repeated full-category runs all correctly reaped the +straggler. On-device re-confirmation (the real target device has neither +pid namespace nor previously-confirmed cgroup delegation) is still +worthwhile but no longer the only way to exercise this path locally. **Known residual limitation**: this only helps when `create_session_cgroup()` actually succeeded (cgroup v2 mounted and writable). A kernel with *neither* diff --git a/src/bwrap.cpp b/src/bwrap.cpp index a8a1e2b..d8b4ca1 100644 --- a/src/bwrap.cpp +++ b/src/bwrap.cpp @@ -397,17 +397,40 @@ int run_bwrap(const std::string& root, const std::vector& command, *argv, [&](pid_t pid) { session_lock = create_session_lock(container_name, pid); - // Best-effort, like the session lock above -- lets kill_session() - // (kill_session.cpp) reach every process this session ever starts, - // including anything that later daemonizes/double-forks and gets - // reparented, regardless of whether the kernel supports pid - // namespaces at all. See session_cgroup.h. - session_cgroup = create_session_cgroup(container_name, pid); + // The actual cgroup creation/membership write happens + // synchronously in the child itself, via before_exec below, + // *before* it execs into bwrap -- doing it here instead (in the + // parent, concurrently with the child) raced against bwrap's own + // internal fork of the sandboxed target and could lose: confirmed + // by testing that without --unshare-pid, bwrap has little enough + // setup work to do that it can clone its target before this + // callback's own write into cgroup.procs ever completed, leaving + // that target -- and everything it later spawns -- outside the + // tracked cgroup entirely (session_cgroup_pids() showing only + // bwrap's own pid, never its child's). The path itself is fully + // deterministic from (container_name, pid), so it's reconstructed + // here unconditionally regardless of whether the child-side + // creation actually succeeded -- session_cgroup_pids()/ + // remove_session_cgroup() below already tolerate a nonexistent + // directory gracefully either way, the same as they already did + // for a session_lock/session_cgroup that failed to create for + // some other reason (e.g. no cgroup v2 delegation when rootless). + session_cgroup = SessionCgroup{session_cgroup_path(container_name, pid)}; if (on_bwrap_pid_known) { on_bwrap_pid_known(pid); } }, - build_sandbox_env(user, extra_env)); + build_sandbox_env(user, extra_env), + [&]() { + // Runs in the child, before execvp() -- see this function's own + // before_exec doc comment (process.h) for why this must happen + // here rather than in the on_start callback above. getpid() here + // is this same child's own pid, matching exactly the `pid` + // on_start was (or will be) called with -- exec() never changes + // a process's pid, so this is the same identity bwrap itself + // will have. + create_session_cgroup(container_name, getpid()); + }); // Runs unconditionally, regardless of *why* run_process_foreground() just // returned (normal exit, or bwrap forwarded a SIGINT/SIGTERM) -- covers a diff --git a/src/process.cpp b/src/process.cpp index 9d04851..3b536a0 100644 --- a/src/process.cpp +++ b/src/process.cpp @@ -109,7 +109,8 @@ ProcessResult run_process(const std::vector& argv) { } int run_process_foreground(const std::vector& argv, const std::function& on_start, - const std::optional>>& env) { + const std::optional>>& env, + const std::function& before_exec) { spdlog::debug("running external command: {}", fmt::join(argv, " ")); pid_t pid = fork(); @@ -124,6 +125,9 @@ int run_process_foreground(const std::vector& argv, const std::func setenv(key.c_str(), value.c_str(), 1); } } + if (before_exec) { + before_exec(); + } auto c_argv = to_c_argv(argv); execvp(c_argv[0], c_argv.data()); const char* msg = "run_process_foreground: execvp failed\n"; diff --git a/src/process.h b/src/process.h index 5411aec..1b52c3b 100644 --- a/src/process.h +++ b/src/process.h @@ -45,7 +45,21 @@ ProcessResult run_process(const std::vector& argv); // child replaces its entire environment with exactly these key/value pairs (via // clearenv()/setenv(), before exec) instead of inheriting this process's own -- // e.g. run_bwrap() uses this to give the sandboxed command a minimal, controlled -// environment without relying on bwrap's own --clearenv/--setenv. Returns the +// environment without relying on bwrap's own --clearenv/--setenv. If `before_exec` +// is set, it's called *in the child*, synchronously, immediately before execvp() +// -- for setup that must be guaranteed complete before the target program starts +// doing anything of its own, which `on_start` (running concurrently, in the +// parent) can't guarantee: e.g. run_bwrap() uses this to put the child into its +// own session cgroup (session_cgroup.h) before it execs into bwrap, since doing +// that from the parent's own `on_start` instead raced against bwrap's own +// internal fork of the sandboxed target and could lose (confirmed by testing: +// without --unshare-pid, bwrap can clone its target before the parent's own +// write into cgroup.procs completes, leaving that target -- and everything it +// later spawns -- outside the tracked cgroup entirely). Only async-signal-safe-ish +// work belongs here in the strictest POSIX sense, but this project has no +// threads, so ordinary calls (filesystem I/O, etc.) are safe in practice -- +// the same reasoning daemonize()'s own child branch (daemonize.cpp) already +// relies on. Returns the // exit code if the child exited normally; -1 only if fork() itself failed // (on_start is not called in that case) -- a failed execvp inside the child // is a normal exit with code 127, not -1. If the child was instead killed by @@ -61,8 +75,8 @@ ProcessResult run_process(const std::vector& argv); // unexpected crash. int run_process_foreground(const std::vector& argv, const std::function& on_start = nullptr, - const std::optional>>& env = - std::nullopt); + const std::optional>>& env = std::nullopt, + const std::function& before_exec = nullptr); // Searches $PATH for an executable regular file named `name`, in PATH order. // Returns its full path, or nullopt if not found. diff --git a/tests/integration/test_rootless_run.cpp b/tests/integration/test_rootless_run.cpp index 6abd069..bb1c8e3 100644 --- a/tests/integration/test_rootless_run.cpp +++ b/tests/integration/test_rootless_run.cpp @@ -66,6 +66,7 @@ #include "commands.h" #include "config_file.h" #include "fixtures.h" +#include "session_cgroup.h" namespace { @@ -141,17 +142,39 @@ std::string run_in_fixture(const std::filesystem::path& image, const std::vector return capture.contents(); } -// Scans /proc for any process whose cmdline contains `needle` -- used below -// to confirm a backgrounded process detached inside a container doesn't -// survive the session, checked from the *host's* own process table. This -// works regardless of whether the session's own pid namespace is isolated -// or not: every process, wherever it lives namespace-wise, is still a -// perfectly ordinary task on the host with its own real pid and -// /proc//cmdline entry -- only the pid *number* a process sees for -// itself differs inside an isolated namespace, not whether it shows up -// here at all. Same directory-scanning shape as bwrap.cpp's own -// find_fuse_overlayfs_pid(). -bool any_process_cmdline_contains(const std::string& needle) { +// Splits a raw /proc//cmdline blob (NUL-separated, per Linux's own +// documented format) into its individual arguments. +std::vector split_cmdline_args(const std::string& raw) { + std::vector args; + size_t start = 0; + while (start < raw.size()) { + size_t end = raw.find('\0', start); + if (end == std::string::npos) { + end = raw.size(); + } + args.push_back(raw.substr(start, end - start)); + start = end + 1; + } + return args; +} + +// True if some process on the *host* is genuinely running `sleep ` -- +// checked from the host's own process table (works regardless of whether +// the session's own pid namespace is isolated or not: every process, +// wherever it lives namespace-wise, is still a perfectly ordinary task on +// the host with its own real pid and /proc//cmdline entry -- only the +// pid *number* a process sees for itself differs inside an isolated +// namespace, not whether it shows up here at all). Deliberately checks +// argv[0]'s basename and argv[1] exactly, not a substring search across the +// whole cmdline -- a real bug found via this exact test session: an early +// version searched for the literal text "sleep 137" anywhere in the +// cmdline, which produced a false positive by matching this test's *own* +// diagnostic `pkill -f 'sleep 137'` cleanup commands (run by hand, outside +// the test, while investigating a separate issue) -- their own cmdline +// literally contains that text as a pkill pattern argument, despite not +// being a sleep process at all. Same directory-scanning shape as +// bwrap.cpp's own find_fuse_overlayfs_pid(). +bool sleep_process_running(const std::string& arg) { std::error_code ec; auto it = std::filesystem::directory_iterator("/proc", ec); if (ec) { @@ -163,27 +186,28 @@ bool any_process_cmdline_contains(const std::string& needle) { continue; } std::ifstream cmdline_file(entry.path() / "cmdline", std::ios::binary); - std::string cmdline((std::istreambuf_iterator(cmdline_file)), std::istreambuf_iterator()); - std::replace(cmdline.begin(), cmdline.end(), '\0', ' '); - if (cmdline.find(needle) != std::string::npos) { + std::string raw((std::istreambuf_iterator(cmdline_file)), std::istreambuf_iterator()); + auto args = split_cmdline_args(raw); + if (args.size() == 2 && std::filesystem::path(args[0]).filename() == "sleep" && args[1] == arg) { return true; } } return false; } -// Bounded (2s, 50ms interval) poll for `needle` to disappear from the -// host's own process table -- a session ending via a pid namespace's own -// kernel-guaranteed collapse-on-pid-1-exit isn't necessarily synchronously -// complete by the instant run_in_fixture() above returns (that guarantee is -// about eventual termination, not that every other task in the namespace -// has already been fully reaped), so a single instantaneous check right -// after could be flaky. The straggler sweep this test actually targets -// (run_bwrap()'s own post-exit cgroup sweep, bwrap.cpp) already applies a -// real grace period internally for the same reason. -bool process_cmdline_gone_within(const std::string& needle, int timeout_ms) { +// Bounded (2s, 50ms interval) poll for the `sleep ` process to +// disappear from the host's own process table -- a session ending via a +// pid namespace's own kernel-guaranteed collapse-on-pid-1-exit isn't +// necessarily synchronously complete by the instant run_in_fixture() above +// returns (that guarantee is about eventual termination, not that every +// other task in the namespace has already been fully reaped), so a single +// instantaneous check right after could be flaky. The straggler sweep this +// test actually targets (run_bwrap()'s own post-exit cgroup sweep, +// bwrap.cpp) already applies a real grace period internally for the same +// reason. +bool sleep_process_gone_within(const std::string& arg, int timeout_ms) { for (int waited = 0; waited < timeout_ms; waited += 50) { - if (!any_process_cmdline_contains(needle)) { + if (!sleep_process_running(arg)) { return true; } struct timespec ts { @@ -192,7 +216,44 @@ bool process_cmdline_gone_within(const std::string& needle, int timeout_ms) { while (nanosleep(&ts, &ts) != 0 && errno == EINTR) { } } - return !any_process_cmdline_contains(needle); + return !sleep_process_running(arg); +} + +// Whether the current -t run's effective config (g_test_app_config, +// fixtures.h) would actually get bwrap to request --unshare-pid for a +// container it creates -- both the config's own policy *and* live kernel +// support (detect_bwrap_unshare_args(), bwrap.h) have to allow it, the same +// two-gate check build_bwrap_args() itself applies. +bool pid_namespace_would_isolate() { + if (!g_test_app_config.unshare_pid.value_or(true)) { + return false; + } + auto supported = detect_bwrap_unshare_args(); + return std::find(supported.begin(), supported.end(), "--unshare-pid") != supported.end(); +} + +// Whether run_bwrap()'s own session cgroup (session_cgroup.h) would actually +// be creatable/writable right now -- tried directly against a throwaway, +// never-joined path rather than guessing from e.g. geteuid(), since the real +// failure mode this needs to detect (no delegated subtree when rootless) is +// exactly a permissions question create_directories()/access() can answer +// directly. Deliberately never writes this process's own pid into the +// probe directory's cgroup.procs (unlike the real create_session_cgroup()), +// so cleanup is just removing the still-empty directory -- no need to +// un-join anything first. +bool session_cgroup_would_work() { + if (!cgroup_v2_available()) { + return false; + } + auto probe_path = session_cgroup_path("selftest-cgroup-probe", getpid()); + std::error_code ec; + std::filesystem::create_directories(probe_path, ec); + if (ec) { + return false; + } + bool writable = access((probe_path / "cgroup.procs").c_str(), W_OK) == 0; + std::filesystem::remove(probe_path, ec); + return writable; } } // namespace @@ -295,14 +356,25 @@ TEST_CASE("rootless -r/--run: a nohup-backgrounded process does not survive the SKIP("no busybox fixture (images/busybox.tar) -- see tests/setup-tests.py"); } + // With neither a pid namespace nor a working session cgroup available, + // there is genuinely no mechanism left that could reap a reparented + // straggler -- a documented, known limitation (CLAUDE.md's + // session_cgroup.{h,cpp} entry, "Known residual limitation"), not a + // regression this test should be flagging. Skip rather than fail so a + // deliberately-crippled config (e.g. via -c/--config-file, g_test_app_config + // above) doesn't produce a misleading failure here. + if (!pid_namespace_would_isolate() && !session_cgroup_would_work()) { + SKIP("neither a pid namespace nor a working session cgroup is available under the " + "current config/kernel -- no mechanism exists to reap a reparented straggler here"); + } + // A distinctive duration -- not a realistic value anything else on this - // host would coincidentally already be sleeping for -- so scanning - // /proc for it can't produce a false positive either way. - const std::string marker = "sleep 137"; - REQUIRE_FALSE(any_process_cmdline_contains(marker)); + // host would coincidentally already be sleeping for. + const std::string marker = "137"; + REQUIRE_FALSE(sleep_process_running(marker)); ScratchXdgDirs scratch; run_in_fixture(*image, {"sh", "-c", "nohup sleep 137 >/dev/null 2>&1 & exit"}); - CHECK(process_cmdline_gone_within(marker, 2000)); + CHECK(sleep_process_gone_within(marker, 2000)); }