feat(seccomp): Handler trait + IntoIterator-shaped run_with_extra_handlers (Follow-up A)

[dzerik]

Follow-up A from the PR #20 review. Reshapes the user-supplied seccomp-notif extension API around a Handler trait + Syscall::checked newtype, kept dyn-compatible and minimal in public surface. Hard break — no deprecation cycle.

Public API

New exports at the crate root (sandlock_core::*):

• pub trait Handler { fn handle<'a>(&'a self, cx: &'a HandlerCtx) -> Pin<Box<dyn Future<Output = NotifAction> + Send + 'a>>; } — manually written (no async_trait macro dep) so the trait stays object-safe; the supervisor stores user handlers as Vec<Arc<dyn Handler>>.
• pub struct HandlerCtx { pub notif: SeccompNotif, pub notif_fd: RawFd } — no <'a> parameter, no sup field; SupervisorCtx is pub(crate) and not part of the extension contract.
• pub struct Syscall(i64) + Syscall::checked(nr) -> Result<Self, SyscallError> — non-negative + arch-known validation. Closes the silent-never-fires footgun.
• pub enum HandlerError { InvalidSyscall(SyscallError), OnDenySyscall { syscall_nr } }, surfaced as SandlockError::Handler(HandlerError).
• Three blanket impls for ergonomic composition:
  • impl<F, Fut> Handler for F where F: Fn(&HandlerCtx) -> Fut + Send + Sync + 'static, Fut: Future<Output = NotifAction> + Send + 'static — closures.
  • impl Handler for Box<dyn Handler> and impl Handler for Arc<dyn Handler> — type-erasure for callers mixing differently-typed handlers in one IntoIterator.

Reshaped entry points

Sandbox::run_with_extra_handlers and run_interactive_with_extra_handlers keep their names; the handler parameter shape changes from Vec<ExtraHandler> to IntoIterator<Item = (S, H)>:

pub async fn run_with_extra_handlers<I, S, H>(
    policy: &Policy,
    name: Option<&str>,
    cmd: &[&str],
    extra_handlers: I,
) -> Result<RunResult, SandlockError>
where
    I: IntoIterator<Item = (S, H)>,
    S: TryInto<Syscall, Error = SyscallError>,
    H: Handler;

Old call:

Sandbox::run_with_extra_handlers(&p, name, &c, vec![ExtraHandler::new(libc::SYS_openat, h)]).await?;

New call:

Sandbox::run_with_extra_handlers(&p, name, &c, [(libc::SYS_openat, openat_h)]).await?;

Sandbox::run and Sandbox::run_interactive keep their signatures unchanged from main.

Removed (hard break, no deprecation)

• pub struct ExtraHandler, ExtraHandler::new
• pub type HandlerFn (the boxed-closure typedef)
• async_trait = "0.1" dep from crates/sandlock-core/Cargo.toml

Public utility promotion

• pub fn read_child_cstr — TOCTOU-safe NUL-terminated child-memory read (was pub(crate)). Mirrors the 272ae0d precedent for read_child_mem / write_child_mem.

Internal changes

• DispatchTable backing storage is Vec<Arc<dyn Handler>>. register<H: Handler> and the pub(crate) register_arc accept any handler / pre-Arc'd handler.
• All 21+ builtin handler chunks rewritten onto the new shape using the closure-via-blanket-impl form.
• Builtins that previously dereferenced cx.sup.<field> now capture the specific sub-Arc (ctx.chroot, ctx.cow, ctx.processes, etc.) at table-build time.
• validate_extras_against_policy renamed to validate_handler_syscalls_against_policy, now takes &[i64].
• seccomp::ctx and seccomp::state modules are pub(crate).
• DispatchTable::dispatch is pub(crate); its ctx parameter is gone.
• notif::supervisor parameter follows the same Vec<(i64, Arc<dyn Handler>)> shape change.
• The chroot_handler! / chroot_handler_fallthrough! / cow_call! macros clone the specific sub-Arcs they need at expansion site instead of going through the public dispatch context.

Tests

• 8 carry-over integration tests from PR feat(seccomp): ExtraHandler — user-supplied syscall handlers #20, rewritten to use the new closure-via-blanket-impl shape and the [(syscall, handler)] array-literal call site.
• 6 new integration tests: blanket-impl-dispatches, struct-state-persists, two Syscall::checked rejections, insertion-order, and default-deny rejection.
• 1 new dispatch-level unit test plus 4 unit tests for Syscall::checked.

259 lib unit + 14 integration extra-handler tests pass; clippy clean; zero deprecation warnings.

Notes

• Pin<Box<dyn Future + Send + '_>> instead of native impl Future + Send in trait — RPITIT is not object-safe; the native shape would require a non-erased dispatch chain incompatible with arbitrary user handler types. This still drops the async_trait macro dep; the Box-future behaviour is the same the macro was generating.
• Concrete Box<dyn Handler> / Arc<dyn Handler> impls (not blanket <H: Handler + ?Sized>) to avoid coherence overlap with the closure blanket above.

Out of scope (deferred per PR #20 review)

• HandlerPriority::Before axis — explicitly deferred until a concrete consumer.
• FFI / Python parity for Handler trait — Follow-up B, separate PR on top of A.

[congwang-mk]

Thanks for the thorough PR. The security work is exactly right — Syscall::checked, per-syscall deny validation, the Handler trait + blanket impl, read_child_cstr promotion, dogfooding all 21 builtins onto the new shape, and the 6 new integration tests are all solid.

I want to walk back two things from my PR #20 review notes before this lands:

1. Drop the builder; keep the existing four-method shape.

I sketched Sandbox::add_handler builder in the PR #20 review, but on reading this PR I don't think it's the right shape for sandlock right now. The builder forces:

• The run → execute rename (only because the deprecated static run blocks the name)
• A two-phase configure/execute model where run takes &mut self, leaking the mem::take on pending_handlers into the type signature
• Two extra tokens (Sandbox::new(&p)?.run(&c) vs Sandbox::run(&p, &c)) on every common-case invocation, forever

What I actually wanted from "builder pattern" was the registration-site cleanup (no more Box::new(move |notif, ctx, fd| Box::pin(async move {...}))), per-syscall validation, and dropping the : HandlerFn ascription. All three come along by simply changing the handler parameter type on the existing run_with_extra_handlers to use your new Handler trait + Syscall::checked:

pub async fn run_with_extra_handlers<I, S, H>(
    policy: &Policy,
    cmd: &[&str],
    extra_handlers: I,
) -> Result<RunResult, SandlockError>
where
    I: IntoIterator<Item = (S, H)>,
    S: TryInto<Syscall, Error = SyscallError>,
    H: Handler,

Old call:

Sandbox::run_with_extra_handlers(&p, &c, vec![ExtraHandler::new(libc::SYS_openat, h)]).await?;

New call:

Sandbox::run_with_extra_handlers(&p, &c, [(libc::SYS_openat, openat_h)]).await?;

Same Handler trait, same Syscall::checked, same security boundary, no naming collision, no two-phase model. If we later expose more public Sandbox configuration (init_fn, on_bind, stdout_pipe, etc.) and a builder is warranted on its own merits, we can add Sandbox::builder() additively next to the existing run family then.

2. No deprecation cycle — hard break.

Sandlock doesn't maintain backwards compatibility before 1.0. The whole #[deprecated] apparatus — HandlerFnAdapter, the deprecated marks, the HandlerFn typedef, the ExtraHandler struct, the regression test bridging old↔new — should come out. Hard rename, mention the migration in the changelog.

Concrete asks

Keep, source-compatible with main:

• Sandbox::run(policy, cmd) — unchanged
• Sandbox::run_interactive(policy, cmd) — unchanged

Keep the names, change only the handler parameter shape:

• Sandbox::run_with_extra_handlers(policy, cmd, handlers) — handlers becomes IntoIterator<Item = (S, H)> instead of Vec<ExtraHandler>
• Sandbox::run_interactive_with_extra_handlers(policy, cmd, handlers) — same

Delete:

• The builder layer added by this PR: Sandbox::add_handler, Sandbox::execute, Sandbox::execute_interactive
• Sandbox::new from the public API surface — drop to pub(crate); users no longer need to construct a Sandbox directly
• pub type HandlerFn, pub struct ExtraHandler, ExtraHandler::new, HandlerFnAdapter
• BuilderError — fold validation errors into SandlockError (or a thin HandlerError if you prefer; no strong opinion)
• All #[deprecated] markers and the module-level #![allow(deprecated)] on tests/integration/test_extra_handlers.rs
• The deprecated_run_with_extras_observable_behavior_matches_add_handler regression test (bridging old↔new is no longer a thing once the old shape is gone)

Re-export at the crate root in lib.rs: Handler, HandlerCtx, Syscall, SyscallError. Right now users have to spell out sandlock_core::seccomp::dispatch::Handler etc.

What stays — keep all of it:

• Syscall(i64) newtype + Syscall::checked + TryFrom<i64> + SyscallError
• Handler trait, HandlerCtx<'a>, blanket impl<F, Fut> Handler for F
• validate_handler_syscalls_against_policy (the per-syscall variant)
• read_child_cstr promotion + the generic-phrasing fix on read_child_mem / write_child_mem doc comments
• All 21 builtin handler closures rewritten onto the new shape
• The 6 new integration tests (blanket-impl dispatch, struct state persistence, negative/arch-unknown rejection, insertion order, default-deny rejection) and the dispatch-level struct-handler unit test
• MAX_SYSCALL_NR + is_known_syscall in arch.rs
• Cargo.toml async-trait dep
• The docs/extension-handlers.md rewrite (will need a pass to unwind the builder examples and the Backwards-Compatibility / Migration sections, but the structure and the new content on interactive mode + child-mem reads + state patterns is keep-as-is)

Substance is right; just want to land it with a simpler shape. Happy to discuss if any of the above feels off.

[dzerik]

Thanks for the careful review — pushed v2 (force-push, 6 commits replacing the previous 14).

Done per the asks:

• Builder layer (Sandbox::add_handler / execute / execute_interactive) and BuilderError removed. Validation errors fold into a thin HandlerError enum surfaced as SandlockError::Handler(HandlerError).
• run_with_extra_handlers and run_interactive_with_extra_handlers now take I: IntoIterator<Item = (S, H)> where S: TryInto<Syscall, Error = SyscallError>, H: Handler. No rename, no two-phase model, no source-incompatible change to the existing run / run_interactive.
• pub struct ExtraHandler, pub type HandlerFn, HandlerFnAdapter, all #[deprecated] marks, the module-level #![allow(deprecated)], and the deprecated-path adapter-regression test all removed.
• Handler, HandlerCtx, HandlerError, Syscall, SyscallError re-exported at the crate root.
• Doc unwound: builder examples replaced with run_with_extra_handlers examples, Backwards-Compatibility and Migration-from-ExtraHandler sections deleted.
• During self-review I also dropped a small impl Handler for Arc<dyn Handler> blanket that an earlier draft had — it wasn't part of your asks, and the chain-ordering integration tests now use a homogeneous struct handler (Counter / OrderTracker with id + configurable action) instead of trait-object erasure, which is cleaner and removes that public-surface footnote.

One ask I'd like to clarify before going further:

• Sandbox::new left as pub. Trying to drop it to pub(crate) surfaced that it isn't only used by the kill/pause/resume lifecycle tests — it's the entry point for an entire alternative public API surface: spawn / spawn_captured / spawn_with_io / spawn_with_gather_io / wait / pause / resume / kill / commit / abort_branch / fork / reduce / checkpoint, all of which take &mut self on a Sandbox first constructed via new. This pattern is what supports long-running supervised processes (and what pipeline.rs uses internally too). Did you intend to retire that whole alternative API in favour of run_with_extra_handlers only, or just remove the add_handler builder layer that this PR added? Happy to follow whichever direction — pub(crate)-ing new plus the spawn/lifecycle suite is a meaningfully larger surface change and seemed worth a check before doing it.

Tests / CI: 235 lib unit + 14 integration extra-handler tests pass; workspace builds with zero deprecation warnings and zero clippy errors. Branch rebased onto current main.

[dzerik]

Rebased onto current main (your name runtime parameter, arch::FORK_LIKE_SYSCALLS, policy.virtual_hostname, argv_safety_required, c218e28's /bin/pwd+SYS_getcwd test fix all picked up). PR is mergeable again; same 6-commit shape over the new base.

One note on the final test result locally: builtin_non_continue_blocks_extra (the cat /proc/1/cmdline; cat /etc/hostname carry-over from PR #20) fails for me on this rebase even though it passes on the v2-pre-rebase tip and presumably passes on your CI. The /etc/hostname path doesn't appear in the observed-paths list — only the dynamic-linker probes do — which suggests cat no longer reaches the second argument in my environment. I haven't isolated whether it's a SysV-IPC default-deny interaction (3816e8a), an argv-safety effect (3a87632), or a local-environment thing. I'd appreciate eyes on this from your CI run — the behaviour change isn't from anything in this PR's scope (the test body is unchanged from carry-over, only the IntoIterator call-site was rewritten), but it'll be more useful with confirmation either way.

Everything else still holds: 259 lib unit tests pass, the other 13 carry-over+new integration tests pass, clippy clean, zero deprecation warnings.

[dzerik]

Found and fixed the builtin_non_continue_blocks_extra failure I flagged earlier — it was a stale path in the carry-over test (/etc/hostname from PR #20) that you had already updated to /etc/passwd on main. Picked up your version of the path; CI is now fully green (8/8 checks).

[congwang-mk]

Thanks for the rework. The hard-break direction is right and the body of the change is in good shape — three things I'd like resolved before merge.

1. Homogeneous-iterator constraint regresses multi-handler registration

run_with_extra_handlers<I, S, H> constrains every entry to a single concrete H. The old Vec<ExtraHandler> was already type-erased via Box<dyn Fn>, so callers could mix handler shapes freely; the new shape forces every downstream that registers more than one handler kind to write a wrapper enum (the S3Handler / DownstreamHandler pattern from the doc, repeated per crate). That's a real ergonomics regression vs. PR #20.

The fix is small and lives upstream:

#[async_trait]
impl<H: Handler + ?Sized> Handler for Box<H> {
    async fn handle(&self, cx: &HandlerCtx<'_>) -> NotifAction {
        (**self).handle(cx).await
    }
}

Then downstream can write Vec<(i64, Box<dyn Handler>)> with Box::new(h_open) / Box::new(h_close) of any concrete types — H resolves to one type, the per-crate adapter enum disappears, and &cmd, [...] array-literal call-sites still work.

The doc note ("sandlock-core does not ship a built-in erasure to keep the public surface minimal") is the wrong tradeoff: 8 lines upstream vs. 12 lines × every downstream forever. Please add the blanket impl (and an Arc<H> mirror) and drop the wrapper-enum guidance from extension-handlers.md.

2. #[async_trait] vs. native AFIT

Adds a proc-macro dep, boxes a future on every dispatch, and forces #[async_trait::async_trait] on every downstream impl block. Native async-fn-in-trait has been stable since Rust 1.75 (Dec 2023); the workspace is on edition 2021 with no rust-version pin. Unless there's a concrete MSRV blocker, the trait should be:

pub trait Handler: Send + Sync + 'static {
    fn handle<'a>(
        &'a self,
        cx: &'a HandlerCtx<'_>,
    ) -> impl Future<Output = NotifAction> + Send + 'a;
}

Drops the dep, drops the per-call Box::pin, and downstream impl blocks lose the macro. If you've considered AFIT and ruled it out, please document the reason in the PR description; otherwise let's switch.

3. Don't expose SupervisorCtx at all

pub sup: &'a Arc<SupervisorCtx> makes every public field of SupervisorCtx (processes, network, chroot, cow, netlink, time_random, …) part of the downstream extension contract. The dispatch builtins already reach into sup.processes / sup.cow / etc., so any internal restructure of SupervisorCtx becomes a breaking API change downstream. Please drop it from the public surface.

Concretely:

pub struct HandlerCtx {
    pub notif: SeccompNotif,
    pub notif_fd: RawFd,
}

No sup field, no 'a parameter. Public Handler impls become fn handle(&self, cx: &HandlerCtx) — no lifetime threading.

Builtins keep working by capturing the sub-Arcs they need at table-build time instead of going through cx.sup. build_dispatch_table takes &Arc<SupervisorCtx> (still pub(crate)), and each builtin closure clones the specific field it needs once at registration:

let network = Arc::clone(&ctx.network);
table.register(libc::SYS_connect, move |cx: &HandlerCtx| {
    let network = Arc::clone(&network);
    async move {
        crate::network::handle_net(&cx.notif, &network, cx.notif_fd).await
    }
});

Same pattern for processes / cow / chroot / netlink / time_random; the chroot_handler! and cow_call! macros take ctx: and clone the relevant fields at expansion. Mechanical change — each of the ~30 builtin closures already does Arc::clone(cx.sup) followed by Arc::clone(&sup.foo), so this is one line per closure plus a captured-binding above.

Benefits:

• SupervisorCtx itself can drop from pub to pub(crate) (please verify nothing else re-exports it).
• Adding a field to SupervisorCtx becomes a non-breaking change forever.
• No HandlerCtx<'a> lifetime parameter in the public API.
• The "future task wants Arc<SupervisorCtx> to spawn helpers" justification still works for builtins — they capture Arc::clone(ctx) at table-build time and stash it in the closure. None of it leaks into HandlerCtx.

---

Separately: the PR body still describes the original add_handler / Sandbox::execute / #[deprecated] / 14-commits sketch, none of which is in the diff. Please rewrite it before merge so the merge commit message matches what shipped.

[dzerik]

Pushed three follow-up commits addressing all three asks; PR title and body rewritten to match what shipped.

1. Box / Arc blanket impls. Added impl Handler for Box<dyn Handler> and impl Handler for Arc<dyn Handler> so callers mixing different handler types in one iterator just Box::new(...) per entry — wrapper-enum guidance dropped from the doc. Note: these are concrete Box<dyn Handler> / Arc<dyn Handler> impls rather than the <H: Handler + ?Sized> blanket form you sketched, because the latter has a coherence overlap with the existing impl<F, Fut> Handler for F where F: Fn(...) -> Fut blanket — the compiler rejects it. Concrete impls cover the same use case (any Box<dyn Handler> / Arc<dyn Handler>).

2. Drop async-trait macro dep. Done — async_trait = "0.1" removed from Cargo.toml, #[async_trait] attributes removed from the trait + every impl. One nuance worth flagging: I tried your exact fn handle<'a>(&'a self, cx: &'a HandlerCtx) -> impl Future<Output = NotifAction> + Send + 'a shape first; it doesn't compile because RPITIT in trait position is not object-safe today, and the supervisor stores user handlers as Vec<Arc<dyn Handler>>. The trait now uses Pin<Box<dyn Future<Output = NotifAction> + Send + 'a>> explicitly — same Box::pin per dispatch the macro was generating, but with the proc-macro dep gone and the trait still dyn-compatible. If you'd prefer to change the storage shape away from dyn Handler to allow native AFIT, that's a meaningfully larger restructure and probably a separate PR; happy to do it if you want.

3. Drop SupervisorCtx from public API. HandlerCtx is now { pub notif, pub notif_fd } — no <'a> parameter, no sup field. seccomp::ctx and seccomp::state are pub(crate). Builtins capture the specific sub-Arcs (ctx.chroot, ctx.cow, ctx.processes, etc.) at table-build time inside the chroot_handler! / chroot_handler_fallthrough! / cow_call! macros and the per-syscall closures — cx.sup no longer exists internally either. DispatchTable::dispatch is pub(crate) and dropped its ctx parameter. Continue-site safety doc rewritten to reflect the simpler contract (handler-owned mutexes only).

4. PR body and title rewritten to match the shipped diff — no more references to Sandbox::add_handler / execute / #[deprecated] / 14-commits.

CI green on both targets, 259 lib unit + 14 integration extra-handler tests pass, clippy clean, zero deprecation warnings.