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();