control: free a sandbox name even when the host forks or spawns - #243
Closed
congwang-mk wants to merge 2 commits into
Closed
congwang-mk wants to merge 2 commits into
congwang-mk wants to merge 2 commits into
Conversation
A sandbox name is an abstract unix socket, bound while any fd refers to it. fork() copies every fd, and sandlock closed the copies only in the children it forks itself. A host that forks on its own, os.fork() or a multiprocessing worker in Python, got a child that holds the name of every live sandbox and may never exec, so SOCK_CLOEXEC never fires and the names stay taken for as long as that worker lives. Do it from a pthread_atfork handler instead, which runs in every fork() child of the process. The handler puts an unnamed socket on each control fd's number rather than closing it: the child still has copies of the ControlFd values, and their drop must not close whatever reused the number. The sandbox child keeps its pgrp socket as before. Signed-off-by: Cong Wang <cwang@multikernel.io>
wait() promises that its sandbox name can be reused the moment it returns, and it closed the control sockets to keep that promise. That is not enough when the host spawns processes of its own. A child made by vfork, posix_spawn or a raw clone, as Command and Go's os/exec make them, copies every fd and holds the copies until it execs. libc's fork handlers do not run for it, and until its exec the abstract names stay bound, so running the same name again failed with "sandbox is already running". Under the parallel integration suite this broke the test_control name reuse tests in about one run out of four. Nothing can stop such a copy from existing, but the kernel says exactly when the last one is gone: a connection sitting in a listener's backlog hangs up when the listener itself is released, which takes every fd that refers to it, in any process. So before closing its sockets, release leaves one connection in each backlog, then waits for the hangups. With no copy around they arrive at once; with a spawning child around, wait() returns as that child execs. The sockets move behind an Arc so that they survive the control loop and release can still reach them. Signed-off-by: Cong Wang <cwang@multikernel.io>
Contributor
Author
|
Superseded by #244. The wait added here returns only when the last copy of the control sockets is gone, so a host child that never execs (made by |
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.
A sandbox name is a pair of abstract unix sockets, and an abstract name stays bound while any fd refers to it.
fork()copies every fd, and sandlock only closed the copies in the children it forks itself. A host that spawns processes of its own therefore holds the name of every live sandbox in each child until that child execs or exits, and reusing a name in that window fails withsandbox '<name>' is already running.Under the parallel integration suite this broke
test_control_name_is_free_when_wait_returnsandtest_control_parked_child_does_not_pin_other_namesin about one run out of four: other tests spawnsandlock psin polling loops, and each spawn sits between fork and exec for a moment with copies of every control fd. It is a product bug as well: any embedder that spawns processes while reusing a sandbox name can hit it.The two cases
A
fork()child that may never exec (os.fork(), a multiprocessing worker).SOCK_CLOEXECnever fires, so the name would stay taken for the worker's whole life. Apthread_atforkchild handler now puts an unnamed socket on every control fd's number, in everyfork()child of the process, not only sandlock's own. It usesdup3rather thanclose: the child still has copies of theControlFdvalues, and their drop must not close whatever reused the number. The placeholder socket is made just before the fork and closed by both sides right after, so sandlock keeps no standing fd in the host. The sandbox child keeps its pgrp socket as before.A child made by vfork, posix_spawn or a raw clone (
Command, Go'sos/exec). libc's fork handlers do not run for it, and nothing can stop the copy from existing until its exec. But the kernel says exactly when the last copy is gone: a connection sitting in a listener's backlog hangs up when the listener itself is released, which takes every fd that refers to it, in any process. Sowait()now stops the control loop, leaves one unaccepted connection in each listener's backlog, closes the listeners and awaits the hangups. With no copy around they arrive at once; with a spawning child around,wait()returns as that child execs. No polling, retry or timeout, and no extra thread.The control sockets move behind an
Arcso they survive the control loop andreleasecan still reach them.Dropcannot await and keeps its current behavior: the name is free soon, not at once.Tests
test_control_wait_outlasts_a_spawning_childis deterministic: a rawclonechild (no fork handlers, like vfork) holds the copies for 300 ms while the name is reused right afterwait(). It fails with the production error when the hangup wait is removed.a_fork_made_by_the_host_holds_no_name(a plainlibc::fork()child holds no name, and the fd numbers stay occupied) andsandbox_child_keeps_only_its_pgrp_name.Seen while testing, not addressed here
test_forkclones can hang on a futex. A stuck run on clean main looked the same, but it was not confirmed as a hang. Unverified guess: the COW fork clones callstd::env::set_varafter a fork from the threaded test process, and std holds its env lock across everyCommand::spawn.write ready signal: Broken pipefails a test now and then (test_restore_glibc_vdso_program_resumes,test_gather_disjoint_policies), also on main.🤖 Generated with Claude Code