Repository navigation
linux: a placeholder is removed only when no running sandbox relies on it - #584
ronleizrowice-ant wants to merge 68 commits into
Conversation
…rnel say so A deny on a path that does not exist needs a mount point on the host, and unlinking one while a sandbox is still bound over it detaches that mount inside the sandbox: the denied path can then be created, and what is written there lands on the host. So a mount point may only be removed when no running sandbox relies on it. Nothing a process counts for itself can decide that. A second srt process sees another's live mount point as a leftover and takes it away when it exits; a caller that crashes, cleans up early, twice, or never, each gets a different answer. Ask the kernel instead. Each wrap writes a manifest naming the mount points it relies on into a per-user runtime directory and points bubblewrap's --lock-file at it: bubblewrap's sandbox init opens that path and holds an fcntl read lock on it for exactly the sandbox's lifetime, which the kernel drops however the sandbox ends, SIGKILL included. /proc/locks then answers, for any process and at any moment, whether a sandbox that named a mount point is still running. The lock belongs to the sandbox's init process, not to the command: bubblewrap opens the manifest with O_CLOEXEC, so the descriptor does not survive the exec, and a POSIX lock is only dropped by the process that took it. Every error reads as "still in use": an unreadable /proc/locks, a directory that cannot be listed, a manifest that cannot be read while a sandbox holds its lock, and a manifest younger than the moment bubblewrap takes to reach that lock.
wrapCommandWithSandboxLinux() named every mount point it made in a set in
this process's memory, and cleanupBwrapMountPoints() counted the wraps it had
been told about and emptied that set when the count reached zero. The count
only ever described this process, so a second srt process took away a live
sandbox's mount points, and a caller that crashed, cleaned up early, twice or
never broke the count itself.
Each wrap now names its mount points in a manifest and passes
--ro-bind <dir> <dir> --lock-file <manifest> to bubblewrap, and the cleanup
is a garbage collect against those manifests: it is safe from any process, at
any time, any number of times, and it also takes away what other srt
processes have finished with. The counter and the exactly-once contract are
gone; cleanupBwrapMountPoints() keeps its signature, and `force` no longer
changes what is removed.
The manifest directory is bound read-only after every allow and deny bind, so
a sandboxed command can neither delete nor rewrite a manifest, and so
bubblewrap can open one whatever the profile has mounted over its directory.
A command wrapped but not started when a cleanup runs no longer keeps its
mount points: the cleanup releases them, and bubblewrap then refuses to start
that command ("Unable to open lock file") rather than running it with a mount
point another call has taken away.
The new suite drives the cases a count in one process's memory could not answer: two concurrent processes, where the second's cleanup used to take the first's live mount point away and the survivor could then create the denied path; a cleanup that comes early and twice while the sandbox runs; a process killed before it can clean up, whose manifest and mount point the next cleanup collects; a sandboxed command that tries to delete, plant and rewrite manifests; twelve wrap-and-run rounds against a process collecting in a tight loop; a command wrapped before the cleanup that released it; and a wrap that threw letting go of the mount points it had named. Five of the seven fail on the previous behaviour. Two tests in stale-mount-point.test.ts described the counter: one expected a mount point to wait for a second wrap that had produced no running sandbox, which the kernel now answers instead, and one collected a killed process's mount point before the grace that covers a sandbox still starting.
The section described the mount points as removed after the command and told embedders not to clean up after a wrap that threw. Both were the old contract: a mount point goes when no running sandbox relies on it, whichever process asks, and the cleanup is safe to call at any time.
A wrap held a token so that one cleanup could not release another's deferral. It does not hold: release() calls cleanupAfterCommand() without a token, so a doubled cleanup still frees whichever wrap is oldest, and nothing held in one process's memory can see a second srt process adopt a live mount point and unlink it on the way out. The README and the commit message described a guarantee the code did not give. Ask the kernel instead. Each wrap names the mount points it relies on in a manifest and hands bubblewrap a --lock-file on it, so the sandbox init process holds an fcntl lock for exactly the sandbox's lifetime and /proc/locks answers, from any process, whether a mount point is still in use. That is #584, and it makes cleanup an idempotent garbage collect with no counter and no pairing to keep, so this change should not add public API for the pairing either. The cleanup path goes back to what it was before: the counter, cleanupBwrapMountPoints() with no wrap to name, and no wrapWithSandboxScoped(). Four invariants remain in this branch.
…fests
Commands in parallel: cleanupBwrapMountPoints() released every manifest
of the process, so the first of two overlapping commands to finish took
the mount point from under the other's running sandbox. The process now
counts its wraps and releases nothing of its own until cleanup has been
called once per wrap, as on main. cleanupAfterCommand({ commandId })
releases that one command's manifest at once for callers that have an
id per command.
Also in this commit:
- The per-user manifest directory is created and checked through a
descriptor opened without following links, and its mode is set on the
descriptor, so a link planted at the name is not followed or chmodded.
- A manifest is written under a temporary name and linked into place,
so a collector never reads half of one; leftovers are swept.
- The collector reads the directory again after taking its lock, and
the lock is created atomically and given up when its holder dies.
- A manifest records its PID namespace; one from another namespace is
always treated as live, because its pid means nothing here.
- A mount point is removed only when it is still on the device it was
created on and still looks like bubblewrap's work.
- A removal that is refused keeps the manifest, so a later pass retries.
- The manifest directory is bound read-only into the sandbox under both
of its names, with --ro-bind-try so a missing one is not an error.
- Every entry point returns at once off Linux.
…checks A new suite drives the collector directly: overlapping commands with and without a commandId, a planted link at the manifest directory, a lock held by a dead process, a manifest from another PID namespace, a mount point replaced by something on another device, and a refused removal. The three existing suites are updated for the wrap count and for --ro-bind-try on the manifest directory.
The Linux section says what cleanupAfterCommand() releases when commands overlap, what commandId is for, and where the manifest directory lives.
…own directories are pinned Six changes to how mount points are recognised, kept and protected. A wrap names an existing directory at a deny path whenever any manifest names it, a finished one as much as a live one. It used to ask only the live ones: after a killed process left an empty directory and its manifest, a later wrap that denied that directory bound it without naming it, and the next clean-up removed it from under the running sandbox. An existing empty read-only file at a deny path is a mount point only when a manifest names it. It used to be taken for a leftover by its shape alone, so a file the user keeps empty and read-only there was covered, named and removed after the command. One that no manifest names is now bound read-only where it is and never removed. A mount point left by a killed process of a release that wrote no manifest therefore stays until someone deletes it. The manifest directories, the store of fake files and the empty directory that directory mount points bind from are seeds of the ancestor pins. With the temp dir strictly inside a write root a command could rename it aside, make the name again with a manifest directory of its own, and have the next clean-up in another process remove whatever a forged manifest named. The same three are bound where they really are. bubblewrap makes a bind's destination by name inside the new root, so with the temp dir reached through a link whose target is absolute no sandbox that restricts writes started. A manifest is read only if it is a regular file of the user's of at most 1 MiB, through a descriptor opened without following a link and without blocking. liveMountPoints() is exported from the package root: a snapshot of the mount points a sandbox that may still be running relies on, each checked to still be an empty placeholder on disk, for a caller that removes paths of its own accord after a command. It makes nothing on the host.
…s, a linked temp dir Real bubblewrap runs for each: a directory mount point stays while a sandbox that denies it runs, with the process that made it killed and another cleaning up; the temp dir cannot be moved aside from inside a sandbox to plant a manifest directory; the user's own empty read-only file is unwritable during the command and still there after it; a sandbox starts with the temp dir behind a link, absolute or relative, inside or outside the write root, with a directory mount point and a store of fake files as well. The cases that planted an empty read-only file without a manifest and expected it collected now write the killed process's manifest beside it, which is what such a process leaves.
…pinning cannot hold
… environment Two processes of one user keep each other's mount points only when each reads what the other writes. Where the manifests went followed XDG_RUNTIME_DIR and TMPDIR, which differ between a login shell and what a service, an IDE or cron starts: such processes did not see each other, the second took the first's mount point for the user's own file, and the first's clean-up removed it from under the second sandbox. The candidates are now /run/user/<uid>/srt-mount-points, where /run/user/<uid> is a real directory of the user's alone, then /tmp/srt-mount-points-<uid>, and only then the two names the environment gives. A process writes to the first it can use. It believes the manifests under the first two, and under the environment's names only when it can use neither of the first two, which is when it writes there, and in the directory it has settled on. What the environment names is kept out of a sandbox's reach only by processes with that environment, so a manifest found there is not taken on trust by a process that has somewhere else to look. Every candidate is still made when missing, bound read-only and pinned by every wrap that restricts writes, whether or not the process writes there: a command that may write /tmp would otherwise make the directory itself and be believed. A clean-up makes one pass per directory under that directory's lock, and a live manifest in any of them keeps a path.
…touches the user's own manifest directories Real bubblewrap runs with two real processes, in a mount namespace where a private directory stands at /tmp and at /run/user: the deny holds in a second sandbox after the first has ended, with different XDG_RUNTIME_DIR and with different TMPDIR; a command that may write /tmp can neither make, write nor move aside the directory under it; a manifest forged under one process's TMPDIR by the sandbox of another gets nothing removed. bunfig.toml preloads stand-ins for the four places before any test file is loaded, and children are handed a module or a program that does the same, so no suite reads, writes or collects where the user's own sandboxes keep their manifests. Several did.
Comments only: the JavaScript emitted with comments removed is byte-identical for every file, and the test names are unchanged. Kept: every invariant and what a sandboxed command could do without it, the documentation of exported names, and the reasons for what is not done the obvious way. Cut: how the code came to be as it is, narration of the lines that follow, one explanation repeated at several call sites, and the arithmetic behind constants.
Comments only: the JavaScript emitted with comments removed is byte-identical for every file, and the test names are unchanged. The same standard as on the branch this one is stacked on, applied to the comments this branch rewrites and adds.
…ifest-directory-from-uid
…irectory lock Whether a sandbox still ran was read from /proc/locks, which is not a snapshot: it is read a page at a time, and a sandboxed command that holds a few hundred locks and churns one makes a held lock vanish from a reading. Whenever only the lock vouched (an extra clean-up call, reset(), the wrapping process gone), the mount point was then removed and the denied write landed. The wrapped command now records itself before bubblewrap starts: `sh -c 'read -r s </proc/$$/stat && printf ... >>"$1" && shift && exec "$@"'` appends the shell's own stat line, and the exec makes that pid and start time bubblewrap's. A manifest is live while a process on its record exists. Whatever cannot be read or asked counts as live. The manifest is the first thing bound after the root, so a command started after a clean-up has taken its manifest fails before bubblewrap makes anything. A clean-up renames a finished manifest to a claim, reads every manifest again, and removes a path only when every manifest naming it is claimed with no process on its record; it gives back a claim it made when that reading finds one. It takes no lock and waits for nothing: `--lock-file`, /proc/locks and directory.lock are gone, and with them the two-second stalls a stale lock caused. A manifest that can be recorded nowhere is kept in memory and removed by the process that made it. README: what is left (two wraps of one path and a clean-up held up between a listing and a removal).
…deny path made meanwhile One listing of the manifest directory was good for a quarter of a millisecond, so in a pass over ten paths the last was removed on a listing up to that old, not a few microseconds old as the README said. A pass of up to 64 candidates now lists before every removal; a larger one keeps the clock, so a directory of thousands is not listed thousands of times. The deny loop asked whether a component of an absent path is a file of the path itself too. Another sandbox's bubblewrap can make its mount point there between the two looks, and the path then got no bind for the whole command. It asks about what lies above the path.
|
/approve 6f6be30 |
…options directoryFailureLogged could not be true where it was tested. collectMountPoints defaulted to releasing every manifest of the process, for the tests' sake; no caller in src relied on it. isOurPrivateDirectory took three booleans in a row. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SEZ8LUnnuZdEgiFocuvVDm Co-authored-by: ronleizrowice <ronleizrowice@anthropic.com>
Each declared its own manifest writer (five copies), dead pid, stat parser (six), placeholder, started record, wait loops (eight), wrap and run, and the liveness suite spelled the wrap of a child process out eleven times. One module, test/helpers/mount-point-fixtures.ts, holds them now. No case changes. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SEZ8LUnnuZdEgiFocuvVDm Co-authored-by: ronleizrowice <ronleizrowice@anthropic.com>
The module's header retold what collect() states as invariants and the README tells in full; it now says what each file in the directory is and where its rules are. liveMountPoints() repeated its README section. One invariant of the wrap stood above the publishing and is about the binds. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SEZ8LUnnuZdEgiFocuvVDm Co-authored-by: ronleizrowice <ronleizrowice@anthropic.com>
Its comment gave only the lesser reason, that it spares a waiting wrap a second plan, which reads as if the record of a clean-up had made it redundant. It has not, for a clean-up that no wrap can see and for a directory made since. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SEZ8LUnnuZdEgiFocuvVDm Co-authored-by: ronleizrowice <ronleizrowice@anthropic.com>
…mbie A shell may wait for a child that ends before it execs: in 8 of 300 runs of `true & echo $!; exec sleep 600` the child was gone and no zombie, and the case then failed on ENOENT from /proc. The child now ends once the shell has become the program that never waits. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SEZ8LUnnuZdEgiFocuvVDm Co-authored-by: ronleizrowice <ronleizrowice@anthropic.com>
On a 9p drive the plan makes the mount point itself. A wrap that was refused because the manifests could not be read had made it by then, and the clean-up at exit, in the same doubt, removes nothing: it stayed, with nothing to name it. The cases that hold the tracking of what a plan has made now raise the doubt as the mount point is made, and cover one that is made as a directory. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SEZ8LUnnuZdEgiFocuvVDm Co-authored-by: ronleizrowice <ronleizrowice@anthropic.com>
…process is cleaned up after A wrap whose manifest could not be written (a full disk) keeps its mount points in memory. A second wrap that denied the same path named it in a manifest, and the clean-up after that second command removed it, under the sandbox of the first. What is kept in memory is now kept by every pass but the one that releases everything, and a published manifest takes out of memory only what a plan had made itself. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SEZ8LUnnuZdEgiFocuvVDm Co-authored-by: ronleizrowice <ronleizrowice@anthropic.com>
… with two names is bound once A wrap matched the clean-ups it waits for by their full paths. Where two of the places are one directory, which name that goes by can differ from one listing to the next; the record then seemed gone, and the command was handed out with the clean-up still under way. The same two names were both bound, at one destination. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SEZ8LUnnuZdEgiFocuvVDm Co-authored-by: ronleizrowice <ronleizrowice@anthropic.com>
Each could be deleted with every suite green: the guard of liveMountPoints() off Linux, whose only case went with the duplicates; two of the three looks at a manifest directory that now throw; and the clean-up at exit of a process whose only wrap made the directory its mount points bind from and then came to nothing. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SEZ8LUnnuZdEgiFocuvVDm Co-authored-by: ronleizrowice <ronleizrowice@anthropic.com>
…es everywhere The one was still spelled out 32 times beside the fixture for it, which nothing used, and the other twice. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SEZ8LUnnuZdEgiFocuvVDm Co-authored-by: ronleizrowice <ronleizrowice@anthropic.com>
They took the number of open descriptors after a first, small wrap as their baseline. The runtime closes the two sockets of a child that has ended, which is ripgrep here, on a thread of its own: in 9 of 60 runs one or both were still open at that moment, and every count after it came out too low. One of the three cases is on main as it was here. Sockets are no longer counted, nor what has been closed since the listing. 180 runs of a case without a failure; a profile left open at either place that closes one still fails them. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SEZ8LUnnuZdEgiFocuvVDm Co-authored-by: ronleizrowice <ronleizrowice@anthropic.com>
Since a plan reads the manifests first, it read them before it had made the places that follow from the user id. Until one of those can be written, what the environment names is believed. So the first wrap of a user - was refused for what is no manifest in a directory that no process believes a moment later; - took the user's own empty directory for a mount point on the word of a stale or forged manifest there, named it in its own manifest, which every process believes, and had it removed by the next clean-up. A wrap with an empty file at a denied path read that early before, and had both. The places are now made at the head of the plan. Where the wrap's own manifest goes is settled before the pins, as it was. In the tests: two hooks that went by the first call of a kind now go by the call they mean, the suite on unmakeable mount points has a manifest directory of its own, a wrap in doubt from the start is shown to make no directory to bind from either, and the README says no more than is so. What a wrap was refused with is asked by one helper. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SEZ8LUnnuZdEgiFocuvVDm Co-authored-by: ronleizrowice <ronleizrowice@anthropic.com>
…her wrap could not record What is kept in memory is kept by every clean-up but the one that releases everything. Two things followed that should not have: - The directory that mount points bind from stayed in memory after a manifest named it. A clean-up after one command kept it and dropped that manifest, so from then on only this process knew of it, and killed, left it for good. A published manifest now takes it out of memory like a mount point the plan made. - A plan that made a mount point marked it as its own though an earlier wrap, which could record it nowhere, relied on it. The manifest then took it over, and the clean-up after that command removed it under the earlier sandbox. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SEZ8LUnnuZdEgiFocuvVDm Co-authored-by: ronleizrowice <ronleizrowice@anthropic.com>
Since the places are made at the head of a plan, the call before the pins did one thing more: it settled where this wrap's manifest would go, so that a directory of the process's own was pinned in the same plan. No case held that, and the wrap is right without it. A manifest that went to a directory the pins do not cover has the wrap plan again, which a case does hold, and the next plan finds the directory settled. So the second call goes, and the `recording` parameter with it. A process that can write none of the shared places plans its first recording wrap twice. Every other wrap saves the second look at the five places: with twenty manifests there, `lstat` 66 to 57, `open` 28 to 24, `access` 6 to 3. The hook of the cases that act between the deny loop and the pins counted calls of `mkdirSync`. It is keyed on the one question a plan asks there. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SEZ8LUnnuZdEgiFocuvVDm Co-authored-by: ronleizrowice <ronleizrowice@anthropic.com>
Brought in by the commit before last. Once a manifest named the directory that mount points bind from, the process no longer kept it in memory. So with that manifest lost, to a cleaner of temp dirs say, no clean-up of the process removed the directory, and every later wrap went on binding from it, also after the temp dir had changed. It is this process's own: a clean-up with none of its commands outstanding takes it for removal, whatever the manifests say. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SEZ8LUnnuZdEgiFocuvVDm Co-authored-by: ronleizrowice <ronleizrowice@anthropic.com>
…spelled out
What is no JSON is in doubt for half a second only, so a case that plants
it and is slow to wrap would see no refusal. `{}` is in doubt for an hour.
Two more captures of a refusal use the fixture, and a doc comment is back
on the function it describes.
Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SEZ8LUnnuZdEgiFocuvVDm
Co-authored-by: ronleizrowice <ronleizrowice@anthropic.com>
f203eb6 was wrong to cut that. It relied on the wrap planning again when its manifest went to a directory the pins do not cover, and that is asked of a manifest. Where none can be written, on a full disk say, there is none to ask it of: the command went out with the process's own directory, just made, neither bound nor pinned. With the temp dir in a write root the command could write a manifest there, and the host's next clean-up removed an empty file outside every write root on its word. It also had such a process plan twice after every forced clean-up, which removes that directory, and not once in its life as that message said. And a wrap given up between the two plans left the directory behind at exit. So it is settled before the pins as it was, and listed with the others. Without the second look at the shared places, which is what cost: with twenty manifests there, `lstat` 66 at fdce419, 58 now, `open` 28 and 24. The case is what was missing: neither shared name will do, and the wrap publishes once and binds the directory, with the manifest written and with the disk full, also after a forced clean-up. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SEZ8LUnnuZdEgiFocuvVDm Co-authored-by: ronleizrowice <ronleizrowice@anthropic.com>
This reverts commit 8997f0d. It took the directory for removal at every clean-up with no command outstanding, for as long as the process remembered the name. So where the name outlived the directory, or a sandbox still running kept it, each later clean-up went on record and read everything, with nothing to remove, and other processes' wraps waited for it: ten records in five idle clean-ups, none before. It removed the directory from under a running sandbox whose manifest and record were both lost. And it made two cases pass by itself that hold the tracking from when the directory is made. What it mended is a case of a limit the README states: what a lost manifest named stays. For this directory less than that. The next wrap of the process that binds from it names it again, and it goes with that manifest. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SEZ8LUnnuZdEgiFocuvVDm Co-authored-by: ronleizrowice <ronleizrowice@anthropic.com>
It asks three times whether what is no manifest is in doubt, and planted what is so for half a second. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SEZ8LUnnuZdEgiFocuvVDm Co-authored-by: ronleizrowice <ronleizrowice@anthropic.com>
…be written Older than this round: reproduced at fdce419, and the two conditions it hangs on read the same in the head that was approved. A wrap settles where its manifest goes before the pins only when it has mount points of its own. One that relies on another sandbox's alone learns that after the pins, which are among what it relies on, and settles as it publishes. Written, the manifest has the wrap plan again. Not written, on a full disk say, there is no manifest to ask that of, and the command went out with the directory, just made, neither bound nor pinned: with the temp dir in a write root the command could write a manifest there, and the host's next clean-up removed an empty file outside every write root on its word. So it is closed where the directory is made, whichever way a wrap came there: a directory only this process knows, and with nothing in it, is removed again when the manifest it was made for cannot be written. One that holds an earlier manifest stays, and was settled before this plan began, so this plan has bound it. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SEZ8LUnnuZdEgiFocuvVDm Co-authored-by: ronleizrowice <ronleizrowice@anthropic.com>
Each is bound where it really is, and last, on top of every deny. The plan makes sure at its head that each is a directory and no link, and asked where it really is only at the pins, the whole deny loop later. A name swapped for a link to the home directory in between had that bound last, on top of a read deny under it, and the command read what was denied. Since fcd46bc for a wrap with no mount point, since f203eb6 for every wrap: before those, the look came right before the pins. Only what the environment names can be swapped, and only by a process of the user's that has not got it bound, such as a sandbox of a process with another temp dir. Now it is asked at once. A name swapped later is bound by that name, and bubblewrap refuses to mount on a link. The hook of the cases that act before the pins hung on that question. It is keyed on the same question about the directory mount points bind from, so the one case whose first plan had none has one more deny. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SEZ8LUnnuZdEgiFocuvVDm Co-authored-by: ronleizrowice <ronleizrowice@anthropic.com>
…it asks of a wrap Exported since 260e0dc, it was the one function here that makes something on the host without asking for the platform first. Its callers did that for it until then. Its doc comment says what a caller takes on, and the one that lists what a wrap binds points to it. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SEZ8LUnnuZdEgiFocuvVDm Co-authored-by: ronleizrowice <ronleizrowice@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SEZ8LUnnuZdEgiFocuvVDm Co-authored-by: ronleizrowice <ronleizrowice@anthropic.com>
`src/` is byte for byte what it was there. That is the last head a review found free of regressions in the wrap path. Its nine findings were small, and each of my three rounds of answers to them brought in worse: - f203eb6 cut the second look at the manifest directories, and with it the settling of a directory of the process's own before the pins. On a full disk that directory went out neither bound nor pinned. - 8997f0d made every later clean-up a full pass in three states. Reverted in b936bc7 already. - 260e0dc settled again, without the look. A name swapped for a link during the deny loop was then followed, for every wrap. - 5bb12e9 removed a directory that could not be written, also one that commands already handed out bind, so that the next one made was in their reach. On a disk that takes no directory either, the process recorded nothing for the rest of its life. - 57a7678 resolved the directories at the head of the plan. One that cannot be resolved then counted as a guess against every placeholder the plan would have skipped, and the command could not start. All of it lies in some twenty lines, in states that are rare: no shared name can be used, the disk is full, or another process of the user's races the plan. CI was green on every one of these heads. What stays is tests. Junk that is in doubt for an hour and not for half a second, the refusal fixture, a hook that no longer counts calls, and two cases for what the second look does, which no case held: a directory of the process's own is bound by the plan that settles on it, written or not, and a link put at a shared name during the plan is not followed. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SEZ8LUnnuZdEgiFocuvVDm Co-authored-by: ronleizrowice <ronleizrowice@anthropic.com>
Its swap hangs on the wrap making the directory that mount points bind from, which a wrap does only with none left from an earlier case. In file order the case before it had cleaned up, under a filter or in a random order it failed. It now removes what is left first, and acts only on that one directory being made. Three clean-ups in `finally` could throw and replace the failure they followed. Two titles said of a mount point what is true of its manifest, one claimed the second look for every wrap, one still had the directory settled on as the wrap records, and a comment said more of read denies than holds. One of three rounds in a loop held nothing the others do not. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SEZ8LUnnuZdEgiFocuvVDm Co-authored-by: ronleizrowice <ronleizrowice@anthropic.com>
|
@antdres this needs your approval again. Thirty-five commits came after cdfb8a8, the head you approved, and the first push dismissed the approval. What to read. All of the source change is in the first twenty-three commits, up to fdce419: +247 −207 in two files. The description has a row for each commit, with what it mends and which case holds it. Why the last twelve net out to tests. Three rounds of answers to small review findings each brought in regressions, ten in all, in the twenty lines of What is not closed. The passes also turned up five older gaps, all in the fallback to a directory of the process's own or in a race over a shared name. They are under "Gaps found and not closed here" in the description. Each is for a small PR of its own, after this one. CI. All eight required legs are green at db71e7c. At 5d5faf5 macOS arm64 failed with no test named in its annotations. That head differed from green ones only in test files that run on Linux alone, and the leg is green again on the next push, which touches one such file. Auto-merge is off. Generated by Claude Code |
Human: @ronleizrowice-ant
Requested by Ron · project thread
On Linux an absent
denyWritepath is blocked by an empty file or directory (a "mount point") that bubblewrap makes on the host. Each process kept its own list of them in memory. So:SIGKILLleft all of its mount points for good, and nothing ever swept them;bwrap: Can't find source path.Now a mount point goes only when no running sandbox relies on it, whoever made it, and what a killed process leaves goes with the next clean-up of any process.
How big this is, and what has been read
37 files, +11,283 −323 against
main.mount-point-liveness,mount-point-manifests,mount-point-passesbwrap-mount-manifests.tsBy a person: @antdres read 14d0b4c and raised two blockers, answered below, and approved cdfb8a8. The thirty-five commits listed next came after that approval and dismissed it.
By automated review (Claude Code's
/code-reviewatxhigh), with the range of each pass:main..cdfb8a8cdfb8a8..the fixes and cuts, first formcdfb8a8..016902b016902b..de39d0d, with the first form of 3276893de39d0d..fdce419fdce419..fa247d2,fdce419..273ea90,273ea90..7a31540src/back, byte for byte, to fdce419, and keeps the tests. What these passes found that fdce419 has too is under "Gaps found and not closed here"fdce419..5d5faf5src/and the README checked to be the same byte for byte. 12 findings, none on what the cases hold. One is real: a new case passed in file order and failed under a filter, or in 4 of 16 random orders. Two clean-ups could replace the failure they followed, one hook was looser than its comment, four titles or comments said too much, one round of a loop held nothing. Answered in db71e7c. Left: set-up repeated between three cases, and two outside the range (a fixture tears down in an order that leaves an empty directory to the next case, one refusal is still spelled out)The simplification pass did not make it smaller. It cut about 470 lines net (repeated and empty cases 156, five copies of the same fixtures 249, the protocol told three times in comments 60), and the fixes with their tests added more: +10,963 before, +11,283 now. Where the lines are is what the table above says: tests of a protocol between processes that share nothing but a directory.
Since the approval at cdfb8a8: thirty-five commits
+1,501 −1,212 in all; source +247 −207. The first twenty-three, up to fdce419, are each readable by themselves and are all of the source. The eight from f2f10a2 on answer the third and fourth review passes: +409 −166, source +75 −37. The last twelve net out to tests.
/proc/self/statintos. A caller's exportedsreached the sandboxed command holding the host shell's stat line. Now a function-local variable.sexported, under each of the two step shellsmaindoubled a README paragraph./var/tmpis a link to/tmpor a bind mount of it, two of the places are one directory. A clean-up could not go on record in it twice, so it never removed anything. Directories are now told apart by device and inode.denyReadon/tmpor/var/tmpleft them readable. They are not bound where a tmpfs hides them.lstatwas taken for "not there", so a manifest directory went unbound and unpinned for that command. The wrap now refuses withmount_points_unreadable.lstatfailing on a place that is bound but not written undercollectMountPointsdefaulted to releasing everything for the tests' sake; three booleans in a row became named options.test/helpers/mount-point-fixtures.ts. No case changes. −249.collect()states as invariants and the README tells in full. −60.ENOENT. It failed a Linux leg once, and could on any PR on top. The child now ends only once the shell has become the program that never waits.liveMountPoints()off Linux, two of the three looks at a manifest directory that now throw, and the clean-up at exit of a process whose only wrap came to nothing.mainas it was here. Sockets are no longer counted.$TMPDIR/srt-mount-points-<uid>, or took the user's own empty directory for a mount point on the word of a stale or forged manifest there, named it in its own manifest, and had it removed by the next clean-up. A wrap with an empty file at a denied path read that early before, and had both. The places are made at the head of the plan.src/as at fdce419. What stays is tests: junk that is in doubt for an hour and not for half a second (in six places), the refusal fixture in two more places, a hook that no longer counts calls, a doc comment back on its function, and two cases for what the second look does.finallyno longer throw; four titles and two comments say what holds; a loop has two rounds for three.mount-point-livenessin file order, under the two filters that failed, and in 24 random ordersLeft as it is, on purpose. The cache in
namedMountPoints(it matters with many sessions in one home).sourcesas a second list in the manifest (cutting it changes the format and whatliveMountPoints()returns). The listing before each removal (above). The container end-to-end suite goes on using the real places. Two points on speed only: a started record at its 4 MiB cap costs some 14,000 reads of/proc, about 100 ms, each time it is asked after, so about twenty such records would use up a clean-up's two seconds and it would remove nothing; and each manifest directory islstated twice per reading. Three that err on the safe side: a started record past the cap vouches until the next boot, and where the manifest has no boot id, for good; a process that comes to write under the other name of its manifest directory lets go of its earlier manifests only at its exit (the one comparison by full path that erred the other way is 2753345's); and what a wrap could record nowhere is not kept by command, so it stays until none of the process's commands is outstanding, also once its own is over. One on where a rule lives: what the environment names is believed only while none of the three places that follow from the user id can be written, so until a wrap has made those, a bareliveMountPoints()can answerundefinedfor junk where the environment says. It errs on the safe side, and a look that made directories would break the rule that a look makes nothing. And a cost: reading first and making the places first add some 30 calls on the manifest directories to each wrap (lstat46 to 66,open24 to 28,readdir6 to 8, with twenty manifests there).Gaps found and not closed here
The last three passes found these in fdce419's source too. Each attempt to close one inside this PR cost more than it mended, so they are for small PRs of their own, where twenty lines can be read as twenty lines. The first three need the fallback, a directory only this process knows, which is used when none of the five shared names can be: in practice, when somebody else has the name under the temp dir.
SandboxManagerresets at exit.Since the review at 14d0b4c
Up to date with
main(85 commits), and #615 is folded in: the two have to ship in one release, because no release may write manifests anywhere but where the user id says.Blocker 1, a clean-up paused between its check and its delete. Closed, not narrowed. A clean-up puts itself on record (
<pid>-<start>-<ns>-<boot>-<random>.pass, an empty file beside the manifests) before the reading it decides on, and takes that away after its last removal. A wrap lists those records once it has published its manifest, waits for the ones it finds, and only then looks at the host again and hands its command out. Whichever of the two came second has seen the other: either the clean-up read the manifest and keeps what it names, or no sandbox starts under that manifest until the clean-up is over, however long that is held up. A process in the same PID namespace is asked after by pid and start time, so a stopped one is waited for and a dead one is not. A wrap waits five seconds and then refuses withmount_points_changed, naming the record.Test: a clean-up in its own process stops itself with
SIGSTOPright before theunlink; the wrap hands nothing out for as long as it is stopped; afterSIGCONTthe sandbox starts, and itsrmand write of the denied path are refused.Blocker 2, the same wrapped command run twice. It works now. A manifest stays live while the process that wrote it runs and has not let go of it, so no other process collects it between two runs. The writer lets go of commands that have run and ended at its next
cleanupAfterCommand(); after that a run is refused, as before.Test: one string run three times with another process cleaning up after each run, then refused after
cleanupBwrapMountPoints().Found on the way: a zombie counted as running. A killed bubblewrap that nothing waits for stays in
/proc. Under a process 1 that reaps nothing, as in many containers, that is for good, and so were its mount points. StateZorXnow counts as gone.Found on the way: two clean-ups that cannot judge each other's manifests each left a path to the other. A path that a clean-up keeps for another manifest is dropped from its own and left to that one. To a clean-up in another PID namespace a claimed manifest is live like any other. So two of them, each finishing with a manifest that names the same path at the same moment, each kept it for the other's and dropped their own: the path stayed, and nothing named it. A clean-up that can ask now renames the claim of a manifest it finds no process behind to
<id>.finished, before the reading it decides on, and drops it only at that name; one that cannot ask keeps nothing for a manifest at that name. Whichever reads second finds the other's finished. One that goes by a manifest's age does not give it the name, and one that can ask goes by what it finds, whatever the name.Test: two processes that take themselves to be in PID namespaces of their own meet once both have claimed and again once both have read; before the fix the file stays with no manifest left, every time.
What changes
mount_points_changed).bash -p, else/bin/sh) that appends its/proc/self/statline to a record and execs bubblewrap./var/tmp/srt-mount-points-<uid>;/run/user/<uid>/srt-mount-points;/tmp/srt-mount-points-<uid>; then the environment's two names. Every wrap that restricts writes makes, binds read-only and pins every place, but for one that a read deny already hides from the command. Two names of one directory count once.mount_points_unreadable, naming the file), never for good. So does a place that cannot be looked at for the moment. A manifest of another boot is finished on file systems only this kernel reaches; one of another PID namespace is live for seven days..claimed), asks again and renames it once more (.finished), goes on record, reads again, and removes a path, children first, only when every manifest naming it is so claimed with no process on record.cleanupAfterCommand({ commandId }); exportliveMountPoints().Joins with
main--argsprofile of a command that has not started. A wrap that comes to nothing gives its place back without letting go of other commands.withoutManifestName); child processes of the newer suites load the library throughtest/helpers/isolated, so no suite but the container one reaches the user's real manifest directories; the mount point suites join the bubblewrap 0.4.1 step.Who sees a difference
Linux only. A wrapped string with mount points starts with a shell. Three new
LinuxSandboxProfileErrorcodes. A mount point whose manifest is gone (0.0.81 or earlier; a wiped directory) stays until deleted. A process that exits under a running command leaves its mount points to the next clean-up.Known limits (README)
Merge order
Before #569 and #574, which are re-derived on top, and before #753. #594 joins in tests (the word after the record, not the first word or a bare
bwrap).Testing
CI: all eight required legs were green at 016902b and at fdce419, whose source this is. At 5d5faf5 seven were, and macOS arm64 failed with no test named in its annotations; that head differed from green ones only in test files that run on Linux alone. The checks on this page speak for the head. All eight were green at fa247d2, 273ea90 and 7a31540 as well, which had the regressions listed above: CI does not reach those states.
The rest is from one Linux x86-64 machine, non-root, Bun 1.4.2 and Node 24, real bubblewrap built from the two commits CI pins. It has no socat and no outbound network: a stand-in relays for socat, so CI is what speaks for the network legs.
mainon the same machine, at the fourteenth commit with linux: mount points hold across hosts that share a home #753's on top: 4,601 pass. The 24 that fail are the same names that fail onmainthere (they need outbound network); nothing that passes onmainfails..bashrc,.gitconfig,.mcp.json,.claude/settings.jsonand two more with the command's content. This branch had none of either and left nothing.SIGKILLto srt, its shell and bubblewrap at once: 12 mount points left in both. After the next command: 0.0.81 still has all 12, this branch has none. The machine's process 1 reaps nothing, so the killed bubblewrap was a zombie throughout.eslint .and both type-checks clean.Claude session #01SEZ8LU
Generated by Claude Code