fix(spawnSync): throw instead of segfaulting when event-loop creation fails - #35033
fix(spawnSync): throw instead of segfaulting when event-loop creation fails#35033robobun wants to merge 4 commits into
Conversation
WalkthroughChangesSpawnSync loop failure handling
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 11:01 PM PT - Jul 21st, 2026
✅ @robobun, your commit 27677661a16d810154ebe0bf15f8414fc6ede754 passed in 🧪 To try this PR locally: bunx bun-pr 35033That installs a local version of the PR into your bun-35033 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
… use absolute argv0 in test - macOS us_internal_create_async: free cb + machport_buf and drop the receive right on mach_port_* failure, now that the NULL return is survivable - SpawnSyncEventLoop::uws_loop doc: the non-null guarantee is via init()'s early-false return, not an assert - test: absolute /bin/sh skips which_for_spawn, which fails with ENOENT under EMFILE on musl before the event loop is created
![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 `@packages/bun-usockets/src/eventing/libuv.c`:
- Around line 294-295: Check the return value of us_internal_loop_data_init in
the loop initialization path and propagate its -1 failure result immediately.
Preserve the function’s existing cleanup behavior and prevent returning a loop
with loop->data.wakeup_async unset.
🪄 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: 8688de43-f1f6-4613-87fe-b4684a7d9648
📒 Files selected for processing (10)
packages/bun-usockets/src/eventing/epoll_kqueue.cpackages/bun-usockets/src/eventing/libuv.cpackages/bun-usockets/src/internal/internal.hpackages/bun-usockets/src/loop.cpackages/bun-uws/src/Loop.hsrc/event_loop/SpawnSyncEventLoop.rssrc/jsc/rare_data.rssrc/runtime/api/bun/js_bun_spawn_bindings.rssrc/uws_sys/Loop.rstest/js/bun/spawn/spawn-sync-loop-init-fail.test.ts
There was a problem hiding this comment.
Beyond the inline nit, I checked the scopeguard ordering around the new early-return in spawn_maybe_sync: the cleanup defer! (with its .expect("cached by the IS_SYNC prepare above")) is armed after the fallible spawn_sync_event_loop() call, so the expect is sound; and the earlier defer_guard (abort_signal unref) and memfd_guard are already armed at that point, so they still fire on the new error path. spawn_options is constructed after this block, so nothing to deinit() there.
Extended reasoning...
The new early return at the loop-init failure sits between two sets of scopeguards. I traced the ordering: defer_guard (abort_signal/terminal_info cleanup) and memfd_guard are armed before it and correctly run; the sync-loop cleanup defer and spawn_options are both after it and correctly do not. The two .expect("cached by the IS_SYNC prepare above") sites are only reachable after prepare() succeeded and cached the loop in rare_data.
…nit error The real errno/syscall is not propagated through us_create_loop's NULL return; name the platform's event-provider syscall rather than a Windows-only one on POSIX. Also document why us_internal_loop_data_init cannot fail on the libuv backend.
…es on Windows early return
There was a problem hiding this comment.
All five earlier findings have been addressed and I don't see anything further wrong on this revision, but this touches core event-loop initialization across three C backends (epoll/kqueue/libuv), changes the us_internal_loop_data_init signature, and makes uWS::Loop::get() newly nullable with callers deferred to #33850 — worth a human look before landing.
What was reviewed:
- Cleanup on each new failure path (epoll fd, kqueue fd, eventfd poll, mach_port receive+send rights, libuv
uv_loop,recv_buf/send_buf,num_polls) — all paired. rare_data::spawn_sync_event_loopnot caching on failure, so the two later.expect("cached by the IS_SYNC prepare above")sites are sound.- The Windows
extra_fds.deinit()on the new early return, and thatstdio[0..2]are still user-facingStdioat that point. - The libuv
(void)us_internal_loop_data_initcast is dead-path on that backend (itsus_internal_create_asyncacquires no OS resource).
Extended reasoning...
Overview
The PR converts a segfault/panic in Bun.spawnSync's isolated-event-loop creation into a catchable JS error. It threads a NULL/-1 failure return through us_create_loop (all three backends: epoll, kqueue, libuv), us_internal_loop_data_init, uWS::Loop::create/get, PosixLoop/WindowsLoop::create, SpawnSyncEventLoop::init, and RareData::spawn_sync_event_loop, terminating in a thrown SystemError in spawn_maybe_sync. Ten files, ~220 lines net.
Security risks
None identified. The change is failure-path plumbing under resource exhaustion; no new user-controlled input reaches native code, and the error path only throws.
Level of scrutiny
High. us_create_loop and us_internal_loop_data_init are the core event-loop bootstrap for every uSockets consumer in the process (HTTP client thread, workers, spawnSync). The macOS us_internal_create_async mach_port block was substantially restructured (three linear early-returns → nested success ladder + shared cleanup tail), and mach_port failure paths are essentially untestable. uWS::Loop::get() now returns nullptr under exhaustion; the PR explicitly leaves those callers to a separate PR (#33850, noted as conflicting with main). The Windows failure case (uv_loop_new() NULL) is not exercised by the new test — only the POSIX eventfd/epoll path is.
Other factors
Five prior findings from earlier passes (macOS cb/machport_buf leak, stale SAFETY comment, misleading syscall string, Windows extra_fds pipe leak, mach send-right dead-name leak) were each addressed in follow-up commits and verified against the final diff. The test is hermetic (ulimit + local /bin/sh), asserts a positive retry after freeing fds, and the evidence block shows it timing out on the pre-fix ASAN build. Given the cross-backend C changes, the acknowledged overlap with an in-flight conflicting PR, and the untested Windows/macOS failure branches, a human reviewer should sign off.
|
Heads up: #39641 touches the same three sites in |
2 participants
Crash
Windows only.
Bun.spawnSyncsegfaults the whole process whenuv_loop_new()fails. Crash chain:Cause
packages/bun-usockets/src/eventing/libuv.c::us_create_loop:uv_loop_new()returns NULL whenuv_loop_initfails, which happens whenCreateIoCompletionPortreturnsERROR_INSUFFICIENT_RESOURCESunder handle or non-paged-pool exhaustion. The segfault happens insideus_create_loopso the null-assert inWindowsLoop::createnever fires.Bun.spawnSync(SpawnSyncEventLoop::init->WindowsLoop::create) is the onlyhint == NULLcaller on Windows and constructs a fresh loop per VM, so aspawnSyncunder resource pressure takes down the process instead of failing that one call.The POSIX backend has the same shape:
epoll_create1/kqueuereturning -1 was never checked, and on Linux the wakeupeventfd()failure aborted withBUN_PANIC.Fix
us_create_loopnow returns NULL when the backing event provider cannot be created, cleaning up whatever it allocated:uv_loop_new()epoll_create1/kqueue, and propagate the wakeup-async failure (eventfd/mach_port) through a now-fallibleus_internal_loop_data_inituWS::Loop::create/getreturnnullptrinstead of dereferencing NULL.Rust side:
PosixLoop::create/WindowsLoop::createreturnOption<NonNull<Loop>>instead of assertingSpawnSyncEventLoop::initreturnsfalseon failure (storage left uninitialized)RareData::spawn_sync_event_loopreturnsOption<&mut SpawnSyncEventLoop>; not cached on failure so a later call retriesspawn_maybe_syncthrows aSystemError(code: "EMFILE",syscall: "uv_loop_init") at the one spawnSync callThe
uWS::Loop::get()callers (HTTP client thread, Worker VM init) are not changed here; #33850 handles those. The shared usockets changes overlap with that PR, which is currently conflicting with main.Verification
test/js/bun/spawn/spawn-sync-loop-init-fail.test.ts(POSIX-only) runs a child underulimit -n 512, fills the fd table, and callsBun.spawnSync. On main the child aborts withpanic: eventfd() failed during loop init(exit 134); with this change the spawnSync throws a catchable EMFILE error, and a retry after freeing descriptors succeeds.The Windows path (
uv_loop_new()NULL) routes through the sameWindowsLoop::create->SpawnSyncEventLoop::init->spawn_maybe_syncpropagation. Built and ran the child_process spawnSync suite on Windows x64.[review] gate passed · iteration 1 · 10 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 4 passed · 0 rejected · iteration 1
evidence per changed file