From 217fddba20ab4decee59009ee2d44b1e42e49ae5 Mon Sep 17 00:00:00 2001 From: Cong Wang Date: Fri, 2 Oct 2026 16:47:15 -0700 Subject: [PATCH 1/3] ffi: fail the build on an unknown BranchAction discriminant The on_exit/on_error setters mapped any unrecognized value to Commit, the only action that writes the branch into the workdir, and the build still reported success. A binding passing a bad value had its changes merged with no way to notice. An out-of-range discriminant is a binding-layer programmer error, which the C ABI already signals with a null builder: drop the builder and return null so the setter chain carries it through and the build fails with err = -1, rather than guessing an action on the caller's behalf. Fixes #175 Signed-off-by: Cong Wang --- crates/sandlock-ffi/include/sandlock.h | 8 +++- crates/sandlock-ffi/src/lib.rs | 33 ++++++++----- crates/sandlock-ffi/tests/branch_action.rs | 56 ++++++++++++++++++++++ 3 files changed, 83 insertions(+), 14 deletions(-) create mode 100644 crates/sandlock-ffi/tests/branch_action.rs diff --git a/crates/sandlock-ffi/include/sandlock.h b/crates/sandlock-ffi/include/sandlock.h index 3f8c613b..e2c6fe93 100644 --- a/crates/sandlock-ffi/include/sandlock.h +++ b/crates/sandlock-ffi/include/sandlock.h @@ -455,7 +455,9 @@ sandlock_builder_t *sandlock_sandbox_builder_fs_mount_ro(sandlock_builder_t *b, /** * Set the COW branch action on successful exit. - * `action`: 0 = Commit, 1 = Abort, 2 = Keep, 3 = Defer. + * `action`: 0 = Commit, 1 = Abort, 2 = Keep, 3 = Defer. Any other value + * frees the builder and returns null, so the eventual build fails rather + * than guessing an action that may write into the workdir. * * # Safety * `b` must be a valid builder pointer. @@ -464,7 +466,9 @@ sandlock_builder_t *sandlock_sandbox_builder_on_exit(sandlock_builder_t *b, uint /** * Set the COW branch action on error exit. - * `action`: 0 = Commit, 1 = Abort, 2 = Keep, 3 = Defer. + * `action`: 0 = Commit, 1 = Abort, 2 = Keep, 3 = Defer. Any other value + * frees the builder and returns null, so the eventual build fails rather + * than guessing an action that may write into the workdir. * * # Safety * `b` must be a valid builder pointer. diff --git a/crates/sandlock-ffi/src/lib.rs b/crates/sandlock-ffi/src/lib.rs index 0f176576..832bab6b 100644 --- a/crates/sandlock-ffi/src/lib.rs +++ b/crates/sandlock-ffi/src/lib.rs @@ -304,7 +304,9 @@ pub unsafe extern "C" fn sandlock_sandbox_builder_fs_mount_ro( } /// Set the COW branch action on successful exit. -/// `action`: 0 = Commit, 1 = Abort, 2 = Keep, 3 = Defer. +/// `action`: 0 = Commit, 1 = Abort, 2 = Keep, 3 = Defer. Any other value +/// frees the builder and returns null, so the eventual build fails rather +/// than guessing an action that may write into the workdir. /// /// # Safety /// `b` must be a valid builder pointer. @@ -317,12 +319,16 @@ pub unsafe extern "C" fn sandlock_sandbox_builder_on_exit( return b; } let builder = *Box::from_raw(b); - let action = branch_action(action); - Box::into_raw(Box::new(builder.on_exit(action))) + match try_branch_action_from_raw(action) { + Some(action) => Box::into_raw(Box::new(builder.on_exit(action))), + None => ptr::null_mut(), + } } /// Set the COW branch action on error exit. -/// `action`: 0 = Commit, 1 = Abort, 2 = Keep, 3 = Defer. +/// `action`: 0 = Commit, 1 = Abort, 2 = Keep, 3 = Defer. Any other value +/// frees the builder and returns null, so the eventual build fails rather +/// than guessing an action that may write into the workdir. /// /// # Safety /// `b` must be a valid builder pointer. @@ -335,16 +341,19 @@ pub unsafe extern "C" fn sandlock_sandbox_builder_on_error( return b; } let builder = *Box::from_raw(b); - let action = branch_action(action); - Box::into_raw(Box::new(builder.on_error(action))) + match try_branch_action_from_raw(action) { + Some(action) => Box::into_raw(Box::new(builder.on_error(action))), + None => ptr::null_mut(), + } } -fn branch_action(discriminant: u8) -> BranchAction { - match discriminant { - 1 => BranchAction::Abort, - 2 => BranchAction::Keep, - 3 => BranchAction::Defer, - _ => BranchAction::Commit, +fn try_branch_action_from_raw(raw: u8) -> Option { + match raw { + 0 => Some(BranchAction::Commit), + 1 => Some(BranchAction::Abort), + 2 => Some(BranchAction::Keep), + 3 => Some(BranchAction::Defer), + _ => None, } } diff --git a/crates/sandlock-ffi/tests/branch_action.rs b/crates/sandlock-ffi/tests/branch_action.rs new file mode 100644 index 00000000..8d786793 --- /dev/null +++ b/crates/sandlock-ffi/tests/branch_action.rs @@ -0,0 +1,56 @@ +//! C ABI `BranchAction` setters: discriminants map onto the Rust enum, and +//! any other value fails the build instead of picking an action. + +use std::ptr; + +use sandlock_core::sandbox::{BranchAction, SandboxBuilder}; +use sandlock_core::Sandbox; +use sandlock_ffi::{ + sandlock_sandbox_build, sandlock_sandbox_builder_new, sandlock_sandbox_builder_on_error, + sandlock_sandbox_builder_on_exit, +}; + +type Setter = unsafe extern "C" fn(*mut SandboxBuilder, u8) -> *mut SandboxBuilder; + +const DISCRIMINANTS: [(u8, BranchAction); 4] = [ + (0, BranchAction::Commit), + (1, BranchAction::Abort), + (2, BranchAction::Keep), + (3, BranchAction::Defer), +]; + +fn build_with(setter: Setter, raw: u8) -> Sandbox { + let b = unsafe { setter(sandlock_sandbox_builder_new(), raw) }; + assert!(!b.is_null(), "valid discriminant {raw} rejected"); + unsafe { *Box::from_raw(b) }.build().expect("build failed") +} + +#[test] +fn on_exit_discriminants_match_rust_enum() { + for (raw, action) in DISCRIMINANTS { + assert_eq!(build_with(sandlock_sandbox_builder_on_exit, raw).on_exit, action); + } +} + +#[test] +fn on_error_discriminants_match_rust_enum() { + for (raw, action) in DISCRIMINANTS { + assert_eq!(build_with(sandlock_sandbox_builder_on_error, raw).on_error, action); + } +} + +#[test] +fn unrecognized_discriminant_fails_the_build() { + let setters: [Setter; 2] = [sandlock_sandbox_builder_on_exit, sandlock_sandbox_builder_on_error]; + for setter in setters { + for raw in [4u8, 99, u8::MAX] { + let b = unsafe { setter(sandlock_sandbox_builder_new(), raw) }; + assert!(b.is_null(), "discriminant {raw} accepted"); + + let mut err = 0; + let policy = unsafe { sandlock_sandbox_build(b, &mut err, ptr::null_mut()) }; + assert!(policy.is_null()); + assert_eq!(err, -1); + } + } +} From 3f528cc1857769e18914bfa5f5a5e4cb24865ed6 Mon Sep 17 00:00:00 2001 From: Cong Wang Date: Fri, 2 Oct 2026 16:48:21 -0700 Subject: [PATCH 2/3] go: reject an out-of-range BranchAction before building BranchAction is a plain uint8, so Sandbox{OnExit: BranchAction(9)} compiles and reaches the C ABI. The FFI now fails such a build, but only with a bare error code; checking up front, before the builder exists, gives Go callers an error naming the bad value. Signed-off-by: Cong Wang --- go/sandlock_linux.go | 5 +++++ go/sandlock_linux_test.go | 11 +++++++++++ 2 files changed, 16 insertions(+) diff --git a/go/sandlock_linux.go b/go/sandlock_linux.go index e60159f5..e3674987 100644 --- a/go/sandlock_linux.go +++ b/go/sandlock_linux.go @@ -173,6 +173,11 @@ func (s *Sandbox) buildPolicy() (*C.sandlock_sandbox_t, error) { if err := s.validateStrings(); err != nil { return nil, err } + for _, a := range []BranchAction{s.OnExit, s.OnError} { + if a > BranchActionDefer { + return nil, fmt.Errorf("sandlock: invalid BranchAction %d", a) + } + } b := C.sandlock_sandbox_builder_new() diff --git a/go/sandlock_linux_test.go b/go/sandlock_linux_test.go index 7c7f56a8..a1462aea 100644 --- a/go/sandlock_linux_test.go +++ b/go/sandlock_linux_test.go @@ -148,6 +148,17 @@ func TestRunNULRejected(t *testing.T) { } } +func TestRunUnknownBranchActionRejected(t *testing.T) { + for _, sb := range []*sandlock.Sandbox{ + {OnExit: sandlock.BranchActionDefer + 1}, + {OnError: sandlock.BranchAction(255)}, + } { + if _, err := sb.Run(context.Background(), "true"); err == nil || !strings.Contains(err.Error(), "invalid BranchAction") { + t.Fatalf("err = %v, want invalid BranchAction", err) + } + } +} + func TestSyscallEventArgvContains(t *testing.T) { ev := sandlock.SyscallEvent{Argv: []string{"python3", "-c", "print(1)"}} if !ev.ArgvContains("python") { From 0199deb76e31cb297111069680f69d4e1d9df921 Mon Sep 17 00:00:00 2001 From: Cong Wang Date: Fri, 2 Oct 2026 16:50:50 -0700 Subject: [PATCH 3/3] python: reject an unknown on_exit/on_error value The SDK mapped any unrecognized branch action to Commit through a dict.get default, so Sandbox(on_exit="Abort") committed the workdir it was meant to discard. Coerce both fields through BranchAction when the Sandbox is constructed so a bad value raises ValueError immediately, and look the discriminant up strictly when building. Signed-off-by: Cong Wang --- python/src/sandlock/_sdk.py | 10 +++++----- python/src/sandlock/sandbox.py | 2 ++ python/tests/test_sandbox.py | 9 +++++++++ 3 files changed, 16 insertions(+), 5 deletions(-) diff --git a/python/src/sandlock/_sdk.py b/python/src/sandlock/_sdk.py index 6ba0c14a..7b59c03b 100644 --- a/python/src/sandlock/_sdk.py +++ b/python/src/sandlock/_sdk.py @@ -13,7 +13,7 @@ from pathlib import Path from typing import Any, NamedTuple, Sequence -from .sandbox import Change, Entry, Image, ImageConfig, Sandbox as PolicyDataclass +from .sandbox import BranchAction, Change, Entry, Image, ImageConfig, Sandbox as PolicyDataclass # ---------------------------------------------------------------- # Load the shared library @@ -1201,13 +1201,13 @@ def _build_from_policy(policy: PolicyDataclass): # COW branch actions (0=Commit, 1=Abort, 2=Keep, 3=Defer) _action_map = {"commit": 0, "abort": 1, "keep": 2, "defer": 3} - on_exit_val = policy.on_exit.value if hasattr(policy.on_exit, 'value') else str(policy.on_exit) - on_error_val = policy.on_error.value if hasattr(policy.on_error, 'value') else str(policy.on_error) + on_exit_val = BranchAction(policy.on_exit).value + on_error_val = BranchAction(policy.on_error).value # An image's writes are always discarded, and the native build rejects # any other exit action, so leave the COMMIT default unsent for it. if policy.image is None or on_exit_val != "commit": - b = _b_on_exit(b, _action_map.get(on_exit_val, 0)) - b = _b_on_error(b, _action_map.get(on_error_val, 1)) + b = _b_on_exit(b, _action_map[on_exit_val]) + b = _b_on_error(b, _action_map[on_error_val]) if policy.max_memory is not None: if isinstance(policy.max_memory, str): diff --git a/python/src/sandlock/sandbox.py b/python/src/sandlock/sandbox.py index a1e9282e..6535e9af 100644 --- a/python/src/sandlock/sandbox.py +++ b/python/src/sandlock/sandbox.py @@ -529,6 +529,8 @@ def __post_init__(self): raise ValueError("sandbox name must not contain '/'") if self.name in (".", ".."): raise ValueError("sandbox name must not be '.' or '..'") + self.on_exit = BranchAction(self.on_exit) + self.on_error = BranchAction(self.on_error) # Runtime state — not dataclass fields, not serialized self._native = None # _NativePolicy created lazily on first use self._handle = None # live sandbox handle during start()/run() diff --git a/python/tests/test_sandbox.py b/python/tests/test_sandbox.py index 987cebf1..a59effe2 100644 --- a/python/tests/test_sandbox.py +++ b/python/tests/test_sandbox.py @@ -811,6 +811,15 @@ def test_resume_not_running_raises(self): class TestBranchAction: """Every run reports its changes; DEFER hands the disposition to the caller.""" + def test_string_action_is_coerced(self): + p = Sandbox(on_exit="abort", on_error="keep") + assert (p.on_exit, p.on_error) == (BranchAction.ABORT, BranchAction.KEEP) + + @pytest.mark.parametrize("field", ["on_exit", "on_error"]) + def test_unknown_action_is_rejected(self, field): + with pytest.raises(ValueError): + Sandbox(**{field: "Abort"}) + def test_abort_reports_added_file_without_creating_it(self, tmp_path): workdir = tmp_path / "add" workdir.mkdir()