shell: make SubprocExec child/resume backrefs Option types

RO
3mo ago
+82-74

Problem

  • No user-visible bug. SubprocExec, the state a shell Cmd keeps while an external process runs, held two raw pointers that each used null to mean a state.
  • child is null from staging until the spawn writes the subprocess back. The two deinit paths null-checked it; the pipe-close paths dereferenced it blindly.
  • interp (paired with this_id) is deliberately null until the spawn call has returned, so a callback firing inside the spawn records its result instead of resuming the command. Both callbacks tested it for null.
  • That pair is a hand-rolled copy of the CmdHandle the same function already builds and passes to the spawn.

Fix

  • child becomes Option<NonNull<ShellSubprocess>>; interp and this_id become resume: Option<CmdHandle>, set at the same points the pointers were. Null checks become Some/None matches; the spawn's out pointer changes type to match.
  • Nothing changes at runtime: both Options use the null niche, so the struct stays 80 bytes and each None test is the null compare it replaces. The three derefs that were unchecked stay unchecked. One unsafe block removed, none added.
  • Verification: no new test. cargo check and clippy are clean, a debug build succeeds, and the shell test files were run (498 pass). The 9 failures are reported as pre-existing on main, caused by /dev/null being a regular file on the test machine.

Background

  • The shell runs a script as a state machine. Every node, including each Cmd, lives by index (NodeId) in a Vec owned by the Interpreter, which can reallocate, so nodes hold indices rather than pointers to each other. A step returns a Yield that a trampoline drives onward.
  • A Cmd running an external process owns a heap-allocated subprocess. Process exit and pipe close arrive as callbacks holding only &mut Cmd, so the Cmd must stash how to reach the interpreter to resume itself.
  • Those callbacks can fire synchronously inside the spawn call. Resuming from there would tear the Cmd down under the spawn's own borrow, so the resume handle is published only after the spawn returns; an earlier callback records its result and the spawn path resumes.
  • CmdHandle is (ParentRef<Interpreter>, NodeId), the handle the subprocess itself holds. ParentRef wraps a non-null pointer, so Option<CmdHandle>, like Option<NonNull<T>>, is pointer-sized.
Original description

What

SubprocExec (src/runtime/shell/states/Cmd.rs), the Exec::Subproc payload of a shell Cmd, stored two raw pointers that each used null as a state. child: *mut ShellSubprocess is null from the moment the slot is staged until ShellSubprocess::spawn_async writes the freshly allocated subprocess through its out pointer, and was null-checked in deinit / deinit_from_finalizer but dereferenced blindly on the pipe-close paths. interp: *mut Interpreter (paired with this_id: NodeId) is deliberately left null until the spawn call has returned, so that a Cmd::on_exit or Cmd::buffered_output_close firing synchronously inside the spawn records its result instead of driving the trampoline while the spawn frame still borrows the subprocess; both callbacks tested it for null before yielding. The (interp, this_id) pair is exactly the subproc::CmdHandle { interp: ParentRef<Interpreter, Mut>, id: NodeId } that transition_to_exec already builds a few lines later and hands to spawn_async.

// before
pub struct SubprocExec {
    pub(crate) child: *mut ShellSubprocess,
    pub(crate) buffered_closed: BufferedIoClosed,
    pub(crate) interp: *mut Interpreter,
    pub(crate) this_id: NodeId,
}
exec.interp = interp_ptr;                               // publish after spawn
let (interp, this_id) = (sub.interp, sub.this_id);      // on_exit
if interp.is_null() { return; }
Yield::Next(this_id).run(unsafe { &*interp });

// after
pub struct SubprocExec {
    pub(crate) child: Option<NonNull<ShellSubprocess>>,
    pub(crate) buffered_closed: BufferedIoClosed,
    pub(crate) resume: Option<CmdHandle>,
}
exec.resume = Some(cmd_parent);                         // publish after spawn
let Some(handle) = sub.resume else { return };          // on_exit
Yield::Next(handle.id).run(&handle.interp);

In Cmd.rs the staging literal writes None for both fields, child_out becomes *mut Option<NonNull<ShellSubprocess>>, the post-spawn read-back derefs with unwrap_unchecked().as_mut() inside its existing unsafe block (whose SAFETY comment already stated that Ok implies the pointer was written), deinit and deinit_from_finalizer replace their !is_null() match guards with if let Some(child), buffered_output_close matches on resume, the two buffered_output_close_{stdout,stderr} derefs stay inside their existing unsafe blocks, and on_exit runs the Yield through the handle's ParentRef Deref, which removes its unsafe { &*interp } block. In subproc.rs, spawn_async and spawn_maybe_sync_impl take *mut Option<NonNull<Self>>, and the write site derives the NonNull from the same &mut *slot reborrow the raw subprocess pointer was already derived from, so the provenance chain is unchanged. 10 sites in Cmd.rs, 3 in subproc.rs; removes 1 unsafe block and adds none; 2 files, +82/-74. IOWriter and IOReader already keep their interpreter backref as Option<ParentRef<Interpreter>>, so this brings the third shell backref into the same shape.

Why

"Not yet spawned" and "spawn has not returned yet" are now None values a reader can see in the field types, rather than two null conventions documented in comments, and the resume path reuses the handle type the subprocess itself holds instead of a second hand-rolled copy of it. It is zero-cost: Option<NonNull<T>> is pointer-sized via the null niche and Option<CmdHandle> takes the niche of its ParentRef (repr(transparent) over NonNull), so SubprocExec stays 80 bytes with the same field sizes; each remaining Some/None test compiles to the null compare it replaces, the three blind derefs remain unchecked, and no allocation, copy or branch is added on any path.

Part of a series of small type-system hardening changes; each PR stands alone.

Verification

cargo check and cargo clippy are clean for the touched crates. Debug build succeeds. test/js/bun/shell/{shell-pipe-read-fault,exec,pipeline_stack,bunshell}.test.ts: 498 pass, 2 skip, 83 todo, 9 fail (592 tests); the three smaller files pass fully.
The 9 failures are all in bunshell.test.ts (condexprs "-c", "-f character device", and the "stdin redirect from a zero-length buffer" cases) and are pre-existing: they fail identically on main because /dev/null on this machine is a regular file rather than a character device (an earlier "> /dev/null" test writes "Bunception!" into it, which the empty-stdin children then read back); unrelated to the Cmd.rs/subproc.rs Option<NonNull> refactor.