resource: release process slots on exit, not on wait - #230
Merged
Merged
Conversation
fork(2) was only notified when argv safety was required, on the theory that it carries no process-limit risk. It creates a process like any clone, so a raw fork(2) loop was never counted: 30 live children under a limit of 5. glibc's fork() is clone(2) underneath, which is why ordinary programs never showed this. Notify fork(2) unconditionally. Releasing slots on exit also depends on it: an uncounted birth followed by a counted death would drive the count below the real number of processes. Signed-off-by: Cong Wang <cwang@multikernel.io>
A slot was taken when a fork was allowed but freed when a blocking wait4/waitid was entered, assuming such a wait reaps one child. It is neither necessary nor sufficient for a process going away: - Children reaped with WNOHANG, auto-reaped under SIGCHLD SIG_IGN, or orphaned never freed their slot, so a long-lived sandbox eventually refused every fork (issue #229). - A blocking wait that reaps nothing (ECHILD) still freed a slot, so the limit could be bypassed: 40 live children under a limit of 10. Free the slot from the pidfd exit watcher instead, once per process. exit and exit_group are now notified so a child that makes no other notified syscall still registers, and handle_fork polls the pidfds before refusing a fork so it cannot lose a race with the watcher. A child killed by a signal before any notified syscall still leaks its slot (upward only), and zombies no longer count. Signed-off-by: Cong Wang <cwang@multikernel.io>
max_processes was the only resource limit that was on by default: memory, open files, CPU and disk are all unlimited unless set. The default of 64 is easy to hit with make -j, pytest-xdist or a long-lived agent shell, and it fails as a bare EAGAIN from fork. The slot accounting is also best effort (a child killed before any notified syscall leaks its slot), so a default limit exposed every long-lived sandbox to that leak. Issue #229 asked for a way to turn the limit off. Make Sandbox::max_processes an Option, None meaning unlimited, like its siblings. This also drops the `!= 64` sentinel checks, which treated an explicit limit of 64 as unset. The supervisor keeps a plain u32 and uses u32::MAX for unlimited, since 0 is a valid runtime restriction (no more forks). Fork notification is unchanged: it is also needed for namespace flag denial, checkpoint freeze and policy_fn. A default sandbox no longer contains a fork bomb; the docs say so. Signed-off-by: Cong Wang <cwang@multikernel.io>
congwang-mk
force-pushed
the
proc-limit-exit-accounting
branch
from
September 19, 2026 22:04
745a69a to
bd6f9ac
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #229.
Problem
The process limit took a slot when a fork was allowed, but freed it when a blocking
wait4/waitidwas entered, on the assumption that such a wait reaps exactly one child. Entering a wait is neither necessary nor sufficient for a process going away, so the count drifted in both directions.Reproduced on main (0.8.8):
WNOHANG(the report)SIGCHLDset toSIG_IGN, kernel auto-reapswaitpid(1, 0)(ECHILD) before each forkfork(2)syscall on x86_64The first three make the limit a lifetime fork budget, which is what the reporter hit with a long-lived agent. The last two let a sandbox run past the limit. Raw
fork(2)was never counted because it was only notified when argv safety was required; glibc'sfork()isclone(2), so ordinary programs never showed it.Fix
Three commits:
fork(2). It is now notified unconditionally, so every process creation reserves a slot.ProcessIndexkeyed byPidKey. A pidfd turns readable however the process dies, so_exit, signals and the OOM killer are all covered. The slot belongs to the thread-group leader, so registering a thread also registers its leader.Two gaps had to be closed for exit-based release to work:
exitandexit_groupare now notified, so it registers on its way out whilepidfd_openstill succeeds.handle_forkpolls the slot-holding pidfds before refusing a fork. The kernel marks the pidfd readable before the parent can observe the death, so this is deterministic.handle_wait, its dispatch entry and the BPF carve-outs for non-blocking waits are removed. No ptrace, user namespace or cgroup is involved.max_processeswas the only resource limit on by default (64); memory, open files, CPU and disk are unlimited unless set. It is now anOption,Nonemeaning unlimited, which is what the issue asked for. Unset also stays clear of the known limits below. The supervisor usesu32::MAXinternally, since 0 is a valid runtime restriction. A zeroedMaxProcessesin Go now means unlimited, the same as a zeroedMaxMemory. Fork notification is unchanged (namespace flag denial, checkpoint freeze andpolicy_fnneed it). A default sandbox no longer contains a fork bomb; the reference docs say so.Known limits
openat,close,socket,cloneare always notified) and the leak only pushes the count up, so it cannot be used to get past the limit.Continue(ENOMEM, host RLIMIT_NPROC) also leaks a slot, as it did before.Tests
test_resource.rs, each forking more children in sequence than the limit allows at once:WNOHANGreaping,SIGCHLDauto-reap, orphaned grandchildren, concurrent short-lived children, the ECHILD wait loop (must stay at the limit), and rawfork(2). TheWNOHANG, auto-reap, orphan and ECHILD tests failed before the fix.max_processes=2), which exercises the at-limit pidfd poll.test_process_limit_unset_is_unlimitedholds 100 live children in a sandbox with no limit set.test_policy_fn.py,test_sandbox.py,test_sandbox_config.pyandtest_profile.pypass (200); Go tests pass. The exit-accounting tests passed 36 consecutive runs. The rest of the Python suite was not run locally.🤖 Generated with Claude Code