From fd234124e13cc9665b2983530d5377e19943f07b Mon Sep 17 00:00:00 2001 From: Viorel Munteanu Date: Sun, 30 Aug 2026 16:35:26 +0000 Subject: [PATCH] Add crash-orphan sweep for stale tap-relay processes Direct tap+relay analog of the existing -p port-forward sweep: unlike a veth pair or a session's own bridge/persistent-namespace state, a relay process is host-global state with no automatic teardown if slocker-lite crashes before reaching its own stop_tap_relay() calls (bwrap still dies immediately via --die-with-parent, so only the relay can actually leak). tap_relay_state_path() uses the same xdg_state_dir()/ sanitize_for_filename() naming scheme as session_pid_file_path()/ port_forward_state_path(), so clean_stale_tap_relays() can cross-reference filenames against list_sessions() the same way clean_stale_port_forwards() already does. record_tap_relays()/ remove_tap_relay_record() are wired into run_container() the same two-places-split as their port-forward counterparts. Wired into clean_processes_command() (--clean-processes) alongside the two existing sweeps. Verified via a controlled scratch test, the same shape the original port-forward sweep used: a plain rootless daemonized session (no network join needed for the sweep logic itself) gave a real, live pid, alongside a hand-written matching record and a fabricated stale one in the same rootless state dir -- the matching record was left untouched, the fabricated one was correctly identified as stale and removed (kill() on its nonexistent pid failing harmlessly with ESRCH), and killing the real session then correctly swept its own now-stale record on a second run. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01Gv3s5jckJKzh6JkMoi2Akz --- CLAUDE.md | 51 ++++++++++++++++++++++-- src/commands.cpp | 14 +++++++ src/network_tap_relay.cpp | 82 +++++++++++++++++++++++++++++++++++++++ src/network_tap_relay.h | 47 ++++++++++++++++++++++ 4 files changed, 191 insertions(+), 3 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index cf5fe73..d6725fd 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -124,8 +124,11 @@ Source layout (all under `src/`): )` line per file actually removed, then also calls `clean_stale_port_forwards()` (`port_forward.h`, see below — commit 6 of `docs/networking-design.md`'s sequence) and prints one `removed stale - port-forward rules for '-'` line per record actually swept — - nothing is printed for sessions still running, and an empty result + port-forward rules for '-'` line per record actually swept, + then `clean_stale_tap_relays()` (`network_tap_relay.h`, see below — the + direct tap+relay analog of the port-forward sweep) and prints one + `removed stale tap-relay processes for '-'` line the same way + — nothing is printed for sessions still running, and an empty result (nothing stale) is silent success, same convention as the rest of this file's list/delete commands. `Mode::exec`'s dispatch case is a one-line call to `exec_in_session(*args.exec_pid, @@ -278,7 +281,13 @@ Source layout (all under `src/`): (`network_tap_relay.h`) analog of `active_port_forwards` above, since a relay process is likewise independent host-global state (unlike a veth pair) that needs an explicit `stop_tap_relay()` call for each, made right - alongside the `remove_port_forward()` loop after `run_bwrap()` returns. + alongside the `remove_port_forward()` loop after `run_bwrap()` returns — + paired the same two-places way with `record_tap_relays(container_name, + pid, active_relays)` (called right after `record_port_forwards()`, same + callback) and `remove_tap_relay_record(container_name, bwrap_pid)` (called + right after the `stop_tap_relay()` loop) for `--clean-processes`'s own + crash-orphan sweep (`network_tap_relay.h`'s `clean_stale_tap_relays()`, + see below). - `self_test.{h,cpp}` — `run_self_tests()` implements `-t/--test`, this project's own built-in self-test mode (distinct from the Meson-driven fixture smoke test under `tests/`, described in "Build & test commands" @@ -1005,6 +1014,42 @@ Source layout (all under `src/`): specifically needs re-verification, ideally on the actual veth-less target device (a different kernel/environment where this dev sandbox's own unidentified cause may not even apply) before being relied on. + + **Crash-orphan sweep**, the direct tap+relay analog of `port_forward.h`'s + own (see its own entry below): unlike a veth pair or a session's own + bridge/persistent-namespace state, a relay process is host-global state + with no automatic teardown at all if `slocker-lite` itself is + killed/crashes before reaching its own `stop_tap_relay()` calls (`bwrap` + still dies immediately in that case via `--die-with-parent`, so only the + *relay* — never the sandboxed container itself — can actually leak). + `tap_relay_state_path()` resolves `xdg_state_dir() / "tap-relays" / + "-"` — the same naming scheme + `session_pid_file_path()`/`port_forward_state_path()` already use, so + `clean_stale_tap_relays()` can cross-reference filenames directly against + `list_sessions()`'s own `SessionInfo::path`. `record_tap_relays()` writes + one line per relay (`" "`) to that path — a + no-op if there's nothing to record. `clean_stale_tap_relays()` + (`commands.cpp`'s `clean_processes_command()`, alongside + `clean_stale_sessions()`/`clean_stale_port_forwards()`) scans that + directory: a record whose filename doesn't match any currently-*running* + session is stale — every relay pid listed is `SIGKILL`ed (best-effort; an + already-dead pid, or one this process was never the parent of, isn't + treated as an error, since this sweep runs from a *separate* later + invocation that can't `waitpid()` an orphan it didn't fork — its true + parent's own exit, or `init` after reparenting, reaps it) before the + record file itself is deleted; a record whose session is still running is + left completely untouched. **Verified via a controlled scratch test**, + the same shape `port_forward.h`'s own sweep test used: root wasn't needed + for the sweep *logic* itself (only real tap/bridge creation needs it), + so this ran as a plain rootless daemonized session (`-D`, no `-n`) to get + a real, live pid + container name, alongside two hand-written record + files in that same rootless `$XDG_STATE_HOME` — one *matching* the live + session (confirmed left untouched by `--clean-processes`) and one + *fabricated* for a nonexistent pid (confirmed identified as stale, its + `kill()` attempt failing harmlessly with `ESRCH`, and its record file + actually removed, reported as `removed stale tap-relay processes for + 'faketest-999999'`); killing the real session and re-running + `--clean-processes` then correctly swept its own now-stale record too. - `session_cgroup.{h,cpp}` — gives `--kill` (`kill_session.{h,cpp}`, see below) a reliable way to find every process a session ever started, however deeply forked/daemonized/reparented, by putting it in a dedicated cgroup v2 diff --git a/src/commands.cpp b/src/commands.cpp index e7bbaf0..4549442 100644 --- a/src/commands.cpp +++ b/src/commands.cpp @@ -341,6 +341,9 @@ int clean_processes_command() { for (const auto& removed : clean_stale_port_forwards()) { fmt::print("removed stale port-forward rules for '{}'\n", removed); } + for (const auto& removed : clean_stale_tap_relays()) { + fmt::print("removed stale tap-relay processes for '{}'\n", removed); + } return 0; } @@ -732,6 +735,14 @@ int run_container(const std::filesystem::path& image_tar, // (--die-with-parent), but these iptables rules are host-global // state with no such automatic teardown. record_port_forwards(container_name, pid, active_port_forwards); + // Same crash-orphan reasoning as record_port_forwards() above, + // for the relay processes collected into active_relays instead + // of iptables rules -- a tap relay is likewise independent + // host-global state (a process, not something bwrap's own + // --die-with-parent tears down) that would otherwise leak + // forever if this process crashes before its own + // stop_tap_relay() calls below ever run. + record_tap_relays(container_name, pid, active_relays); if (daemonize_flag) { report_daemon_started(container_name, pid); } @@ -757,6 +768,9 @@ int run_container(const std::filesystem::path& image_tar, for (const auto& relay : active_relays) { stop_tap_relay(relay); } + if (bwrap_pid > 0) { + remove_tap_relay_record(container_name, bwrap_pid); + } if (!unmount_layer(mounted->top_layer_id)) { spdlog::error("failed to unmount layer {}", mounted->top_layer_id); diff --git a/src/network_tap_relay.cpp b/src/network_tap_relay.cpp index 39b5ece..c6a3db4 100644 --- a/src/network_tap_relay.cpp +++ b/src/network_tap_relay.cpp @@ -27,9 +27,12 @@ #include #include +#include #include #include #include +#include +#include #include #include @@ -37,6 +40,7 @@ #include "network_bridge.h" #include "persistent_netns.h" +#include "pid_file.h" #include "process.h" namespace { @@ -294,3 +298,81 @@ void stop_tap_relay(const TapRelayHandle& handle) { int status = 0; waitpid(handle.relay_pid, &status, 0); } + +std::filesystem::path tap_relay_state_path(std::string_view container_name, pid_t pid) { + return xdg_state_dir() / "tap-relays" / fmt::format("{}-{}", sanitize_for_filename(container_name), pid); +} + +void record_tap_relays(std::string_view container_name, pid_t pid, const std::vector& active) { + if (active.empty()) { + return; + } + + auto path = tap_relay_state_path(container_name, pid); + std::error_code ec; + std::filesystem::create_directories(path.parent_path(), ec); + if (ec) { + spdlog::warn("failed to create directory {}: {}", path.parent_path().string(), ec.message()); + return; + } + + std::ofstream out(path); + if (!out) { + spdlog::warn("failed to create {}", path.string()); + return; + } + for (const auto& relay : active) { + out << relay.relay_pid << ' ' << relay.host_tap_name << '\n'; + } +} + +void remove_tap_relay_record(std::string_view container_name, pid_t pid) { + std::error_code ec; + std::filesystem::remove(tap_relay_state_path(container_name, pid), ec); +} + +std::vector clean_stale_tap_relays() { + std::vector cleaned; + + auto dir = xdg_state_dir() / "tap-relays"; + std::error_code ec; + auto it = std::filesystem::directory_iterator(dir, ec); + if (ec) { + return cleaned; // directory doesn't exist yet -- nothing to sweep + } + + auto sessions = list_sessions(); + + for (const auto& entry : it) { + std::string filename = entry.path().filename().string(); + // Same naming scheme as session_pid_file_path()/port_forward_state_path(), + // deliberately -- cross-referencing filenames directly against + // list_sessions()'s own SessionInfo::path reuses its liveness check + // rather than re-deriving pid liveness a second, drifting way. + bool session_running = std::any_of(sessions.begin(), sessions.end(), [&](const SessionInfo& session) { + return session.running && session.path.filename().string() == filename; + }); + if (session_running) { + continue; + } + + std::ifstream in(entry.path()); + pid_t relay_pid = 0; + std::string host_tap_name; + while (in >> relay_pid >> host_tap_name) { + if (kill(relay_pid, SIGKILL) != 0 && errno != ESRCH) { + spdlog::warn("failed to kill stale tap relay pid {}: {}", relay_pid, strerror(errno)); + } + } + + std::error_code remove_ec; + std::filesystem::remove(entry.path(), remove_ec); + if (remove_ec) { + spdlog::warn("failed to remove stale tap-relay record {}: {}", entry.path().string(), + remove_ec.message()); + continue; + } + cleaned.push_back(filename); + } + return cleaned; +} diff --git a/src/network_tap_relay.h b/src/network_tap_relay.h index b9d28fa..2e36807 100644 --- a/src/network_tap_relay.h +++ b/src/network_tap_relay.h @@ -16,8 +16,11 @@ #pragma once +#include #include #include +#include +#include #include @@ -69,3 +72,47 @@ std::optional create_tap_relay(const NetworkEntry& network, cons // the same "no explicit teardown needed" property veth already has; this // function doesn't attempt any further cleanup beyond stopping the process. void stop_tap_relay(const TapRelayHandle& handle); + +// $XDG_STATE_HOME/slocker-lite/tap-relays/- -- same +// naming scheme (and same xdg_state_dir()/sanitize_for_filename(), +// pid_file.h) as session_pid_file_path()/port_forward_state_path() +// (port_forward.h), deliberately: clean_stale_tap_relays() below +// cross-references this directory's filenames directly against +// list_sessions()'s own SessionInfo::path, rather than re-deriving pid +// liveness a second, drifting way. +std::filesystem::path tap_relay_state_path(std::string_view container_name, pid_t pid); + +// Records `active` (one line per relay: " ") to +// tap_relay_state_path(container_name, pid), so a later +// clean_stale_tap_relays() run (e.g. after this process crashes before ever +// reaching its own stop_tap_relay() calls) knows which relay processes to +// stop for a session that's no longer running. A no-op if `active` is +// empty -- nothing to record. Best-effort: logs a warning and does nothing +// further on failure, never fatal. +void record_tap_relays(std::string_view container_name, pid_t pid, const std::vector& active); + +// Removes the record written by record_tap_relays() for (container_name, +// pid) -- called once this process's own stop_tap_relay() calls have +// already run, so clean_stale_tap_relays() doesn't try to stop the same +// (already-stopped) relays again later. A no-op if no such record exists. +void remove_tap_relay_record(std::string_view container_name, pid_t pid); + +// Implements --clean-processes's sweep for tap relay processes left running +// by a slocker-lite that crashed (or was killed) before reaching its own +// stop_tap_relay() calls -- unlike bwrap itself (--die-with-parent, so the +// sandboxed session dies immediately with its parent), a relay process has +// no such automatic teardown: it's an independent process, not a descendant +// of the crashed slocker-lite, and its device auto-cleanup (see +// stop_tap_relay() above) only happens once *something* actually stops it. +// Scans $XDG_STATE_HOME/slocker-lite/tap-relays/: for each record whose +// filename doesn't match any currently-*running* entry in list_sessions() +// (pid_file.h) -- i.e. the record is either for a session that's exited +// cleanly (and thus should have already removed its own record, but is +// checked again here for safety) or one that crashed -- SIGKILLs every +// relay pid listed in it (best-effort -- an already-dead pid, or one this +// process was never the parent of and so can't waitpid(), is not treated +// as an error) and removes the record file. A record whose session is +// still running is left completely alone. Returns the container names +// actually cleaned up, mirroring clean_stale_port_forwards()'s own return +// shape. +std::vector clean_stale_tap_relays();