fix(child_process): spawn via posix_spawn to avoid a macOS fork/dyld deadlock - #7157
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughUnix child-process execution now resolves bare commands to absolute paths and uses ChangesUnix child-process resolution
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ChildProcessAPI
participant cp_command_for_program
participant cp_resolve_program_path
participant Command
ChildProcessAPI->>cp_command_for_program: provide program and options
cp_command_for_program->>cp_resolve_program_path: resolve bare program with effective PATH
cp_resolve_program_path-->>cp_command_for_program: absolute path or fallback
cp_command_for_program->>Command: construct command and preserve argv[0]
Command-->>ChildProcessAPI: execute child process
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…deadlock `std::process::Command` only uses `posix_spawn` when the program is given as a path and no `pre_exec`/uid/gid closures are set; a bare command name combined with an `env` option (which calls `env_clear()`) drops it onto the `fork`+`exec` fallback. On macOS a `fork` from Perry's multithreaded runtime (async reactor + GC/worker threads) can deadlock the child post-`exec` in dyld (`RemoteNotificationResponder::blockOnSynchronousEvent`): the child inherits locks/Mach state from parent threads that no longer exist after `fork`. The reader/waiter threads then block forever in `read()`/`wait4()` and the main loop idles in `js_wait_for_event`. Resolve a bare command name to its absolute path in the child's effective PATH before building the `Command`, so std stays on the `posix_spawn` fast path even when an `env`/`cwd` option is present; `argv[0]` is preserved via `arg0`. The `exec`/`execSync`/promisify(exec) shell now uses the absolute `/bin/sh` (Node's shell), and `spawn`/`spawnSync`/`execFile`/`execFileSync`/spawn_background all resolve their program. Verified with a dyld interposer: `exec`/`spawnSync` with an `env` option go from `fork()` to `posix_spawn` while stdout/exit-code capture and argv are unchanged. `detached` (setsid), `fork()`'s IPC dup2, and uid/gid necessarily keep std's fork path — `posix_spawn` can't express them via std — and are documented as such; they are outside the reported exec/spawn impact.
43fe6bc to
55737ce
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/perry-runtime/src/child_process/exec.rs (1)
49-55: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReduce triplication of the hardcoded
/bin/shshell path.
js_child_process_exec_sync,js_child_process_exec, andcp_promisified_execeach hardcodeCommand::new("/bin/sh")with a near-identical explanatory comment.cp_default_shell()inoptions.rsalready returns/bin/shfor non-Windows targets. Reuse it here. This keeps the shell-path decision in one place if it ever needs to change.♻️ Proposed refactor to reuse `cp_default_shell()`
- #[cfg(unix)] - let mut command = { - // Absolute path (Node's `exec` shell) keeps std on `posix_spawn` - // instead of the `fork`+`exec` fallback a bare "sh" + `env` triggers - // (the macOS fork/dyld deadlock fix — see `cp_command_for_program`). - let mut c = Command::new("/bin/sh"); + #[cfg(unix)] + let mut command = { + // Already-absolute (Node's `exec` shell) keeps std on `posix_spawn` + // instead of the `fork`+`exec` fallback a bare "sh" + `env` triggers + // (the macOS fork/dyld deadlock fix — see `cp_command_for_program`). + let mut c = Command::new(cp_default_shell()); c.arg("-c").arg(&cmd_str); c };Repeat the same substitution at the other two occurrences. Requires making
cp_default_shellpub(crate)inoptions.rsand re-exporting it frommod.rs:pub(crate) fn cp_default_shell() -> String { ... }pub(crate) use options::{..., cp_default_shell, ...};Also applies to: 284-292, 483-491
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/perry-runtime/src/child_process/exec.rs` around lines 49 - 55, Centralize the shell path by making cp_default_shell available as pub(crate) from options.rs and re-exporting it in mod.rs, then replace the hardcoded Command::new("/bin/sh") calls in js_child_process_exec_sync, js_child_process_exec, and cp_promisified_exec with the shared default-shell value while preserving their existing arguments and behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/perry-runtime/src/child_process/registry.rs`:
- Around line 88-136: Update the PATH selection in the command-resolution block
around cp_resolve_program_path to match cp_effective_path: when env_map is
present, use its child PATH value if available without falling back to the
parent environment; when env_map is absent, use CP_DEFAULT_PATH. Remove the
current parent-PATH and empty-string fallbacks so missing PATH never resolves
against the current directory.
---
Nitpick comments:
In `@crates/perry-runtime/src/child_process/exec.rs`:
- Around line 49-55: Centralize the shell path by making cp_default_shell
available as pub(crate) from options.rs and re-exporting it in mod.rs, then
replace the hardcoded Command::new("/bin/sh") calls in
js_child_process_exec_sync, js_child_process_exec, and cp_promisified_exec with
the shared default-shell value while preserving their existing arguments and
behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f46e58f6-7187-4eca-8aa8-f50435c6ba4f
📒 Files selected for processing (5)
changelog.d/7157-macos-child-process-posix-spawn.mdcrates/perry-runtime/src/child_process/exec.rscrates/perry-runtime/src/child_process/mod.rscrates/perry-runtime/src/child_process/options.rscrates/perry-runtime/src/child_process/registry.rs
| // Parse the env JSON up front so we can read its `PATH` for command | ||
| // resolution below (an env override with a `PATH` key is what pushes | ||
| // std onto the `fork`+`exec` fallback for a bare command name). | ||
| let env_map = { | ||
| let env_bits = env_json_val.to_bits(); | ||
| if env_bits != TAG_NULL_BITS && env_bits != TAG_UNDEFINED_BITS { | ||
| extract_string_from_nanboxed(env_json_val).and_then(|env_json| { | ||
| serde_json::from_str::<serde_json::Map<String, serde_json::Value>>(&env_json) | ||
| .ok() | ||
| }) | ||
| } else { | ||
| None | ||
| } | ||
| }; | ||
|
|
||
| // Resolve a bare command name to an absolute path so std uses | ||
| // `posix_spawn` instead of the `fork`+`exec` fallback that an env | ||
| // override triggers — the macOS fork/dyld deadlock fix (see | ||
| // `options::cp_command_for_program`). `arg0` preserves argv[0]. | ||
| let mut command = { | ||
| #[cfg(unix)] | ||
| { | ||
| let resolved = if cmd_str.contains('/') { | ||
| None | ||
| } else { | ||
| let path = env_map | ||
| .as_ref() | ||
| .and_then(|m| m.get("PATH")) | ||
| .and_then(|v| v.as_str()) | ||
| .map(str::to_string) | ||
| .or_else(|| std::env::var("PATH").ok()) | ||
| .unwrap_or_default(); | ||
| super::cp_resolve_program_path(&cmd_str, &path) | ||
| }; | ||
| match resolved { | ||
| Some(abs) => { | ||
| use std::os::unix::process::CommandExt; | ||
| let mut c = Command::new(abs); | ||
| c.arg0(&cmd_str); | ||
| c | ||
| } | ||
| None => Command::new(&cmd_str), | ||
| } | ||
| } | ||
| #[cfg(not(unix))] | ||
| { | ||
| Command::new(&cmd_str) | ||
| } | ||
| }; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## locate relevant files"
fd -a 'options\.rs|registry\.rs|mod\.rs' crates/perry-runtime/src/child_process 2>/dev/null || true
echo
echo "## registry.rs outline and relevant lines"
for f in $(fd 'registry\.rs' crates/perry-runtime/src/child_process); do
echo "--- $f"
wc -l "$f"
sed -n '1,190p' "$f"
done
echo
echo "## options.rs relevant symbols"
for f in $(fd 'options\.rs' crates/perry-runtime/src/child_process); do
echo "--- $f"
wc -l "$f"
rg -n "CP_DEFAULT_PATH|cp_effective_path|cp_resolve_program_path|PATH|env" "$f" -C 4
done
echo
echo "## child_process mod exports"
for f in $(fd 'mod\.rs' crates/perry-runtime/src/child_process); do
echo "--- $f"
wc -l "$f"
sed -n '1,220p' "$f"
done
echo
echo "## tests/usages for cp_effective_path and resolve"
rg -n "cp_effective_path|CP_DEFAULT_PATH|cp_resolve_program_path|contains\('/'\)" crates/perry-runtime/src crates/perry-tests || trueRepository: PerryTS/perry
Length of output: 25634
🌐 Web query:
Rust std::process::Command posix_spawn fork exec PATH env_clear execvp documentation
💡 Result:
On Unix-like platforms, Rust's std::process::Command uses a combination of posix_spawn and the traditional fork/exec pattern to launch child processes [1][2][3]. 1. Strategy Selection: Rust preferentially uses posix_spawn for process creation because it can be more efficient and avoids some complexities associated with memory management after a fork (e.g., thread synchronization) [1][2][4]. However, it falls back to the fork/exec pattern if the configuration requires operations not easily supported by posix_spawn, such as pre-exec closures (via pre_exec), setting specific UIDs/GIDs, or when the environment's PATH has been modified (depending on the target) [3][5][6]. 2. PATH and Environment: If a program is not specified as an absolute path, the system must search for the executable. When using the traditional fork/exec path, Rust uses execvp, which automatically searches the PATH environment variable [7][8]. If env_clear or env_remove is used to remove the PATH variable without providing a replacement, execvp will typically fall back to an OS-defined default (often /bin:/usr/bin), rather than defaulting to the parent process's PATH [7][8]. 3. posix_spawn and Environment: posix_spawn generally does not perform PATH resolution in the same way as execvp (unless posix_spawnp is specifically used, which has inconsistent platform availability and behavior) [4]. Consequently, if Rust detects that the environment's PATH has been altered or that complex path-related logic is needed, it may disqualify the posix_spawn path in favor of fork/exec to ensure consistent, predictable executable resolution [3][4][5]. 4. env_clear: This method prevents the child process from inheriting any environment variables from the parent [7][9]. While this effectively clears the environment, users should be aware that if the PATH is not explicitly re-added, the system's ability to locate executables will rely on the default behavior of the underlying exec function (e.g., execvp's fallback to system defaults) rather than the parent's original environment [7][8].
Citations:
- 1: https://doc.rust-lang.org/stable/src/std/sys/process/unix/unix.rs.html
- 2: https://github.com/rust-lang/rust/blob/4a9d5368/library/std/src/sys/process/unix/unix.rs
- 3: https://github.com/rust-lang/rust/blob/ce9954c0/library/std/src/sys/process/unix/unix.rs
- 4: Command: Support posix_spawn() on FreeBSD/OSX/GNU Linux rust-lang/rust#48624
- 5: https://github.com/rust-lang/rust/blob/1d05e3c131d7181eb3df1a8c261f43135c99200d/library/std/src/sys/process/unix/unix.rs
- 6: https://github.com/rust-lang/rust/blob/d2f24127/library/std/src/sys/process/unix/common/tests.rs
- 7: https://doc.rust-lang.org/stable/std/process/struct.Command.html
- 8: https://github.com/rust-lang/rust/blob/master/library/std/src/process.rs
- 9: https://doc.rust-lang.org/nightly/std/process/struct.Command.html
🌐 Web query:
POSIX execvp empty PATH element current directory semantics PATH missing
💡 Result:
Under the POSIX standard, if the PATH environment variable is not present, the behavior of execvp regarding the search path is explicitly implementation-defined [1][2][3]. Consequently, there is no single "POSIX standard" behavior for what happens when PATH is missing; different systems handle this differently [4][5]. Regarding the interpretation of PATH elements: 1. Empty PATH Elements: POSIX specifies that a zero-length (empty) prefix in the PATH variable shall be treated as the current working directory [6]. This is commonly represented by a leading colon, a trailing colon, or two adjacent colons in the PATH string (e.g., PATH=:/bin or PATH=/bin::/usr/bin) [6]. 2. Missing PATH Variable: On many Linux systems (specifically those using glibc), if the PATH variable is absent, the system defaults to a search path that typically includes the current working directory followed by the directories defined by confstr(_CS_PATH) [4][7][6]. Historically, this often placed the current directory at the beginning of the search path [4][5]. However, this behavior can vary; for instance, some modern implementations have opted to drop the current directory from this default search path to mitigate security risks associated with potential Trojan horses [4][7]. In summary, while POSIX mandates that empty elements in a present PATH variable denote the current directory, it leaves the default behavior entirely to the implementation when the PATH variable itself is missing [1][2][3]. Users should consult the specific documentation for their operating system or C library (e.g., glibc) to determine the exact search order employed [4][7].
Citations:
- 1: https://pubs.opengroup.org/onlinepubs/9699919799/functions/execvp.html
- 2: https://pubs.opengroup.org/onlinepubs/9699919799.2018edition/functions/execvp.html
- 3: https://pubs.opengroup.org/onlinepubs/9799919799.2024edition/functions/execv.html
- 4: https://man7.org/linux/man-pages/man3/execvp.3.html
- 5: https://manned.org/man/ubuntu-cosmic/execvp.3
- 6: https://github.com/zerovm/glibc/blob/master/posix/execvp.c
- 7: https://man.archlinux.org/man/execv.3.en
Align background-spawn PATH lookup with cp_effective_path.
cp_effective_path documents replacement-mode env lookup as child PATH or the execvp default only, with CP_DEFAULT_PATH as that default. This resolver falls back to the parent’s PATH when an env object has no "PATH", then uses an empty fallback when PATH is absent from both, which cp_resolve_program_path interprets as the current directory. Match the existing resolved Command path: use the child’s PATH when the env map is present, otherwise CP_DEFAULT_PATH.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/perry-runtime/src/child_process/registry.rs` around lines 88 - 136,
Update the PATH selection in the command-resolution block around
cp_resolve_program_path to match cp_effective_path: when env_map is present, use
its child PATH value if available without falling back to the parent
environment; when env_map is absent, use CP_DEFAULT_PATH. Remove the current
parent-PATH and empty-string fallbacks so missing PATH never resolves against
the current directory.
Single conflict, in the `options` re-export list in crates/perry-runtime/src/child_process/mod.rs: this branch adds `cp_command_for_program` while main's Node-26 child_process parity series added `cp_stdio_stream_fd`. Resolved as the union of both names. The rest of the child_process series (exec.rs, options.rs, registry.rs) merged cleanly; all of this branch's `cp_command_for_program` / `cp_resolve_program_path` call sites survive. main's new `spawn_detached_command` keeps `Command::new` deliberately — it installs a `pre_exec` setsid closure, which forces std's fork path regardless, and is the same `detached` carve-out this branch already documents.
2 participants
On macOS, a Perry program that starts a child process with
child_process(exec,spawn, orexecFile) can hang forever instead of running the command. Even something as trivial assh -c "echo hi"never gets as far as executingecho. The parent then waits on a child that will never finish, so the whole program stalls. This affects any macOS program that shells out, and it shows up under garbage-collection pressure because that is when the runtime has the most threads alive. Two real programs hit it: one reads a machine identifier at startup by shelling out throughnode-machine-id, and another spawnsgo,mvn, anddotnetfrom a tool-execution code path.The fix is to resolve a bare command name such as
shto its absolute path (/bin/sh) before handing it to the operating system. That one change keeps Rust's standard library on theposix_spawncode path instead of theforkcode path, andforkis what deadlocks. Linux is unaffected either way. The original command name is still what the child sees as its ownargv[0], and if nothing resolves, the bare name is passed through unchanged.What the hang looks like from the outside
The child process gets as far as
execand then stops inside dyld, the macOS dynamic linker, on this stack:blockOnSynchronousEventis dyld telling any attached observer that a new image is loading, and waiting for an acknowledgement that never arrives. With the child stuck there, Perry's reader and waiter threads block forever inread()andwait4()respectively, and the main loop sits idle injs_wait_for_event.Why the standard library falls back to fork
There are two ways to start a child process on Unix.
posix_spawnis a single operating-system call that creates the new process and loads the new program in one step. Theforkplusexecpair first clones the current process, then replaces the clone's program image. Rust'sstd::process::Commandprefersposix_spawnbut silently falls back toforkplusexecwhen it cannot express what was asked of it.The relevant condition lives in
library/std/src/sys/pal/unix/process/process_unix.rs, whereposix_spawnis chosen only when the program is given as a path and nopre_exec, uid, or gid closures are set. The specific guard isenv_saw_path() && !program_is_path(). Passing anenvoption triggersenv_clear(), which setsenv_saw_path(), so a bare command name combined with anenvoption lands on the fallback.forkis the unsafe half. Perry's runtime is multithreaded, with an async reactor plus garbage-collector and worker threads.forkonly copies the calling thread, but the child inherits all the locks and Mach state those other threads were holding, and those threads no longer exist to release anything. dyld's synchronous notification in the child then waits on state that can never be handed over, which is the deadlock.Syscall-level before and after
Confirmed by interposing on dyld with
DYLD_INSERT_LIBRARIESto record which spawn mechanism the standard library actually chose:exec('echo hi', { env })in a Promisefork()posix_spawnspawnSync('sh', …, { env })fork()posix_spawnexec('echo hi')(no opts)posix_spawnposix_spawnOutput capture, exit codes,
argv, and distinct child process identifiers are unchanged across all three.How the resolution works
Before building the
Command, a bare program name is resolved against the child's effectivePATH. That means thePATHfrom theenvoption when one is present, which matches what Node andexecvpdo, and the parent'sPATHotherwise. The resolved absolute path is what the standard library receives, so itsprogram_is_path()check passes and it stays onposix_spawneven when anenvorcwdoption is present.The original name is preserved as
argv[0]viaarg0, so a child that inspects its own name sees what the caller wrote. If nothing resolves, the bare name is passed through unchanged; a genuineENOENTnever executes a real program image, so it cannot reach the dyld hang.exec,execSync, andpromisify(exec)now use the absolute/bin/sh, which is the shell Node uses.spawn,spawnSync,execFile,execFileSync,promisify(execFile), andspawn_backgroundresolve their program throughcp_command_for_program.What still uses fork, and why
Three cases necessarily stay on the standard library's
forkpath, becauseposix_spawncannot express them through Rust's wrapper:detached(which needssetsid), the IPCdup2thatfork()performs, and setting a uid or gid. Each is documented inline at its call site. None of them is part of the reportedexec/spawnimpact.Verification runs
The dyld interposer shows the
env-carryingexecandspawnSynccases flipping fromfork()toposix_spawn, with output capture, exit codes,argv, and distinct child process identifiers unchanged.Recompiled reproductions run to completion under
PERRY_GC_FORCE_EVACUATE=1: theexec-in-a-Promise case passes 25 out of 25 runs and thespawnSynccase passes 15 out of 15.cargo test -p perry-runtime child_processreports 7 of 7 passing, including 4 new regression tests that assert the resolved program is an absolute path and that it spawns and captures output correctly.Linux behavior is unchanged, since the resolver only rewrites bare names to absolute paths,
posix_spawnis already the norm there, and theforkhazard is macOS-specific.One caveat on the evidence. The hard deadlock only reproduces when a dyld notification observer is attached, such as a debugger, a sampler, or a product's telemetry hook, and no such observer exists in a bare shell. This change is therefore verified at the syscall level, showing the switch from
forktoposix_spawn, plus functional and behavioral equivalence, rather than by reproducing the hang itself in a bare shell.Summary by CodeRabbit
Bug Fixes
PATH.argv[0]while resolving bare commands to reliable executable paths.PATHbehavior.Documentation