ceamac
e047d243f2
create_session_cgroup() used to be called from run_bwrap()'s on_start callback, in the parent, concurrently with the just-forked child execing into bwrap and bwrap then doing its own internal clone() of the sandboxed target. Without --unshare-pid, bwrap has little enough setup work to do that it could reliably win that race, cloning its target before the parent's own write into cgroup.procs completed -- leaving that target, and everything it later spawns, permanently outside the tracked cgroup, so the post-exit sweep found nothing to reap. Found via the user's own request to test "pid namespace off, cgroup on" as root: confirmed directly by inspecting cgroup.procs mid-session, showing only bwrap's own pid. Fixed by giving run_process_foreground() a new before_exec hook, invoked in the child synchronously right before execvp() -- the child cannot proceed to exec (and thus cannot trigger any of bwrap's own internal forking) until this has already returned, closing the race structurally rather than by timing luck. run_bwrap() now creates the session cgroup there instead of in on_start; the parent side just reconstructs the deterministic path unconditionally, since the downstream sweep/cleanup functions already tolerate a nonexistent directory gracefully either way. Also fixes a false positive found while verifying this: the regression test's own process-matching did a substring search across a whole cmdline blob, which matched an unrelated manual `pkill -f 'sleep 137'` diagnostic command run by hand during the investigation. Tightened to an exact argv[0]/argv[1] match. Finally, the regression test now SKIP()s (instead of failing) when neither a pid namespace nor a working session cgroup is available for the current effective config -- a documented, known residual limitation, not a regression -- checked directly via two new helpers rather than assumed from e.g. geteuid().
8.2 KiB
TODO
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, 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), 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.
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 -- 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 <unshare-pid: false>) 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
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
Found on the real target device (a Gentoo chroot on Android), two separate
ways: (1) running slocker-lite from different working directories once
produced two separate state trees -- /root/.config/slocker-lite +
/root/.local/state/slocker-lite vs. /root/src/slocker-lite/.config/... +
.../.local/state/... -- so two independently-invoked processes had
disjoint config.yaml/session/network state and both auto-allocated the
same 10.168.0.0/24 subnet, stepping on each other's host-level
ip/iptables state; (2) later, network_dns.cpp's dnsmasq failed outright
with failed to open pidfile .local/state/slocker-lite/dns-resolvers/...: No such file or directory -- a visibly relative path, not the expected
absolute one.
Root cause, confirmed by the user: HOME was set in the invoking bash
session but never exported (declare -p HOME showed declare -- HOME="/root", not declare -x HOME="/root") -- a non-exported shell
variable is visible to the shell itself (prompts, tab completion, declare -p) and to anything that reads it back via the shell's own variable table,
but is not part of the environment execve() passes to a child process,
so getenv("HOME") in slocker-lite itself returned nullptr. A first
environment dump the user pasted to help debug this showed HOME=/root
and looked like it ruled this out -- it didn't, since that dump was a
declare -p-style listing of all shell variables, not strictly the
exported environment a child process actually receives.
Fixed: xdg_state_dir()/config_file_path() (pid_file.{h,cpp}/
config_file.cpp) now fall back to the passwd database entry for the
current uid when $HOME itself is unset (resolve_home_dir()) -- exactly
this scenario -- and both additionally make their own final return value
absolute (std::filesystem::absolute()) regardless of which piece was
relative, so a relative $XDG_STATE_HOME/$XDG_CONFIG_HOME override would
be caught the same way even though it wasn't the actual cause here.
Security: run the DNS resolver (dnsmasq) as a low-privilege user
start_dns_resolver() (src/network_dns.cpp) currently runs dnsmasq with
--user=root --group=root, explicitly overriding dnsmasq's own default
privilege-drop behavior. This was done because dnsmasq's default drop-to-
unprivileged-user broke reading $XDG_STATE_HOME (typically
/root/.local/state/slocker-lite/..., mode 0700 -- unreadable/
untraversable by a non-root user) for its own --hostsdir/--addn-hosts
files, causing every query to come back REFUSED. Keeping dnsmasq at root
was the fastest correct fix, but it's a real, deliberately-accepted
regression from dnsmasq's own security posture: this project's DNS resolver
process now runs as root for its entire (per-session) lifetime, with no
privilege drop at all, purely to work around a directory permissions
mismatch.
Future direction: let dnsmasq actually drop to a low-privilege user, and
instead make the files it needs to read reachable by that user -- e.g. move
(or additionally expose) the relevant per-network dns-hosts/,
dns-internal-hosts/, and dns-resolvers/ state directories somewhere a
non-root user can traverse into (not nested under /root), with permissions
scoped narrowly to just what the resolver needs. Needs some design thought
about where that location should live consistently with this project's
existing $XDG_STATE_HOME conventions, and whether the rest of this
project's own state should stay root-only or move too. Not urgent -- this
project's networking is already root-only in every other respect (bridges,
iptables, etc.), so this is a narrower, lower-priority hardening pass for
later, not a blocker for anything currently in progress.