builder: latch a rejected setter argument instead of coercing it by dzerik · Pull Request #186 · multikernel/sandlock

dzerik · 2026-08-04T14:08:58Z

Closes #175.

Second of three. Stacked on #185, so this branch carries that commit too:
review the second one, builder: latch a rejected setter argument, and
merge #185 first.

The problem

A builder setter returns Self, not Result, so it has no channel for a
value the core cannot accept. The C ABI answered that by coercing:

an on_exit discriminant with no variant became Commit, through the
fall-through arm of a match;
an unrecognized protection discriminant was a documented no-op;
every string setter ran its argument through to_str().unwrap_or(""),
except the two mount setters, which dropped the whole call instead.

Each of those runs a configuration the caller never wrote, and says nothing
while doing it. unwrap_or("") is reachable without any bug in the caller,
since a path read off readdir() is an arbitrary byte string on Linux, and
the empty path it produces is a prefix of every guest path.

What this does

SandboxBuilder carries a pending-error latch. reject records a reason a
surface diagnosed itself, reject_error records one the core's own parser
produced, and build() returns it instead of a Sandbox.

The setter contract is otherwise untouched, which is why the bindings do not
move: this commit changes nothing under go/ or python/src.

Four design decisions worth naming

The latch holds a String, not a SandboxError. SandboxBuilder is
Clone and SandboxError is not; making it Clone would widen a public
error type for the benefit of one private field.

The check sits in build_unchecked, not in build. build_unchecked is
public and is what sandlock-oci calls
(crates/sandlock-oci/src/policy.rs:463). A check in build alone would let
the one caller that deliberately skips cross-section validation also skip the
caller's own rejected input, which is not the invariant it asked to skip.

Clone carries the latch. Dropping it there would make .clone().build()
a laundering channel for a value the core has already refused.

First write wins. The earliest bad input is the one that explains whatever
follows it, so the message names the caller's first mistake rather than its
last.

What now reports instead of coercing

on_exit and on_error, on an unrecognized discriminant. BranchAction
gains #[repr(u8)] with explicit discriminants and a from_repr, so the
values the bindings pass as a u8 are a written-down contract rather than
the fall-through arm of a match. Serde is unaffected: a data-less enum
serializes by variant name, not by discriminant.
allow_degraded and disable, on an unrecognized protection. The no-op was
documented, which meant a binding built against a newer header was told
nothing when an older library did not recognize the protection it asked to
be degradable: the caller believed it had opted out, and the protection
stayed strict.
All 22 string setters, through one setter_arg helper: a null pointer, and
bytes that are not UTF-8. Those two stay the C ABI's own verdicts because
they are representation problems the core cannot see once the value is a
&str; the grammar's verdict still comes from the core untouched. The
three-argument setters report per half (env_var key, fs_mount_ro host path), so the message names the pointer to fix.

fs_mount and fs_mount_ro had a coercion of their own shape: a private
mount_pair helper answered "add no mount" for a null, non-UTF-8 or empty
path, so the caller who asked for a read-only subtree got a writable one and
the caller who asked for a host directory got nothing there. They go through
the latch now, which is what makes the sentence above true of every *const c_char builder setter rather than of most of them.

Zero and the empty set, in the same commit

The latch stops a surface from inventing a value the caller did not write;
these stop a surface from having to invent a verdict the core would not give.
Both have to be in place before a binding can be reduced to forwarding, and
neither is visible in a binding's own diff.

max_processes = 0: the supervisor compares proc_count >= limit, so a
limit of zero denies every fork with EAGAIN no matter how few processes
are alive, and the workload reads "Resource temporarily unavailable" from
its first subprocess with nothing naming the setting.
num_cpus = 0: reaches the synthetic procfs as an empty /proc/cpuinfo
and an affinity mask with no bits, so the guest reads nproc = 0.
max_memory = 0: zero is the sentinel the supervisor already carries
for "no ceiling", but the memory handler is registered on is_some(). An
explicit zero therefore installs a ceiling of zero and SIGKILLs the loader's
first anonymous mmap while /proc/meminfo reports the sandbox unlimited.
Refusing the value is what lets the sentinel keep meaning "unset".
max_disk is deliberately not the same: zero is its documented spelling of
"unlimited", and one reading is all it has.
cpu_cores = []: an affinity mask with no bits, which
sched_setaffinity(2) refuses with EINVAL. confine_child skipped the
call for an empty set instead, so the pinning the caller asked for silently
did not happen and the sandbox ran on every core. Unlike gpu_devices,
where an empty list is the spelling of "every device present", there is no
cpu set an empty list could stand for.
an empty virtual or host path in fs_mount/fs_mount_ro: this is the
check that came back from the C ABI. mount_pair was making a policy
judgement the core's own profile grammar already makes, and making it in the
one place that could not report it. An empty virtual path is a prefix of
every guest path, so ChrootCtx::is_mounted would match the whole tree and
short-circuit can_read and can_write.

One over-strict check removed

Confinement::try_from listed on_exit and on_error among the fields a
confinement cannot honour. A confinement has no branch to act on: it is
applied in place, and fs_storage and workdir, the two knobs that create
one, are already refused above it. The check only ever refused a field that
could not have changed the outcome, and it compared against two hardcoded
actions rather than against what build() resolves an unset field to, so a
caller who said nothing about the error path was refused a confinement its
policy allowed.

The C ABI does not move

No signature, no struct and no discriminant value changes.
include/sandlock.h changes by 171 lines (141 added, 30 removed) and every
one of them is inside a comment block: with comment lines stripped the header
is byte-identical to its parent. The added ones write the four new refusals
where a binding author reads them, along with max_open_files = 0, which was
already refused and had never been documented anywhere.

Testing

crates/sandlock-ffi/tests/builder_pending_error.rs (727 lines) covers the
latch itself: both branch-action setters, survival across later valid calls,
first-write-wins, Clone, build_unchecked, and null, non-UTF-8 and per-half
arguments across every string setter.

tests/fs_mount.rs had four tests pinning the drop-silently behaviour of the
mount setters; they become one that pins the report, over both setters and all
six unusable inputs. In protection.rs the two tests that asserted the no-op
now assert the report. builder.rs covers reject_error directly with a real
ByteSize::parse error, asserting the built message is the parser's own text
rather than a doubled invalid sandbox: invalid sandbox: ....

cargo test -p sandlock-core --lib: 739 pass at this commit.

Closes the second half of multikernel#174. The SDK carried its own TOML parser and its own grammars, and they had already diverged from the core in two places you found: `parse_memory_size` accepted fractions and a `T` suffix that `ByteSize::parse` rejects, and `time_start` went through `int()`, so an RFC 3339 stamp worked in the CLI and raised through the SDK. A third grammar sat unused in the dataclass, `time_start_timestamp`, with naive-means-UTC semantics. `sandlock_profile_parse` takes TOML text and returns canonical JSON with every micro-grammar already resolved: mounts as `{virt, host, ro}` objects, sizes as integer bytes, `time_start` as epoch seconds. The SDK's remaining job is a field-for-field copy into its dataclass, so introspection, `dataclasses.replace` and preset composition keep working. Unknown keys are rejected on both sides, so future drift fails at load time instead of mis-parsing silently. `sandbox_to_json` was not reusable as-is: it re-emits mounts as `V:H:ro` spec strings, which would have put string parsing straight back into the SDK. The canonical form emits structured mounts instead. Its `ro` is the effective setting for the virtual path, not the flag written on one spec: the core keys read-only mounts by virtual path (`Sandbox::fs_mount_ro` is a list of virtual paths), so two specs sharing a virtual path share one verdict, and reporting the written flag would describe a policy no layer applies. Public Python API changes, deliberately and without shims: max_memory: str | int | None -> int | None max_disk: str | None -> int | None time_start: float | str | None -> float | None fs_mount: Mapping[str, str] -> Sequence[Mount] `Mount(virt, host, ro)` is new and mirrors the canonical field names. `fs_mount` becoming a sequence is what lets a read-only mount be expressed at all from Python; it reaches the C ABI through the `fs_mount_ro` setter added in multikernel#180. `tomli` is gone from the dependencies. Two more changes to the same surface, both consequences of the SDK no longer holding an opinion of its own: - `on_error` loaded from a profile now defaults to COMMIT where it defaulted to ABORT. The canonical form always resolves both branch actions, and the SDK copies what it is handed, so a profile that says nothing about the error path gets the core's answer rather than the dataclass's second opinion. Deliberate, since the CLI, a profile and the Go SDK have always meant COMMIT for that policy, but it changes what happens to a COW branch for a profile already in use, and it changes it silently. Only the profile path moves; the dataclass default is untouched here. - `parse_memory_size`, `Sandbox.memory_bytes()` and `Sandbox.time_start_timestamp()` are removed. The first two were the SDK's byte-size grammar and its accessor, the third the unused third grammar named above. Nothing replaces them: the resolved value is the field. Two core changes came out of this rather than the SDK: - Rebuilding a builder from a parsed profile ran `extend_net_allow_for_http` a second time over an allowlist that already held its derived entries, so the helper is now idempotent, with a test. - `ByteSize::parse` multiplies with `checked_mul`. The unchecked multiply wrapped in release builds, so `memory = "17179869184G"` parsed cleanly and installed a ceiling of zero bytes, with nothing reported anywhere and the guest SIGKILLed on its first allocation. It is an out-of-range error now. Verified against the CLI message for message on every grammar: the same profile loads identically, or fails identically, through both paths, with one gap left open and pinned rather than papered over. `sandlock_sandbox_builder_time_start` takes a `uint64` of seconds, so a stamp the core keeps in full loads from a profile and then cannot be handed to a builder: `"2026-01-01T00:00:00.5Z"` and any instant before 1970 are what that costs. The SDK refuses them by name instead of wrapping a negative value through an unsigned setter, and `test_time_start_the_c_abi_cannot_carry_is_refused_loudly` holds it there. Closing the gap means changing that setter's signature, which is a later commit in this series.

A builder setter returns Self, not Result, so it has no channel for a value the core cannot accept. The C ABI answered that by coercing. An on_exit discriminant with no variant became Commit through the fall-through arm of a match. An unrecognized protection discriminant was a documented no-op. Every string setter ran its argument through `to_str().unwrap_or("")`, except the two mount setters, which dropped the whole call instead. Each of those runs a configuration the caller never wrote, and says nothing while doing it. SandboxBuilder now carries a pending-error latch. `reject` records a reason a surface diagnosed itself, `reject_error` records one the core's own parser produced, and `build()` returns it instead of a Sandbox. The setter contract is otherwise untouched, which is why the bindings do not move: this commit changes nothing under go/ or python/src. The three python/tests files it does touch move because of the zero checks below, not because of binding work. Three decisions worth naming. The latch holds a String, not a SandboxError. SandboxBuilder is Clone and SandboxError is not; making it Clone would widen a public error type for the benefit of one private field. `reject_error` keeps the parser's own text rather than the wrapped Display, because build() puts the reason back into SandboxError::Invalid, so a value refused through the C ABI reads exactly as it reads on the command line instead of as a doubled "invalid sandbox: invalid sandbox: ...". The check sits in `build_unchecked`, not in `build`. `build_unchecked` is public and is what sandlock-oci calls (crates/sandlock-oci/src/policy.rs:463). A check in `build` alone would let the one caller that deliberately skips cross-section validation also skip the caller's own rejected input, which is not the invariant it asked to skip. Clone carries the latch. Dropping it there would make `.clone().build()` a laundering channel for a value the core has already refused. First write wins. The earliest bad input is the one that explains whatever follows it, so later rejections are dropped and the message names the caller's first mistake rather than its last. What now reports instead of coercing: - on_exit and on_error, on an unrecognized discriminant. BranchAction gains #[repr(u8)] with explicit discriminants and a `from_repr`, so the values the bindings pass as a u8 are a written-down contract rather than the fall-through arm of a match. Serde is unaffected: a data-less enum serializes by variant name, not by discriminant. - allow_degraded and disable, on an unrecognized protection. The no-op was documented, which meant a binding built against a newer header was told nothing when an older library did not recognize the protection it asked to be degradable: the caller believed it had opted out, and the protection stayed strict. - 22 string setters, through one `setter_arg` helper: a null pointer, and bytes that are not UTF-8. Those two stay the C ABI's own verdicts because they are representation problems the core cannot see once the value is a &str; the grammar's verdict still comes from the core untouched. `unwrap_or("")` is reachable without any bug in the caller, since a path read off readdir() is an arbitrary byte string on Linux, and the empty path it produces is a prefix of every guest path. The coercion survives only in the entry points that take no builder and so have nothing to latch a reason on. The three-argument setters report per half (`env_var key`, `fs_mount_ro host path`), so the message names the pointer to fix. fs_mount and fs_mount_ro are the two that had a coercion of their own shape: a private `mount_pair` helper answered "add no mount" for a null, non-UTF-8 or empty path, so the caller who asked for a read-only subtree got a writable one and the caller who asked for a host directory got nothing there. They go through the latch now, which is what makes the sentence above true of every `*const c_char` builder setter rather than of most of them. Zero and the empty set, in the same commit and for the same reason. The latch stops a surface from inventing a value the caller did not write; these stop a surface from having to invent a verdict the core would not give. Both have to be in place before a binding can be reduced to forwarding, and neither is visible in a binding's own diff. The max_open_files check that was already here said as much in its comment, which claimed a binding "must" filter zero itself; that comment is corrected here too, and corrected to what is true today rather than to what the series is heading for. Python already forwards whatever is not None, zero included. Go still filters (`if s.MaxOpenFiles > 0` in go/sandlock_linux.go), so a Go caller who writes zero still gets no cap and no diagnosis; reducing Go to forwarding needs its fields to spell "unset" without using the value, which is a later commit. - max_processes = 0: the supervisor compares proc_count >= limit, so a limit of zero denies every fork with EAGAIN no matter how few processes are alive, and the workload reads "Resource temporarily unavailable" from its first subprocess with nothing naming the setting. - num_cpus = 0: reaches the synthetic procfs as an empty /proc/cpuinfo and an affinity mask with no bits, so the guest reads nproc = 0. - max_memory = 0: zero is the sentinel the supervisor already carries for "no ceiling" (max_memory.map(..).unwrap_or(0) in Sandbox::run, read back as > 0 by the synthetic /proc/meminfo), but the memory handler is registered on is_some(). An explicit zero therefore installs a ceiling of zero and SIGKILLs the loader's first anonymous mmap while /proc/meminfo reports the sandbox unlimited. The two readings cannot both stand, and refusing the value is what lets the sentinel keep meaning "unset". max_disk is deliberately not the same: zero is its documented spelling of "unlimited", and one reading is all it has. - cpu_cores = []: an affinity mask with no bits, which sched_setaffinity(2) refuses with EINVAL. confine_child skipped the call for an empty set instead, so the pinning the caller asked for silently did not happen and the sandbox ran on every core; that branch is deleted now that the value cannot reach it. Unlike gpu_devices, where an empty list is the spelling of "every device present", there is no cpu set an empty list could stand for, because "every core" is what omitting the field already means. - an empty virtual or host path in fs_mount and fs_mount_ro. This is the check that came back from the C ABI: `mount_pair` was making a policy judgement the core's own profile grammar already makes when it splits a VIRTUAL:HOST spec, and making it in the one place that could not report it. Neither half has a reading as "unset", and an empty virtual path is a prefix of every guest path, so ChrootCtx::is_mounted would match the whole tree and short-circuit can_read and can_write. Confinement::try_from listed on_exit and on_error among the fields a confinement cannot honour. A confinement has no branch to act on: it is applied in place, and fs_storage and workdir, the two knobs that create one, are already refused above it. The check only ever refused a field that could not have changed the outcome, and it did so by comparing against two hardcoded actions rather than against what build() resolves an unset field to, so a caller who said nothing about the error path was refused a confinement its policy allowed. The C ABI is unchanged: no signature, no struct and no discriminant value moves. include/sandlock.h changes by 171 lines (141 added, 30 removed) and every one of them is inside a comment block; with comment lines stripped the header is byte-identical to its parent. The added ones are the four new refusals written down where a binding author reads them: max_memory = 0, max_processes = 0, num_cpus = 0 and an empty cpu_cores are now in the doc comment of the setter that carries each, along with max_open_files = 0, which was already refused and had never been documented anywhere. The same rows in docs/sandbox-reference.md say the same thing. Tests: crates/sandlock-ffi/tests/builder_pending_error.rs covers the latch itself (both branch-action setters, survival across later valid calls, first-write-wins, Clone, build_unchecked, and null, non-UTF-8 and per-half arguments across every string setter). tests/fs_mount.rs had four tests pinning the drop-silently behaviour of the mount setters; they become one that pins the report, over both setters and all six unusable inputs. In protection.rs the two tests that asserted the no-op now assert the report, and the third, which checked that a later valid call still took effect, becomes the first-write-wins case while keeping the memory-safety property it was really watching. sandbox/tests.rs covers the confinement change from both sides, and builder.rs covers `reject_error` directly: it has no caller yet, since the four that use it arrive with the string setters in a later commit, so the test drives it with a real ByteSize::parse error and asserts the built message is the parser's own text rather than a doubled wrapping. Closes multikernel#175.

congwang-mk · 2026-08-05T23:03:05Z

Not a review, just a note: I am planning to cut the release in a few days, since this PR is fairly large, I'd suggest to defer it to the next release. WDYT?

dzerik · 2026-08-06T06:58:18Z

Agreed, defer it. Answered at length in #185, since the three are one stack and the same answer covers them.

Short version: this PR shows +5745/-975 because it carries #185 underneath it; what is new here is +1570/-367 across 14 files. Still not small, and a release is the wrong moment for it either way. Review order whenever you get to the batch: #185, then this, then #187.

dzerik added 2 commits August 4, 2026 16:40

dzerik mentioned this pull request Aug 4, 2026

ffi: take sizes and timestamps as strings; both SDKs stop parsing #187

Open

dzerik mentioned this pull request Aug 6, 2026

feat: parse profiles in the core, and have the Python SDK stop parsing #185

Open

Navigation Menu

Uh oh!

builder: latch a rejected setter argument instead of coercing it - #186

builder: latch a rejected setter argument instead of coercing it#186
dzerik wants to merge 2 commits into
multikernel:mainfrom
dzerik:feat/builder-latch-rejected-args

dzerik commented Aug 4, 2026

Uh oh!

congwang-mk commented Aug 5, 2026

Uh oh!

dzerik commented Aug 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

dzerik commented Aug 4, 2026

The problem

What this does

Four design decisions worth naming

What now reports instead of coercing

Zero and the empty set, in the same commit

One over-strict check removed

The C ABI does not move

Testing

Uh oh!

congwang-mk commented Aug 5, 2026

Uh oh!

dzerik commented Aug 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants