Conversation
…aded seccomp Replace the hand-maintained BPF jump-offset table in CSystemCallFilter_Linux.cc with a program builder that derives every jump from the allowlist vector's own size/index. The applied program is generated from CPytorchInferenceSyscallAllowlist.h, a single machine-readable declaration, instead of a parallel hardcoded list (design.md V3/MG6). CSystemCallFilter::installSystemCallFilter() returns a typed ESystemCallFilterInstallOutcome across all three platform implementations instead of void, and logs the ml.seccomp.installed readiness marker on success. pytorch_inference/Main.cc gains a decideDegradedModeAction() decision that would terminate before CIoManager::initIo() on any degraded-mode seccomp failure; that termination stays behind an internal switch defaulting to false until ml-cpp PR E's typed controller routing can guarantee a degraded-mode launch was deliberate (design.md MG8, "activation is deliberately split across two slices"). The four non-PyTorch callers now make their unchanged log-and-continue policy explicit instead of silently discarding the result. Adds CSeccompFilterBuilderTest.cc: decodes the actually-built BPF program to prove it matches the declaration and that jump offsets are derived, not hand-maintained; fault-injected coverage of decideDegradedModeAction() for every install-failure class (V13); a named regression test per design.md M4 carry-forward syscall (clone3 by number, prlimit64, x86_64 legacy filesystem syscalls) so a blank-slate reconstruction cannot silently drop a hard-won compatibility fix from frozen PR elastic#2873; and an explicit structured degradedModeAttestationMarker() (design.md M2) so a controller/ES observer can assert seccomp installation directly instead of inferring it from the absence of a fatal log line. The Sandbox2-grants half of V3 (CPytorchInferenceSandboxPolicy.cc) does not exist yet in this clean rebuild lineage (ml-cpp PR C); this change establishes the single declaration for that PR to consume. d9a856d (Sandbox2 AllowFutexOp arg-filtering) and 730933d/f8b0a534 (Buildkite run_tests.sh/build.sh packaging) are likewise out of this file's scope - see the bd note on elastic-workspace-3b59.3 for the explicit deferral to PR C/D/E. Verified on a real x86_64 devbox: ml_test_seccomp (7 test cases) and the real pytorch_inference/autodetect/categorize/normalize/ data_frame_analyzer binaries all build, link, and run correctly against the new typed API; the generated BPF program installs via a real prctl(PR_SET_SECCOMP) call.
…rence Replace raw argument-directory inference with a typed launch spec that validates every input/output/restore/logPipe path against the pinned child-root contract ($TMPDIR/ml-child-ipc/<child-id>): rejects relative, root, out-of-root, dot-dot, duplicate, and mutable-symlink/alias paths before any policy is built. Adds the minimized fixed-mount enumeration (narrows /etc to individually justified files, never binds host /proc//sys), a private bounded tmpfs at /tmp, and a purpose-built allowlisted mechanism probe (ml_sandbox_probe) proving allowed IPC access, denied host reads/writes, mount enumeration, and external-egress denial. Validator logic verified standalone on this host (non-Sandbox2 path): compiles clean with -Wall -Wextra, and a driver exercising every V16 rejection case (relative/root/dot-dot/duplicate/aliased/wrong-depth/ child-id-mismatch) plus the valid multi-pipe case all pass. The Sandbox2-gated PolicyBuilder path and the Linux mechanism-probe integration test are unverified in this session (no Linux/Sandbox2 toolchain on this host) and need a devbox or Buildkite pass before V4/V5(policy half)/V7/V16 can be marked closed - see elastic-workspace-3b59.4.
…process spawner (header only) Adds include/sandbox/CSandboxedProcessSpawner.h declaring the explicit child lifecycle state enum (Prepared/Launched/IdentityCaptured/ Registered/Monitoring/TerminationRequested/CleanupRequired/Reaped/ Failed, design.md's mermaid diagram), a registry-entry shape (SSandboxedChild) carrying that state, a generation counter, and pidfd/Sandbox2 handle fields, and a one-shot CAS-controlled outcome latch (CCasOutcomeLatch: Pending -> TimedOut|Completed via a single compare_exchange_strong) replacing the two-boolean timeout/completion coordination in the frozen enhancement/sandbox2 reference. Declares the public API (spawn/terminateChild/hasChild) with no injectable seams yet - those are PR D Task 2's contract. No .cc file and no CMakeLists change: lib/sandbox/CMakeLists.txt's SRCS only lists .cc files, so a header-only commit needs none. Header-only; verified with a standalone g++ -fsyntax-only -std=c++17 -Wall -Wextra probe against this worktree's include/ (no Sandbox2 toolchain on this host - see docs/projects/mlcpp-sandbox2-pr2873/ pr-d-lifecycle.plan.md Task 1).
E_IdentityCaptured is not present in design.md's mermaid diagram (only 8 states); it comes from the Sandbox2 rebuild plan's explicit Prepared->Launched->IdentityCaptured->Registered->Monitoring->Reaped sequence. Corrected the class-level and enum-level doc comments to cite the rebuild plan as the source for that state and stop claiming one-for-one fidelity to design.md's diagram, which covers a related but not identical lifecycle.
Implements Task 2 of the PR D Sandbox2 lifecycle rebuild: a new CSandboxedProcessSpawner_Linux.cc gives spawn() its full skeleton, calling PR C's validateChildIpcLaunchSpec()/buildPytorchInferenceFilesystemPolicy() as an interlock before TryBuild(), and arming a non-throwing CKillAndReapGuard immediately after RunAsync()/pid() capture (E_IdentityCaptured) so every early-return before registry insertion and monitor handoff both succeed (E_Monitoring, LI2) routes through one cleanup owner instead of a duplicate catch block (LI1/LI3, closes MG3). Adds four injectable seams (pidfd acquisition, registry allocation, monitor-thread creation/detach, Sandbox2 completion) with production defaults and a test-only constructor overload; SSandboxedChild/SPidRegistry move from private to public so the seam signatures are nameable by test code. The new .cc is added to lib/sandbox/CMakeLists.txt's SRCS unconditionally, matching lib/core/CMakeLists.txt's CDetachedProcessSpawner.cc pattern, with #ifdef SANDBOX2_AVAILABLE gating the real logic inside. Does not implement pidfd-error classification or terminateChild()'s real logic - Task 3 scope. No numeric kill(pid) fallback anywhere in this file.
CKillAndReapGuard held a raw, non-owning Sandbox2* declared before the shared_ptr locals (sandbox, child.s_Sandbox) that could become its last owner; on the registry-insert-throw and monitor-launch-fail paths those locals were destroyed before the guard, freeing the object before the guard's destructor called Kill() on it. The guard now stores its own shared_ptr<Sandbox2> copy, making its cleanup self-sufficient regardless of other locals' declaration order; the unique_ptr->shared_ptr conversion moves earlier (before RunAsync()) so the guard can be constructed from a real shared_ptr once the process is actually running. Also wrap the span between successful registry insertion and monitor launch (copying seams, constructing monitorBody) in try/catch, so an exception there (e.g. bad_alloc copying a std::function) can no longer escape spawn() leaking the registry entry; erase-on-failure logic is factored into a shared lambda used by both the new catch and the existing monitor-launch-false path.
…terminateChild() Adds EPidFdOutcome (E_Acquired/E_KernelUnsupported/E_Failed) and a pure classifyPidFdOutcome() so spawn() fails registration outright on any non-ENOSYS pidfd_open errno (MG2/LI8), and records the classification on the registry entry at registration time. Implements terminateChild() for real: pidfd_send_signal(SIGTERM) request for E_Acquired, Sandbox2::Kill() (SIGKILL via the owned monitor) for E_KernelUnsupported only - selected from the recorded classification, never re-derived. Wires the monitor-body completion path through CCasOutcomeLatch::tryResolve() (MG4/V11) instead of an ad-hoc boolean. No numeric kill(pid) call exists anywhere in the file.
Fix round 1/5 for Task 3 review: terminateChild() set s_State to E_TerminationRequested before releasing the registry lock and attempting the actual pidfd_send_signal()/Sandbox2::Kill() call. On failure of that call it returned false but left s_State claiming a termination request had been issued, with no rollback and no distinguishable "attempted and failed" state. Capture the entry's prior state before overwriting it, and on each failure path (pidfd_send_signal() non-zero return, the defensive neither-pidfd-nor-sandbox branch, and Sandbox2::Kill() throwing) re-acquire the registry lock briefly to roll s_State back to what it was, but only if nothing else has since moved the state on. The syscall/Kill() call itself still happens outside the lock, unchanged.
Implements the 8 required lifecycle proofs (pidfd classification, allocation/monitor-launch failure, stale generation, PID reuse, timeout/completion race, descriptor baseline, destructor latency, monitor-outlives-spawner) by driving CSandboxedProcessSpawner's injectable seams against one real spawn() per case, since no seam bypasses RunAsync() itself and the private registry has no external accessor other than the registry-insert seam's mutable reference. Adds lifecycle_signal_payload.cc, a long-lived sandboxee that catches and survives SIGTERM but not SIGKILL (using FUTEX_WAIT, since pause() is not in the seccomp allowlist), needed to distinguish terminateChild()'s two mechanisms by observable effect - there is no seam around Sandbox2::Kill() itself. See task-4-report.md for the full list of self-review findings, open items (no TSan/ASan CI job exists yet; ML_SANDBOX2_REQUIRE gating referenced in the brief does not exist in this checkout; gate 7's orphan-cleanup half is explicitly out of scope), and what could/couldn't be verified on this macOS host. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
testTerminateChildFallsBackToKillWhenKernelUnsupportsPidfd now asserts reason_code() == SIGKILL in addition to final_status() == SIGNALED, so a regression sending a different signal no longer passes silently. The monitorBody() call that drives AwaitResult() is now run on a bounded background thread (join on success, detach on timeout) since production AwaitResult() has no wall-clock limit and a Kill()-regressed-to-no-op would otherwise hang indefinitely instead of failing fast.
capturedResult's outer shared_ptr was not captured by the detached monitor thread's lambda, only its raw pointer traveled via awaitResultFn's closure. On the timeout/detach regression path, the test case's stack unwind (BOOST_TEST_REQUIRE failure) freed the heap-boxed inner shared_ptr<Result> while the still-running detached thread held a dangling raw pointer into it, ready to write through it once AwaitResult() unblocked. Capture capturedResult by value in the detached thread's own lambda so its copy keeps the object alive for as long as the thread might still run, independent of the test case's lifetime.
…ead lifecycle states Final review fix wave for PR D (Sandbox2 lifecycle rebuild), all 6 findings in one pass: - C1/I3: terminateChild() now performs pidfd_send_signal() for the E_Acquired branch while still holding s_Mutex, closing the race against monitorBody's completion handler closing/recycling the same pidfd underneath a delayed signal. rollBackState() now matches on both s_Generation and s_State, so a stale rollback cannot clobber a newer registration that reused the same numeric PID. - I1: wire up E_Monitoring (after monitor handoff is confirmed in spawn()) and E_Reaped (immediately before the registry erase in monitorBody), generation-matched under the lock. - I2: reset childPid to 0 immediately after capturing sandboxPid, and only set it to the live PID at the very end of spawn(), so an uncaught throw between guard-arm and the first try/catch can no longer leave childPid non-zero on a false/throw exit. - I4: lifecycle test assertions that a fresh pidfd_open() succeeds on an already-killed-and-reaped PID now also accept ESRCH (fully reaped) as proof of cleanup, in the 3 affected test cases. - I5: realMonitorLaunchWithCompletionSignal destroys the monitor closure (and its captured Sandbox2 handle) before signaling completion, so a waiting test cannot observe "done" while fds may still be open. - I6: add a BOOST_GLOBAL_FIXTURE that warms up Sandbox2's forkserver before any per-case fd-baseline snapshot is taken, removing the test-ordering dependency shared with gate 4's fork().
…ng, guard E_Monitoring write Final whole-branch review of the fix wave (b42ee96) surfaced three new, small issues in that very wave: - LOG_INFO printed childPid before the I2 fix restores it, always logging "PID 0" on success. - The I6 forkserver warm-up fixture called AwaitResult() unconditionally even when terminateChild() failed, risking an unbounded hang of the BOOST_GLOBAL_FIXTURE (and therefore the whole test binary) if Sandbox2::Kill() ever throws. - The new E_Monitoring write was unconditional on generation match alone, which could silently revert an E_TerminationRequested marker set by a concurrent registry-scanning terminator (PR E's future timeout caller). Applied directly per the SDD final-review adjudication rule (no second fix-wave dispatch): all three are small, load-bearing, and closing them now is cheaper than a PR E regression hunt.
…8 UAF, gate 3 fd leak, promise UAF, log classification - Gate 1 (testTerminateChildFallsBackToKillWhenKernelUnsupportsPidfd): Sandbox2::Kill() produces EXTERNAL_KILL/reason_code()==0, not SIGNALED/SIGKILL - fix the assertion to match the pinned sandboxed-api v20241008 status classification order. - realMonitorLaunchWithCompletionSignal: stop destroying the monitor closure (and its co-owned shared_ptr<SPidRegistry>) before signalling completion; hand the caller its own shared_ptr<function<void()>> reference instead. Fixes a deterministic UAF in gate 8, which dereferenced a raw SPidRegistry* after the only remaining owning shared_ptr had already been released. Also switch its promise parameter to shared_ptr, matching the existing ENOSYS-case precedent, so a throw between spawn() and the wait can no longer free a stack-local promise out from under the still-running detached thread. - Gate 3 (testStaleMonitorGenerationCannotEraseNewerRegistration): close the real pidfd the case's fabricated stale-generation scenario leaves open, so SFdBaselineFixture's descriptor count matches the baseline. - logSandboxeeTermination: give EXTERNAL_KILL, VIOLATION, TIMEOUT, SETUP_ERROR and INTERNAL_ERROR their own log cases instead of funnelling them into one generic LOG_ERROR - EXTERNAL_KILL is this file's own successful ENOSYS-fallback path, and VIOLATION is the most operationally important signal a sandbox can report. - terminateChild(): document that a repeated call reaching Sandbox2::Kill() twice is safe (idempotent at the pinned tag) - considered during review, confirmed harmless, recorded so it isn't re-derived next time.
…checks Round-3 self-review M1: testDescriptorCountReturnsToBaselineAfterSpawnTerminateCleanup asserted the fd baseline while its local monitorBody std::function still held the closure's captured shared_ptr<Sandbox2>, keeping the comms socketpair fd open. Release it (monitorBody = nullptr) before the check. S1: realMonitorLaunchWithCompletionSignal's detached thread called set_value() while still holding its own bodyPtr reference, reintroducing the I5 ordering hazard (a waiter could observe "done" before the thread released its Sandbox2 handle). Reset bodyPtr before signalling. Checked gates 7/8 for the same pattern: neither has an inline fd assertion in-scope with a handle-holding local (their only fd check is SFdBaselineFixture's post-return destructor), so no change needed there.
Contributor
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
PR D of the Sandbox2 clean rebuild epic (docs/projects/mlcpp-sandbox2-pr2873 in the elastic-workspace harness). Stacked on PR C (#3184), which stacks on PR B (#3182), which stacks on PR A (#3181).
Rebuilds
CSandboxedProcessSpawner(new) around an explicit lifecycle state machine (Prepared -> Launched -> IdentityCaptured -> Registered -> Monitoring -> TerminationRequested -> CleanupRequired -> Reaped/Failed):RunAsync()/pid()capture, holding its own owningshared_ptr<sandbox2::Sandbox2>so cleanup is order-independent during unwinding.ENOSYSis the only classification that routes toSandbox2::Kill()(SIGKILL via the owned ptrace monitor, identity-safe, no numeric-PID lookup); every other pidfd error fails registration outright; numerickill(pid, ...)does not appear anywhere in this file.CCasOutcomeLatch) replacing two-boolean coordination for the timeout-vs-completion race.CSandboxedProcessSpawnerLifecycleTest_Linux.cc: fault-injection coverage for every pidfd class, allocation/resource failures, stale-generation protection, PID-reuse identity binding, the CAS race, descriptor-baseline cleanup, and destructor-latency (V8/V9/V10/V11, LI1-LI9 per design.md).Notes
docs/projects/mlcpp-sandbox2-pr2873/pr-d-lifecycle.plan.mdin the elastic-workspace harness repo.E_Launched/E_CleanupRequiredlifecycle states are declared but intentionally left unassigned (no natural single point without broader restructuring) — not silently dropped.Sandbox2::Kill()/SIGKILL-only assumption (no graceful-SIGTERM-via-monitor variant is buildable against pinnedsandboxed-api v20241008) was cross-checked against the vendored monitor source during review.