Repository navigation
Conversation
|
Updated 3:05 AM PT - Aug 1st, 2026
❌ @robobun, your commit 13a8c7c has 2 failures in
🧪 To try this PR locally: bunx bun-pr 31717That installs a local version of the PR into your bun-31717 --bun |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
WalkthroughAdds ENOEXEC handling in spawn_process_posix: on ENOEXEC retry spawn via /bin/sh with the original argv0 forwarded. Adds POSIX-only tests verifying execFileSync, spawnSync, execFile fallback to /bin/sh and PATH resolution before fallback. ChangesENOEXEC Fallback to Shell
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![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
🤖 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 `@test/js/node/child_process/child_process.test.ts`:
- Around line 402-406: Swap the last two assertions so the exit code check runs
last: after calling spawnSync(scriptPath, ["a", "b"], { encoding: "utf8", env:
bunEnv }) and asserting result.stderr and result.stdout, assert result.error is
undefined before asserting result.status equals 0; update the expectations that
reference result.error and result.status (from the current block using
spawnSync) so expect(result.error).toBeUndefined() precedes
expect(result.status).toBe(0).
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e97e94a5-901a-458f-a712-7b012a1256df
📒 Files selected for processing (2)
src/spawn_sys/spawn_process.rstest/js/node/child_process/child_process.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
CI status: the diff is green — the two remaining reds are main breaks, now reported to triage. Build 86902 (head 13a8c7c): 188 jobs passed, including every lane that runs this PR's tests (
The change is gated entirely behind |
There was a problem hiding this comment.
Both earlier issues (Android /system/bin/sh and the CStr scope on macOS) are now fixed and I found nothing further — but since this changes the core POSIX spawn path that every child_process/Bun.spawn/Shell call goes through, it's worth a human sign-off.
Extended reasoning...
Overview
The PR adds a libuv-style ENOEXEC fallback to spawn_process_posix in src/spawn_sys/spawn_process.rs: when posix_spawn returns ENOEXEC (executable file with no recognized format, typically a shebang-less script), it rebuilds argv as [/bin/sh, <resolved-path>, argv[1..]] and retries. A cfg-gated SHELL_PATH constant selects /system/bin/sh on Android. New tests cover execFileSync, spawnSync, async execFile, PATH resolution, and Bun Shell.
Two earlier rounds of my review surfaced (1) the Android shell-path issue and (2) a CStr-not-in-scope build break on macOS; both were fixed in ef88c47 and f3d1587. The current diff is clean on those fronts and the bug hunter found nothing further. The one CI failure (streams-leak.test.ts on x64 Linux) is unrelated to this change.
Security risks
The retry passes the resolved path and user args as separate argv entries to /bin/sh (i.e. sh <file> <arg1> <arg2>), not as a concatenated command string, so there is no new shell-injection surface — args become positional parameters, not parsed shell syntax. The fallback only fires after the kernel has already permission-checked the file and returned ENOEXEC, so no +x check is bypassed. This is the same behavior as execvp(3) and libuv's uv__process_child_init.
Level of scrutiny
High. spawn_process_posix is the single funnel for every process spawn on POSIX — child_process.*, Bun.spawn/Bun.spawnSync, and Bun Shell all go through it. The change is small (~40 lines) and the logic is straightforward, but it alters runtime behavior (previously-failing ENOEXEC now silently runs through the shell) and includes unsafe pointer iteration over the caller-supplied argv array plus platform cfg gates. That combination warrants a human reviewer's sign-off rather than bot-only approval.
Other factors
- Implementation matches the libuv reference and POSIX
execvpsemantics;$0correctly receives the resolved path (verified by the PATH-resolution test). - Test coverage is good across sync/async/PATH/Shell entry points, all gated by
isPosix. - No CODEOWNERS rule covers
src/spawn_sys/. - The
sh_argvVec lifetime is sound: it outlives thespawn_zcall within the match arm, and the borrowed C strings (SHELL_PATH,argv0_cstr,argv[i]) all outlive it.
a92ecda to
fe0eca2
Compare
241a828 to
3200c2e
Compare
There was a problem hiding this comment.
No issues found on this revision — both prior review comments (Android shell path, macOS CStr scope) are addressed. Deferring only because this changes core spawn semantics (previously-failing ENOEXEC execs now run through /bin/sh), which is worth a maintainer's explicit sign-off.
Extended reasoning...
Overview
This PR adds an ENOEXEC → /bin/sh retry to spawn_process_posix in src/spawn_sys/spawn_process.rs (~40 lines), plus five new tests across test/js/node/child_process/child_process.test.ts (execFileSync, spawnSync, async execFile, PATH-resolved) and test/js/bun/shell/bunshell.test.ts. The change is entirely additive on the Err(err) if err.get_errno() == ENOEXEC arm; the successful-spawn path and all other error paths are byte-for-byte unchanged. It mirrors libuv's uv__process_child_init ENOEXEC handling, so child_process behavior converges with Node.
Security risks
The behavioral change is that an executable file the kernel rejects with ENOEXEC (e.g. a script with no shebang) is now handed to /bin/sh instead of surfacing the error. This is standard execvp(3) / libuv semantics and requires the file to already be +x and reachable, so there's no new attack surface relative to Node or a POSIX shell — but it does mean a file that previously failed to run now runs. I don't see a concrete exploit here (Bun already lets callers spawn arbitrary paths; this only changes what happens when the kernel can't parse the format), but "previously-rejected input now executes" is exactly the class of change I'd want a maintainer to consciously accept rather than rubber-stamp.
Level of scrutiny
spawn_process_posix is the single funnel for Bun.spawn, Bun.spawnSync, node:child_process, and Bun Shell — a critical, security-adjacent path. That alone puts it above my auto-approve bar. The change itself is small, well-commented, cfg-gated correctly for Android after the follow-up, and the argv reconstruction (SHELL_PATH, resolved argv0, argv[1..], NULL) matches libuv's layout. The actions/attr are reused across both spawn attempts, which is fine since posix_spawn doesn't consume them.
Other factors
Both of my earlier inline findings on this PR were fixed (Android /system/bin/sh via cfg-gated SHELL_PATH; macOS build via fully-qualified &core::ffi::CStr), and the current bug-hunt pass found nothing. Test coverage is solid: it asserts $0 is the resolved path (including via PATH lookup) and $* preserves user args, and covers sync/async/shell entry points. CI on the PR's own tests is green per the author's build-62059 summary; remaining failures are unrelated flake in untouched files. The author has already flagged that this needs a maintainer to merge.
3200c2e to
d436e4b
Compare
There was a problem hiding this comment.
LGTM — the ENOEXEC → /bin/sh retry is a direct port of libuv's uv__execvpe fallback, gated entirely behind an error arm that was previously a hard failure, so the success path is unchanged.
What was reviewed:
- Retry reuses
actions/attrby shared borrow and thePosixSpawnFdGuardstays armed across it, so fd cleanup is correct on both retry-success and retry-failure. sh_argvlifetime: the Vec outlives thespawn_zcall; every pointer it holds (SHELL_PATH, argv0_cstr, argv[1..]) is valid for the caller-contract duration.- Both prior findings (Android
/system/bin/sh, macOSCStrscope) are fixed in the current diff;use_execve_on_macos+ SETEXEC composes correctly with the retry. - Tests cover sync/async child_process, PATH-resolved
$0, Bun Shell, andbun run <bin>; all imports (chmodSync/join/isPosix/tempDir) are already in scope in each test file.
Extended reasoning...
Overview
The PR adds a single new match arm in spawn_process_posix (src/spawn_sys/spawn_process.rs): when the initial posix_spawn::spawn_z returns ENOEXEC, rebuild argv as [SHELL_PATH, resolved_path, user_args...] and retry once. SHELL_PATH is cfg-gated to /system/bin/sh on Android and /bin/sh elsewhere. The success path and every other error path are byte-for-byte unchanged — the original one-line spawn_z call is now wrapped in a match whose other => other arm preserves prior behavior. Three test files gain POSIX-gated coverage for execFileSync/spawnSync/execFile/PATH-resolved $0, Bun Shell, and bun run <bin>.
Security risks
None identified. The retry only fires on ENOEXEC from a file the caller already asked to execute; routing it through /bin/sh is the documented POSIX/libuv/Node behavior, not a new capability. No user-controlled string is interpolated into a shell command line — the resolved path and args are passed as discrete argv entries, so there is no injection surface.
Level of scrutiny
Medium. spawn_process_posix is on every subprocess path, but the diff is confined to a branch that previously surfaced a hard error to the user, so the worst regression is "still fails, differently." Memory-safety review focused on: (a) sh_argv is a stack Vec whose backing storage outlives the spawn_z call, and every *const c_char it holds points into memory the caller contract already guarantees for the whole function; (b) actions/attr are taken by & in spawn_z, so reusing them is sound; (c) PosixSpawnFdGuard.on_error remains true through the retry, so parent-side socketpair ends are closed if the retry also fails, and disarmed only after a successful pid is obtained.
Other factors
Two earlier review rounds from me flagged the Android shell path and the macOS CStr import gate; both are fixed in the current diff (cfg-gated SHELL_PATH with fully-qualified &core::ffi::CStr) and the threads are resolved. CI build #71234 compiles and passes this PR's tests on every lane; the three remaining failures (sql-prepare-false, issue-26030, worker-message-port ASAN) are in unrelated subsystems and do not reproduce across builds. Test coverage hits every entry point named in the PR description, asserts exact $0/$* values (not just exit codes), and I confirmed the new test files already import chmodSync/join/isPosix/tempDir so no missing-import breakage on the test side.
|
Status: rebased on main (028f7a3), conflicts resolved, added #5386 coverage. The ENOEXEC →
All new tests fail on the current release with ENOEXEC and pass with the fix. |
There was a problem hiding this comment.
No issues found in this pass, but this is a behavior change in the core POSIX spawn path (spawn_process_posix) that every subprocess on Unix funnels through, so it's worth a maintainer sign-off.
What was reviewed:
- argv construction on retry matches libuv (
sh <resolved-path> argv[1..], custom argv0 dropped, args passed verbatim — locked in by the[$HOME][a b][*]test). - fd cleanup:
PosixSpawnFdGuardstays armed across the retry; the failed first-attempt child is reaped insideposix_spawn_bun(waitpid/wait4) so no zombie. - macOS
POSIX_SPAWN_SETEXECretry re-applies the sameactions/attr; for thebun run --silentcase (Inherit stdio + chdir only) these are idempotent, and the--silenttest covers it. - The earlier Android
/system/bin/shand macOSCStrscope issues are both resolved in the current diff.
Extended reasoning...
Overview
The PR adds an ENOEXEC fallback in src/spawn_sys/spawn_process.rs::spawn_process_posix: when the initial posix_spawn::spawn_z returns ENOEXEC, it rebuilds argv as [SHELL_PATH, <resolved-path>, argv[1], …, NULL] and retries. SHELL_PATH is cfg-gated to /system/bin/sh on Android and /bin/sh elsewhere. The happy path is unchanged (other => other). Test coverage spans node:child_process (execFileSync/spawnSync/execFile, PATH-resolved, cwd-relative, verbatim-args), Bun Shell, and bun run <bin> including the --silent (bunx / SETEXEC) variant.
Security risks
Executing an ENOEXEC'd file through /bin/sh is a semantic change, but it matches Node/libuv exactly and only applies to files the caller already asked to execute (executable bit set, path already resolved). Args are passed as separate argv entries — not through sh -c — so no shell-injection surface is introduced; the verbatim-args test asserts $HOME, *, and quotes reach the script unexpanded. I don't see a new security exposure beyond what Node already has.
Level of scrutiny
High. spawn_process_posix is the single funnel for every POSIX subprocess spawn (Bun.spawn, node:child_process, Bun Shell, bun run, bunx). The change is well-gated behind errno == ENOEXEC, but it interacts with several platform-specific mechanisms: Bun's vfork-based posix_spawn_bun on Linux (verified the failed child is reaped before retry), Darwin's system posix_spawn with POSIX_SPAWN_CLOEXEC_DEFAULT, and the POSIX_SPAWN_SETEXEC passthrough used by bun run --silent. On the SETEXEC path, file actions are applied to the current process before the failed exec and then re-applied on retry; for the only in-tree caller (run_command.rs, all-Inherit stdio + chdir) they're idempotent, and the panic guard already forbids Buffer stdio with SETEXEC, but a maintainer should confirm that's the intended contract going forward.
Other factors
- Prior review feedback (CodeRabbit exit-code assertion order; my Android
/bin/shand macOSCStr-scope findings) is fully addressed and marked resolved. PosixSpawnFdGuard.on_errorremainstruethrough the retry, so socketpair fds are correctly closed if the retry also fails and correctly kept if it succeeds;to_close_at_end(child-side ends) are still open in the parent for the retry's dup2 actions.sh_argvis a localVec<*const c_char>holding pointers into the staticSHELL_PATH, the caller-ownedargv0_cstr, andargv[1..]— all valid for the synchronousspawn_zcall, then dropped.- CI on the last build is green for this PR's own tests; the reported failures are in unrelated files (sql, worker-message-port, issue/26030) per the robobun summary.
Given the criticality of the spawn path and the SETEXEC-replay subtlety, I'm deferring rather than approving.
An executable file with no recognized format (a script with no shebang) makes execve()/posix_spawn() return ENOEXEC. Node, via libuv's uv__execvpe, transparently retries the exec through /bin/sh so the file runs as a shell script. Bun's spawn_process_posix only ported libuv's PATH-resolution half; the ENOEXEC -> /bin/sh retry was missing, so the raw error surfaced. All spawn entry points funnel through spawn_process_posix, so this covers: - node:child_process (execFile/execFileSync/spawn/spawnSync) - #31710 - Bun Shell ($`./script.sh`) - #11563 - bun run <bin> / bunx - #5386 The third case is the user-visible one: a package whose bin is a shell script without a shebang works with npx but fails in bun run / bunx with 'Exec format error'. Fixes #31710 Fixes #11563 Fixes #5386
…args for ENOEXEC retry
49a1c03 to
52fa9e8
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Rebased on main (65c47c8); conflicts in |
52c8ab8 to
13a8c7c
Compare
There was a problem hiding this comment.
LGTM — the two issues I flagged earlier (Android /system/bin/sh and the CStr import gate on macOS) are both resolved, and the comment-cop nits have been trimmed.
What was reviewed:
- ENOEXEC retry in
spawn_process_posix: reusesactions/attrsochdirand fd setup replay on the second spawn;PosixSpawnFdGuardstill hason_error = trueif the retry also fails, so parent-side fds are cleaned up. sh_argvlifetime: the Vec outlives thespawn_zcall; pointers intoSHELL_PATH(static),argv0_cstr, and the caller'sargv[1..]are all valid for the call.- No shell injection: retry is
sh <file> <args...>with each arg as its own argv entry, notsh -c; theexecFileSynctest locks this in with$HOME,*, spaces, and quotes. - The two CI failures (worker-terminate on x64-asan, bun-upgrade on Windows aarch64) are unrelated — this change is POSIX-only and gated behind
errno == ENOEXEC.
Extended reasoning...
Overview
Adds the missing ENOEXEC → /bin/sh retry to spawn_process_posix in src/spawn_sys/spawn_process.rs (~35 lines), matching libuv's uv__execvpe. When posix_spawn returns ENOEXEC (executable file with no recognized format, typically a script without a shebang), the spawn is retried as sh <resolved-path> argv[1..]. Adds ~130 lines of tests across three entry points: node:child_process (execFileSync/spawnSync/execFile, PATH resolution, relative-path+cwd, verbatim args), Bun Shell, and bun run <bin> (both default and --silent, the latter exercising the macOS POSIX_SPAWN_SETEXEC path).
Security risks
None. The retry passes the file path and each user arg as separate argv entries to sh, not through sh -c, so there is no shell expansion of user-supplied strings — the execFileSync test asserts $HOME, *, embedded spaces, and quotes reach the script un-expanded. The Android shell path is cfg-gated to /system/bin/sh per the existing repo convention.
Level of scrutiny
Medium-high — this touches the core POSIX spawn path shared by Bun.spawn, node:child_process, Bun Shell, and bun run/bunx. However, the change is narrowly gated behind err.get_errno() == ENOEXEC, so all existing spawn behavior is unchanged. The retry reuses the already-built actions and attr (posix_spawn objects are designed to be reusable; the failed first attempt ran file actions only in the now-dead child, leaving parent fds intact), and the existing PosixSpawnFdGuard covers cleanup if the retry also fails.
Other factors
- This PR has been through multiple review rounds. My two prior findings (hardcoded
/bin/shbreaking Android API 28; the follow-up'sCStrreference breaking the macOS build) were both addressed — the current code uses a cfg-gatedSHELL_PATHconstant with fully-qualified&core::ffi::CStr. The comment-cop bot's length complaints were resolved by trimming. - Test coverage hits the variant matrix per REVIEW.md: sync + async, absolute + relative + PATH-resolved, with and without cwd, both
bun runcode paths, and Bun Shell. All new tests areisPosix-gated (Windows is unaffected — its spawn path is separate). - The two remaining CI failures (
test-worker-message-port-transfer-terminate.jsSIGABRT on x64-asan;bun-upgrade.test.tson Windows aarch64) are unrelated to this change and appear elsewhere on main. bunshell.test.tsalready importschmodSync,join,tempDir, andisPosix, so the appended test needs no import changes.
This branch has not been deployed
2 participants
Fixes #31710
Fixes #11563
Fixes #5386
Repro
A package whose
binis a shell script without a shebang:The same applies to
node:child_process:and Bun Shell (
$`./script.sh`→bun: Exec format error).Cause
An executable file with no recognized format (e.g. a script with no shebang) makes
execve()returnENOEXEC. Node, via libuv'suv__execvpe, transparently retries the exec through/bin/shso the file runs as a shell script. Bun's posix spawn path (spawn_process_posix) only ported libuv's PATH-resolution half ofexecvpe; theENOEXEC→/bin/shretry half was missing, so the raw error surfaced.Fix
In
src/spawn_sys/spawn_process.rs, when the initialposix_spawnreturnsENOEXEC, re-exec/bin/shwith the originally-resolved path asargv[1](becomes$0inside the script) followed by the user-supplied args. This matches libuv/Node exactly:$0is the resolved path (including when the command is resolved fromPATH),$1,$2… are the user args, in order,ENOEXECtriggers the fallback (no pre-sniffing).All spawn entry points funnel through
spawn_process_posix, so this covers the sync path (execFileSync/spawnSync/Bun.spawnSync), the async path (execFile/spawn/Bun.spawn), Bun Shell (#11563), andbun run <bin>/ bunx (#5386).Verification
New tests cover all three entry points:
test/js/node/child_process/child_process.test.ts:execFileSync,spawnSync,execFile(async), and the PATH-resolved case.test/js/bun/shell/bunshell.test.ts: Bun Shell (bun: Exec format error while executing sh file #11563).test/cli/run/run_command.test.ts:bun run <bin>with a no-shebang bin innode_modules/.bin(bunx fails to run scripts (due to error InvalidExe). When npx works perfectly #5386).All fail on the current release with
ENOEXECand pass with the fix.The
grafbasecase in #5386 is slightly different: itsbinfile is a placeholder thatpostinstallreplaces with the real binary. When the postinstall runs, the bin is a valid native binary and works; when it doesn't, the placeholder is a plain text file and this fix makesbun runbehave exactly like npx (route through/bin/sh, which then prints a parse error). Whether bunx runs the postinstall is tracked separately by #19637.