Skip to content

refactor(runtime): confine and reduce unsafe code - #4130

Open
drew wants to merge 2 commits into
mainfrom
codex/unsafe-cleanup
Open

drew wants to merge 2 commits into
mainfrom
codex/unsafe-cleanup

Conversation

@drew

@drew drew commented Oct 2, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Reduce Rust unsafe syntax sites from 503 to 344 (159 removed), and confine the remaining sites to openshell-sandbox (238), openshell-driver-vm (56), and the explicitly excluded Windows MXC driver (50). Replace ordinary descriptor, resource, identity and build configuration operations with safe APIs while retaining explicit kernel ABI, fork/pre-exec and FFI boundaries.

Related Issue

Directly requested maintainer cleanup. No matching accepted issue is currently linked. This is a draft because moving the Linux mechanism module across crate boundaries requires an accepted issue reference before the PR is ready.

Changes

  • Move Linux isolation mechanisms and qualification probes from openshell-isolation-interface::linux to openshell-sandbox::linux; migrate all in-repository consumers and leave the backend interface dependency-light.
  • Add forbid(unsafe_code) to the CLI, supervisor, sandbox backend and isolation interface targets, and network supervisor library.
  • Replace ordinary resource limits, identity/group operations, socket options, sealed provider-file construction, PTY sizing, descriptor setup and probe I/O with safe nix/rustix/standard-library APIs. Preserve syscall selection, flags, errno, native-endian eventfd transfers and exact intercepted/denied syscall tests.
  • Replace global test-environment mutations with isolated subprocess fixtures and completion markers. Keep existing lookup/precedence assertions and fixtures.
  • Configure vendored protobuf compilation explicitly; handle ambient PROTOC_INCLUDE in a child process rather than mutating build-script globals.
  • Use process-local SSH verbosity defaults with explicit child-environment propagation, asynchronous foreground signal handling, and a stdin-based private VM-supervisor liveness handoff. Add verbosity, foreground-signal and liveness regressions.
  • Keep allocation instrumentation in the VM crate's portable, default-feature-disabled test feature; preserve allocation and query measurements.
  • Document crate ownership in the relevant READMEs. The saved source catalog and detailed reviews remain in the local ignored plans/unsafe-catalog/ directory; the current crate summary is below.
Crate Remaining Rust unsafe sites
openshell-sandbox 238 (135 non-test/probes, 103 tests)
openshell-driver-vm 56 (47 non-test, 9 test allocator sites)
openshell-driver-mxc 50 (excluded from cleanup)
All other crates 0
Total 344

Counts include unsafe blocks, functions, implementations and FFI function-pointer declarations, not individual syscalls. Relocated sites are not counted as removals. Generated Go unsafe calls are unchanged.

Compatibility details: external Rust users of openshell_isolation_interface::linux must migrate their imports. The private driver/supervisor flag is now --parent-liveness-stdin, so those binaries must be built together. Tokio foreground signal handlers remain installed for the process lifetime. The IsolationBackend contract and public wire protocol are unchanged.

Testing

  • mise run pre-commit passes, including workspace and sandbox perf-harness lint checks.
  • Full RUST_TEST_THREADS=4 mise run test passes after the final descriptor batch. The initial run had two supervisor-network plaintext failures; all five plaintext tests passed on a serial retry, and both failures also passed in the full-suite retry. Their initial failure cause is not proven.
  • Existing tests preserved and focused regressions added: the capability review retained all 1,521 original tests in changed files, with 1,527 tests declared after additions; final descriptor cleanup retained all function/test names.
  • OPENSHELL_E2E_DOCKER_TEST=transparent_tcp mise run e2e:docker passes after the final batch: six CLI conformance scenarios and native TCP policy-DNS/fail-closed coverage. The Podman-specific scenario is skipped in the Docker lane.
  • Earlier full cleanup validation passed 18 Docker lifecycle scenarios, minimal VM allocation-feature compilation, explicit proxy allocation/query baseline, and protobuf builds with invalid ambient tooling settings.
  • AST unsafe inventory, source locations, crate boundaries and git diff --check verified.
  • Full mise run ci: attempted, but stopped at 20 Go lint errors in unchanged SDK files (5 errcheck, 12 revive, 3 staticcheck).
  • Native macOS/Windows and live VM boot/vsock validation. The macOS cross-build attempt stopped in aws-lc's C build because the Linux host lacks the Apple compiler/SDK; Windows MXC was not modified.

Checklist

  • Follows Conventional Commits.
  • Commits are signed off (DCO).
  • Crate architecture READMEs updated; agent skill references reviewed for drift.
  • Accepted issue linked for the crate-boundary change.

Rebase validation

Rebased onto main at a48920ac0. Retained main's driver-independent mTLS/bearer TLS behavior, bearer-passthrough exposure assertions, 0600 provider memfd metadata and new upload/broker tests. Pre-commit and Docker transparent-TCP/conformance passed after rebase. The first full run timed out in main's new metadata-loopback broker test; that test passed alone on retry. The full RUST_TEST_THREADS=4 mise run test retry passed after the heavy Docker builds completed, including the metadata-loopback test and updated mTLS tests.

CI follow-up

Commit 06907512e fixes the macOS dead-code lint by compiling the boundary environment installer only on Linux, and fixes both Linux nextest signal-probe failures by publishing readiness from an executed Rust workload rather than checking the external sleep executable filename. The old failure was reproduced with a renamed binary; the revised fixture passes nextest with that PATH. Pre-commit and the full local test suite passed. Remote verification on commit 06907512e passed all three previously failing jobs in Branch Checks run 37051739763: macOS Rust lint and both Linux Rust test jobs. Other checks are still running; no failures are currently reported.

@copy-pr-bot

copy-pr-bot Bot commented Oct 2, 2026

Copy link
Copy Markdown

Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

Signed-off-by: Drew Newberry <anewberry@nvidia.com>
@drew
drew force-pushed the codex/unsafe-cleanup branch from 3be2384 to 3cf6200 Compare October 2, 2026 16:17
@drew
drew marked this pull request as ready for review October 2, 2026 16:18
@drew
drew requested review from a team, derekwaynecarr, mrunalp and sjenning as code owners October 2, 2026 16:18
@drew drew added the test:e2e Requires end-to-end coverage label Oct 2, 2026
@github-actions

github-actions Bot commented Oct 2, 2026

Copy link
Copy Markdown

Label test:e2e applied for 3cf6200. Open the existing run and click Re-run all jobs to execute with the label set. The run will execute the standard E2E suite after building the required gateway, sandbox, and supervisor images once. The matching required CI gate status on this PR will flip green automatically once the run finishes.

Signed-off-by: Drew Newberry <anewberry@nvidia.com>
@johntmyers johntmyers added the test:windows Run native Windows x64 and ARM64 lint/tests on PR mirrors label Oct 2, 2026
@johntmyers

Copy link
Copy Markdown
Collaborator

gator-agent

PR Review Status

The independent code review found no blocking defects in the unsafe-code cleanup. Drew, I checked your CI follow-up: the macOS lint and both Linux Rust test jobs now pass on this head, and the full E2E suite is green.

I applied test:windows because the shared protobuf build setup and supervisor's non-Unix path changed, then queued the current-head Windows MSVC workflow rerun. Gator will check its x64 and ARM64 results before handing off for maintainer approval.

Blocking findings: None.

Carried findings: None.

Gator metadata
  • Validation: Maintainer-authored runtime safety refactor; author verified as repository admin.
  • Docs: Public CLI, configuration and wire behavior preserved; crate READMEs and the maintainer's PR description explain the external Rust import migration and paired private liveness flag change. No published user workflow change identified.
  • Checks: Current-head Branch Checks, Helm Lint, Trivy Changes and DCO pass. Windows MSVC rerun queued; both x64 and ARM64 lint/test jobs must pass.
  • E2E: test:e2e applied; current-head Branch E2E Checks run 37051741133 passed, including Docker, Podman, VM and Kubernetes coverage.
  • Windows: test:windows applied; current-head run 37051739780 rerun queued.
  • Head SHA: 06907512e990533d1de19bda9177dbc7e4e4e2e0
  • Base SHA: a48920ac042554ae7cb17f56146e5c1e7881ccae
  • Merge base SHA: a48920ac042554ae7cb17f56146e5c1e7881ccae
  • Patch ID: 3aa2e97e56b4c3977b8883cab4e238c49563306c
  • Gator payload: 10
  • Review mode: initial
  • Previous reviewed SHA: none
  • Review budget exhausted: no
  • Maintainer decision required: no
  • Next state: gator:watch-pipeline

@johntmyers johntmyers added the gator:watch-pipeline Gator is monitoring PR CI/CD status label Oct 2, 2026
@drew drew removed the test:windows Run native Windows x64 and ARM64 lint/tests on PR mirrors label Oct 2, 2026
@johntmyers johntmyers added gator:blocked Gator is blocked by process or repository gates and removed gator:watch-pipeline Gator is monitoring PR CI/CD status labels Oct 2, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gator:blocked Gator is blocked by process or repository gates test:e2e Requires end-to-end coverage

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants