Add -k/--kill <pid> to fully stop a running session
A plain `kill <tracked_bwrap_pid>` doesn't kill everything a container
started: on a kernel without pid namespace support, a service that
daemonizes (double-forks and detaches) before the entrypoint execs into its
main command reparents all the way to the *host's* own pid 1, completely
disconnected from the sandboxed session -- confirmed via a real session log
from the user's own Android target device, where php-fpm and caddy both
kept running as orphans after killing the tracked pid.
kill_session() (src/kill_session.{h,cpp}) picks between three
independently-named strategies per session, based on what's actually
available for it:
- kill_via_cgroup(): preferred when the session has a dedicated cgroup
(src/session_cgroup.{h,cpp}, set up at -r/--run time in run_bwrap()'s
on_start callback). Reaches every process the session ever started,
daemonized/reparented or not, regardless of pid namespace support.
- kill_via_pid_namespace(): used when --unshare-pid was genuinely in effect
for the session (src/sandbox_process.{h,cpp}, shared with exec_session.cpp,
which already needed resolve_namespace_pid()). Relies on the kernel's own
guarantee that killing a pid namespace's pid 1 tears down everything in it.
- kill_via_tracked_pid(): fallback, signals the tracked pid directly -- no
worse than today's manual kill. This is what the user's real target
device currently falls back to (no pid namespace support there).
Each strategy sends SIGTERM, waits up to a 10s grace period, then forces a
SIGKILL. Verified locally (rootless dev machine, which does support pid
namespaces): a daemonizing test session was fully cleaned up via
kill_via_pid_namespace(), including the forced-SIGKILL escalation path,
with no leftover processes, mounts, or layers.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Gv3s5jckJKzh6JkMoi2Akz
This commit is contained in:
@@ -38,7 +38,12 @@ Source layout (all under `src/`):
|
||||
`Mode::exec` dispatch arm) `-e/--exec <pid>`'s own pid parsing/validation
|
||||
(`ParsedArgs::exec_pid`, a positive integer or a hard error) and its
|
||||
trailing-command requirement (`ParsedArgs::command`, required non-empty).
|
||||
Returns an exit code `main()` should return immediately (`0` for `-h`/`-V`,
|
||||
`-k/--kill <pid>` (`ParsedArgs::kill_pid`) shares that same positive-integer
|
||||
parsing/validation via a small extracted `parse_pid_arg()` helper (`.cpp`-local)
|
||||
rather than duplicating the `strtol` dance a second time — unlike `-e/--exec`,
|
||||
it takes no trailing command, so it's simply not added to the leftover-args
|
||||
exemption list (`Mode::run`/`Mode::exec` only). Returns an exit code `main()`
|
||||
should return immediately (`0` for `-h`/`-V`,
|
||||
`1` for any parse error) when set; `nullopt` means `out` is ready for
|
||||
`dispatch_command()`. `-D/--daemonize` (has a short form; `'D'` was free) is
|
||||
a plain boolean flag (`ParsedArgs::daemonize_flag`, set in its own `case
|
||||
@@ -257,7 +262,12 @@ Source layout (all under `src/`):
|
||||
same `on_start` timing — `-D/--daemonize` (`daemonize.{h,cpp}`, see below)
|
||||
hooks in here via `report_daemon_started()` to learn the real pid at the
|
||||
same instant everything else that needs it does, rather than needing its own
|
||||
separate pid-discovery mechanism.
|
||||
separate pid-discovery mechanism. That same `on_start` lambda also calls
|
||||
`create_session_cgroup()` (`session_cgroup.h`, see below), right alongside
|
||||
`create_session_lock()`, so `-k/--kill` can later find every process the
|
||||
session ever starts via its dedicated cgroup; `remove_session_cgroup()` is
|
||||
called from the same post-`run_process_foreground()` spot
|
||||
`release_session_lock()` already is.
|
||||
- `priv_drop_helper.cpp` → the separate `slocker-lite-priv-drop` binary (its own
|
||||
`executable()` target in `meson.build`, **built with `-static`**). Deliberately
|
||||
has zero dependencies on the rest of this project (no fmt/spdlog/etc.) and is
|
||||
@@ -325,7 +335,11 @@ Source layout (all under `src/`):
|
||||
- `pid_file.{h,cpp}` — tracks one running `-r/--run` session (a live `bwrap`
|
||||
process) as a locked pid file, so an outside process (or a later
|
||||
`slocker-lite` invocation) can tell whether it's still running.
|
||||
`session_pid_file_path()` resolves
|
||||
`sanitize_for_filename()` (anything outside `[A-Za-z0-9._-]` → `_`, falling
|
||||
back to `"container"` if that leaves nothing) is exported here (not just
|
||||
`.cpp`-local) specifically so `session_cgroup.{h,cpp}` (see below) can reuse
|
||||
the exact same `<name>-<pid>` naming rule for its own per-session cgroup
|
||||
directory without drifting from this file's own. `session_pid_file_path()` resolves
|
||||
`$XDG_STATE_HOME/slocker-lite/run/<container_name>-<pid>` (falling back to
|
||||
`$HOME/.local/state/...` when `XDG_STATE_HOME` is unset/empty — same
|
||||
resolution pattern as `config_file_path()` below, for state instead of
|
||||
@@ -369,6 +383,76 @@ Source layout (all under `src/`):
|
||||
a check and a later removal. Only files it actually removes are reported back
|
||||
(as `SessionInfo`s with `running=false`); still-locked (running) files are
|
||||
left untouched and not reported.
|
||||
- `session_cgroup.{h,cpp}` — gives `-k/--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
|
||||
group from the moment it starts. `cgroup_v2_available()` checks for
|
||||
`/sys/fs/cgroup/cgroup.controllers` (the same signal systemd's own
|
||||
unified-hierarchy detection uses) — only cgroup v2 is supported; v1 (which
|
||||
splits per-controller into separate hierarchies with no unified
|
||||
`cgroup.procs` at the top) is deliberately out of scope, since this
|
||||
project's real target (Android) has used the unified v2 hierarchy by
|
||||
default since Android 12. `session_cgroup_path()` is deterministic —
|
||||
`/sys/fs/cgroup/slocker-lite/<name>-<pid>/`, reusing `pid_file.h`'s own
|
||||
`sanitize_for_filename()` — so no separate lookup state is needed anywhere.
|
||||
`create_session_cgroup()` is called from `run_bwrap()`'s `on_start` callback
|
||||
(`bwrap.cpp`, see below), the same spot `create_session_lock()` already
|
||||
fires from: `create_directories()`'s the leaf directory (also creating the
|
||||
`slocker-lite/` parent the first time — a plain grouping cgroup, no resource
|
||||
controllers are ever enabled on it via `cgroup.subtree_control`, so the "no
|
||||
internal processes" restriction that comes with actually delegating
|
||||
controllers never applies here) and writes the bwrap pid into its
|
||||
`cgroup.procs`. From that point on, every process bwrap (or anything it
|
||||
execs into) forks inherits this cgroup automatically, permanently —
|
||||
including anything that later daemonizes/double-forks and gets reparented,
|
||||
unlike pid-namespace child membership (only the processes `clone()` itself
|
||||
creates) or process-group membership (many daemonizing services explicitly
|
||||
`setpgid()`/`setsid()` away from it on purpose). Best-effort, mirroring
|
||||
`create_session_lock()`: returns `nullopt` (logging a warning, never fatal)
|
||||
if cgroup v2 isn't available, or the directory can't be created/written
|
||||
(no delegated subtree when running rootless, or an SELinux policy blocking
|
||||
cgroupfs writes even for a root-euid process, are both real, confirmed-by-
|
||||
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
|
||||
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.
|
||||
- `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
|
||||
`exec_session.cpp`, where `resolve_namespace_pid()` originally lived) once
|
||||
`-k/--kill` needed the exact same "find the real sandboxed child" logic, to
|
||||
avoid a second, drifting copy. `resolve_namespace_pid()` is unchanged from
|
||||
its original `exec_session.cpp` form (see that entry for the full
|
||||
reasoning: bwrap's own outer/tracked pid never actually enters the
|
||||
pid/uts/ipc/cgroup namespaces it creates for its clone()'d child, only that
|
||||
child does). Two new utilities added alongside it for `kill_session()`:
|
||||
`pid_namespace_isolated(outer_pid, ns_pid)` compares
|
||||
`/proc/<outer_pid>/ns/pid` and `/proc/<ns_pid>/ns/pid`'s own `readlink()`
|
||||
targets directly — true only when bwrap's `clone()` actually created a
|
||||
separate pid namespace for its child (`--unshare-pid` was requested *and*
|
||||
the kernel supported it), the precondition for the kernel's own guarantee
|
||||
that killing a pid namespace's pid 1 forcibly tears down every remaining
|
||||
process in it. `collect_descendant_pids(root)` generalizes
|
||||
`resolve_namespace_pid()`'s own `/proc/<n>/stat` ppid-scanning fallback to
|
||||
collect a whole transitive tree (root included) instead of just one child,
|
||||
sharing the actual stat-parsing loop between both via a private
|
||||
`build_ppid_map()` (one `/proc` pass, used by both the single-child lookup
|
||||
and the full-tree collection). Reliable specifically when `root` is a
|
||||
genuinely isolated pid namespace's own pid 1: anything that reparents
|
||||
within it (e.g. a daemonizing service) is guaranteed by the kernel to land
|
||||
back on `root` itself, unlike on a kernel without pid namespace support,
|
||||
where it escapes to the *host's* real pid 1 instead (see
|
||||
`kill_session.{h,cpp}` below for exactly this scenario, confirmed on a real
|
||||
target device).
|
||||
- `daemonize.{h,cpp}` — implements `-D/--daemonize`'s fork/detach mechanics.
|
||||
`daemonize(container_name)` sets up a `pipe2(..., O_CLOEXEC)` pair (so it
|
||||
never leaks into `bwrap`/the sandboxed command, same reasoning as the pid
|
||||
@@ -433,22 +517,9 @@ Source layout (all under `src/`):
|
||||
itself never actually enters those namespaces — comparing
|
||||
`/proc/<outer_pid>/ns/{pid,uts,ipc,cgroup}` against this process's own showed
|
||||
them identical, while only `mnt`/`user` differed. `resolve_namespace_pid()`
|
||||
reads `/proc/<pid>/task/<pid>/children` (the direct-children list `procfs`
|
||||
exposes) to find that real inner process and joins *its* namespaces instead.
|
||||
**Real bug found via testing on a real target device, not assumed**: that
|
||||
file requires `CONFIG_CHECKPOINT_RESTORE`, which not every kernel enables —
|
||||
confirmed absent (not just unreadable — the file doesn't exist at all) on a
|
||||
real Android device, where `-e/--exec` then fell back to the outer `bwrap`
|
||||
pid itself and failed outright (`nsenter: no namespace specified`, since
|
||||
every namespace type either matched the outer process's own or couldn't be
|
||||
read at all). Fixed by adding `find_child_by_scanning_proc()`, a portable
|
||||
fallback used only when the children file is missing/empty: scans
|
||||
`/proc/<n>/stat` for any process whose ppid field equals `pid` — the same
|
||||
information `pstree` itself reads to build its tree, which is how the actual
|
||||
sandboxed child was located and confirmed correct on the same device via a
|
||||
manual `nsenter -t <child_pid> -a -- /bin/sh` before the fix was written.
|
||||
Picks the lowest matching pid if more than one child exists, for a
|
||||
deterministic result. For each of `{mnt→--mount, uts→--uts, ipc→--ipc, pid→--pid,
|
||||
(`sandbox_process.{h,cpp}` — moved out of this file once `-k/--kill`
|
||||
needed the exact same logic, see that entry) finds that real inner process
|
||||
so this can join *its* namespaces instead. For each of `{mnt→--mount, uts→--uts, ipc→--ipc, pid→--pid,
|
||||
cgroup→--cgroup, user→--user}` (`net` deliberately excluded — this project
|
||||
never isolates networking, see `bwrap.cpp` below), `readlink()`s both
|
||||
`/proc/<ns_pid>/ns/<type>` and `/proc/self/ns/<type>` and only passes
|
||||
@@ -513,6 +584,85 @@ Source layout (all under `src/`):
|
||||
real `250:250` via the fetched `/etc/passwd` and then fails clearly (helper not
|
||||
bind-mounted, since the session itself had no declared user) rather than
|
||||
silently running as the wrong identity.
|
||||
- `kill_session.{h,cpp}` — implements `-k/--kill <pid>`, stopping a tracked,
|
||||
running `-r/--run` session and everything it started. `kill_session()`
|
||||
validates `pid` the same way `exec_in_session()` does (via `list_sessions()`,
|
||||
`pid_file.h`). **Real bug reported by the user against their own actual
|
||||
target device, confirmed via a captured session log**: a plain `kill
|
||||
<tracked_bwrap_pid>` doesn't kill everything a container started — their
|
||||
`/init` script `php-fpm --daemonize`s (double-forks, detaches) then `exec
|
||||
caddy ...`s (replaces itself); after killing the tracked pid, both `caddy`
|
||||
and the `php-fpm` master+workers kept running as orphans. **Root cause**: on
|
||||
that device, `bwrap`'s `--unshare-pid` isn't actually in effect at all —
|
||||
`detect_bwrap_unshare_args()` (`bwrap.cpp`) only requests `--unshare-xxx`
|
||||
flags the kernel actually supports, and that kernel doesn't support pid
|
||||
namespaces (independently confirmed elsewhere this session, see
|
||||
`exec_session.{h,cpp}`'s own `CONFIG_CHECKPOINT_RESTORE` bug above) — so
|
||||
`php-fpm --daemonize` reparents to the *host's own* pid 1, completely
|
||||
disconnected from the sandboxed session; the classic "kill a pid namespace's
|
||||
pid 1, the kernel guarantees the whole namespace collapses" trick simply
|
||||
doesn't apply there. Per the user's own explicit request (they want to
|
||||
choose the mechanism per host capability, and may need an even more basic
|
||||
one later for some hypothetical older device), `kill_session()` picks
|
||||
between three independently-named strategies, selected dynamically per
|
||||
session (not a single cached host-wide capability flag, since e.g. cgroup
|
||||
creation can fail for session-specific reasons like permissions even on a
|
||||
host that generally supports cgroups) — each runs its own complete
|
||||
`SIGTERM` → wait-up-to-`grace_period_seconds` (10s default, no CLI flag) →
|
||||
forced-`SIGKILL` escalation internally, with no cross-strategy
|
||||
fallback-after-failure chaining:
|
||||
1. `kill_via_cgroup()` — preferred whenever the session has a non-empty
|
||||
dedicated cgroup (`session_cgroup_pids()`, `session_cgroup.h`): `SIGTERM`
|
||||
to every pid currently in it, and, if forcing is needed, either the
|
||||
atomic `cgroup.kill` knob or a fresh re-read-and-`SIGKILL` sweep (fresh,
|
||||
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.
|
||||
**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
|
||||
the pid file's flock, released the moment the tracked outer `bwrap` pid
|
||||
exits, and the `SIGTERM` sweep necessarily hits `bwrap` itself too (it's
|
||||
a cgroup member) — `bwrap` dies and gets reaped in well under a second,
|
||||
long before slower descendants (`caddy` shutting down gracefully,
|
||||
`php-fpm` finishing in-flight requests) actually exit. Gating on the pid
|
||||
file instead would make the poll resolve "done" almost immediately, the
|
||||
forced-kill step would never run, and the original bug would reproduce
|
||||
with unused machinery around it.
|
||||
2. `kill_via_pid_namespace()` — used when no cgroup exists for the session,
|
||||
but `resolve_namespace_pid()`/`pid_namespace_isolated()`
|
||||
(`sandbox_process.h`) confirm `--unshare-pid` was genuinely in effect for
|
||||
it. `SIGTERM`s `collect_descendant_pids(ns_pid)` (reliable here
|
||||
specifically because reparenting within a genuinely isolated pid
|
||||
namespace always lands back on that namespace's own pid 1); if forcing
|
||||
is needed, a single `SIGKILL` to `ns_pid` alone is *guaranteed* complete
|
||||
by the kernel itself, independent of whatever the graceful sweep missed.
|
||||
"Stopped" is simply `kill(ns_pid, 0)` failing with `ESRCH`. Verified
|
||||
end-to-end on this project's rootless dev machine (which does support
|
||||
pid namespaces, unlike the user's real target device): a daemonized
|
||||
busybox session running `sh -c 'sleep 300 & exec sleep 300'` (mirroring
|
||||
the daemonize-then-exec shape of the original bug) was fully cleaned up
|
||||
by `-k/--kill`, including the backgrounded child, with no leftover
|
||||
processes, mounts, or layers; a second run using `sh -c 'trap "" TERM;
|
||||
sleep 300'` (ignoring `SIGTERM` entirely) confirmed the forced-`SIGKILL`
|
||||
escalation path too, taking the full 10s grace period before the pid
|
||||
namespace's own collapse-on-kill guarantee cleaned it up regardless.
|
||||
3. `kill_via_tracked_pid()` — fallback when neither of the above applies:
|
||||
signals the tracked `bwrap` pid directly, `SIGTERM` then `SIGKILL`,
|
||||
polling `list_sessions()` for "stopped" since that's the only signal
|
||||
available without a cgroup or an isolated pid namespace to check
|
||||
directly. Exactly today's manual-`kill` behavior — least complete, but
|
||||
always available, and strictly no worse than before this feature
|
||||
existed. This is the path the user's own real target device actually
|
||||
takes today (no cgroup delegation confirmed working there yet; no pid
|
||||
namespace support at all) — a future, even more basic strategy (for some
|
||||
hypothetical still-more-limited device) would slot in here the same way,
|
||||
per the user's own explicit request to keep this extensible.
|
||||
|
||||
`poll_until()`/`sleep_ms()` (`.cpp`-local) use `nanosleep()` in an
|
||||
`EINTR`-retry loop — matching this project's existing direct-POSIX style
|
||||
(`process.cpp` already retries `waitpid()` the same way) — rather than
|
||||
`<thread>`/`<chrono>` (unused anywhere else in this project).
|
||||
- `config_file.{h,cpp}` — `load_config_file()` reads and parses (via libyaml's
|
||||
document API, `<yaml.h>`) the `global` and `volumes` sections of the local YAML
|
||||
config file located by `config_file_path()` (`$XDG_CONFIG_HOME/slocker-lite/config.yaml`,
|
||||
@@ -628,7 +778,7 @@ Build directory is `buildDir/` (already configured).
|
||||
(see `priv_drop_helper.cpp` in "Project state")
|
||||
- Run the executable: `./buildDir/slocker-lite -m <image.tar>` (see `--help` for the
|
||||
full flag list: `-m/--mount`, `-r/--run`, `-u/--umount`, `-c/--cleanup`,
|
||||
`-l/--list-images`, `-i/--inspect`, `-e/--exec`, `-n/--no-nsenter`, `-D/--daemonize`,
|
||||
`-l/--list-images`, `-i/--inspect`, `-e/--exec`, `-k/--kill`, `-n/--no-nsenter`, `-D/--daemonize`,
|
||||
`--user`, `--group`, `--hostname`, `--env`, `--env-file`, `-v/--volume`, `--list-volumes`, `--delete-volume`,
|
||||
`--delete-volume-full`, `--list-processes`, `--clean-processes`, `-t/--test`, `--log-level`,
|
||||
`-h/--help`, `-V/--version`)
|
||||
|
||||
Reference in New Issue
Block a user