Repository navigation
Conversation
|
Status: ready for review. It cannot merge before oven-sh/WebKit#741: the WebKit pin is the preview build of that PR, and I swap it to the merged SHA when that PR merges. Stacked on #40169. Reproduced on Linux x64, each exit 139 with an empty stderr. A library loaded with
With this branch: Tests: |
There was a problem hiding this comment.
Beyond the inline findings, I also checked the signal_stack.rs refactor for behavior drift: the new ss_sp = base + (len - ALT_STACK_SIZE) is the same address as the old base + page, and the reuse path only drops (SS_DISABLE + munmap) a mapping whose re-registration failed. Moving the call_once ahead of the null check in Bun::tryCreateVM is harmless since both null-checking callers panic before using the VM.
Extended reasoning...
The change routes all three JSC::VM creation sites through a new Bun::tryCreateVM that re-arms SA_ONSTACK once per process, refactors the alt-signal-stack mapping in src/bun_core/signal_stack.rs into Mapping::new/stack with thread-local reuse, and adds a source lint test. It touches crash-signal handling but no auth, injection, or data-exposure surface. Inline findings already flag the missing runtime test and the pre-existing null dereference in vmForBytecodeCache, so a human look is warranted.
18a3db3 to
98da454
Compare
There was a problem hiding this comment.
The two commits since my last review address both earlier points (the bytecode-cache VM now goes through a helper that panics on allocation failure, and the SA_ONSTACK behaviour has runtime coverage); I found no bugs in this version, but because it changes process-wide signal-handler state and unsafe mmap/sigaltstack code, and adds a compile-at-test-time LD_PRELOAD shim to the suite, a human look is still worthwhile.
What was reviewed:
Bun::createVMand its three call sites:RefreplacesRefPtrwith the samerefSuppressingSaferCPPCheckingbalance, thestd::once_flagis process-wide so a non-main-thread first VM still re-arms the flag, andZigGlobalObject.his already included by both other files.signal_stack.rsrefactor:ss_sparithmetic is equivalent to the oldbase + page; the take-then-reregister path only runs when the query above reports SS_DISABLE, so the extraSS_DISABLEinDropon the failure path is a no-op rather than a regression.NodeCompileCache.rs:configure_threadinstalls the alt stack before theStackCheckcall that the test'spthread_getattr_nphook overflows in, so the ordering the test depends on holds.- The unread-stdout pipe in the new
bun build --bytecodetest matches the two existing sibling tests in the same describe block; the build summary is far below the pipe buffer.
Extended reasoning...
The diff adds a Ref-returning Bun::createVM helper in src/jsc/bindings/ZigGlobalObject.cpp that wraps JSC::VM::tryCreate, panics on null, and runs CrashHandler__keepSignalHandlersOnAltStack once per process, and routes Zig__GlobalObject__create, vmForBytecodeCache (ZigSourceProvider.cpp) and BakeCreateProdGlobal (BakeGlobalObject.cpp) through it; it refactors src/bun_core/signal_stack.rs into Mapping::new/stack so a thread whose alt stack was externally disabled reuses its mapping, and calls configure_thread on the BunCompileCache thread. It touches no auth, crypto, or input-parsing surface, and no CODEOWNERS path. Two new subprocess tests in test/cli/run/run-crash-handler.test.ts compile a C LD_PRELOAD shim (gated on isLinux and a cc binary) to force a native stack overflow after the bytecode VM exists and on the compile-cache thread, and a new source lint asserts JSC::VM::tryCreate appears only in the helper. The production change is about 40 lines and I could verify the refcount and signal-state behaviour by reading, but process-wide sigaction state, unsafe mmap/sigaltstack code, and a compiled-shim testing approach are the kind of thing a maintainer should weigh, so defer rather than approve.
98da454 to
725599f
Compare
|
@robobun wake up!! |
|
@robobun wake up!! |
|
I am on it. The state of this PR:
If you prefer the first shape (the bun-side hook in |
…d, delete the hook oven-sh/WebKit#741 sets SA_ONSTACK where WTF::SignalHandlers::finalize() installs the SIGSEGV/SIGBUS handler. CrashHandler__keepSignalHandlersOnAltStack put the flag back after the first VM that Zig__GlobalObject__create made, so a process whose first VM comes from another place (bun build --bytecode) kept a handler without the flag. run-crash-handler.test.ts: a preloaded library overflows the native stack at the exit of `bun build --bytecode`.
The worker that generates bytecode for the compile cache runs JSC and never called Output::Source::configure_thread(), so a native stack overflow on it ended the process with no report. run-crash-handler.test.ts: a preloaded library overflows the native stack of the thread named BunCompileCache.
725599f to
826f224
Compare
| * From https://github.com/oven-sh/WebKit releases. | ||
| */ | ||
| export const WEBKIT_VERSION = "f20ce7744553c910bcf16a33faf976af208de091"; | ||
| export const WEBKIT_VERSION = "autobuild-preview-pr-741-13e08838"; |
There was a problem hiding this comment.
🔴 Every build of bun stops working the moment oven-sh/WebKit PR 741 merges or closes, because the prebuilt WebKit download 404s. scripts/build/deps/webkit.ts:6 pins WEBKIT_VERSION to the preview tag "autobuild-preview-pr-741-13e08838", and the repo's own download.ts:319-337 says GitHub deletes those preview releases when the PR merges or closes. The base pins a merged 40-hex sha that stays published. Fix: pin the merged oven-sh/WebKit main sha that contains the SA_ONSTACK change and confirm the prebuilt tarball exists for every platform and flavor, since src/crash_handler/lib.rs no longer re-arms SA_ONSTACK itself and depends on that WebKit commit being in the pinned build.
Why this was flagged
Any bun bd, bun run build, or CI build in prebuilt WebKit mode (the default) computes the tarball URL from WEBKIT_VERSION in prebuiltUrl at scripts/build/deps/webkit.ts:71-73, which for a value starting with "autobuild-" uses it verbatim as the release tag. The new value at scripts/build/deps/webkit.ts:6 is "autobuild-preview-pr-741-13e08838", an unmerged-PR preview build. The comment above prebuiltDownloadError at scripts/build/download.ts:319-322 states that GitHub deletes the preview release when the PR merges or closes, so every build 404s at once and fails with "WebKit preview release is gone". On the base branch the pin is the merged sha f20ce7744553c910bcf16a33faf976af208de091, whose release persists. This is not a transient state: the same commit deletes CrashHandler__keepSignalHandlersOnAltStack from src/crash_handler/lib.rs and its call in src/jsc/bindings/ZigGlobalObject.cpp, so the SA_ONSTACK behaviour the new tests in test/cli/run/run-crash-handler.test.ts:234-360 assert is only provided by whatever WebKit build is pinned; a later swap to a main sha that predates…
Verification: normal — triggered as soon as oven-sh/WebKit PR 741 merges or closes (or the preview tag is otherwise re-cut), which the pin newly depends on. Mechanism verified in the diff: /home/claude/bun/scripts/build/deps/webkit.ts:6 changes WEBKIT_VERSION from the merged 40-hex sha "f20ce7744553c910bcf16a33faf976af208de091" to "autobuild-preview-pr-741-13e08838". prebuiltUrl at webkit.ts:72-74…
There was a problem hiding this comment.
Correct, and this PR must not merge with this pin. The preview build is here on purpose: it lets CI run the bun side while oven-sh/WebKit#741 is open. When that PR merges, I swap the pin to the merged SHA and check that the prebuilt archives exist for each platform. I leave this thread open until then.
|
Closing: the commits of this PR are on the branch of #40169 now (bb1fd02, 0a553c4, 976c0ed), so that the bun-side hook never reaches main. The reason is a hang that a review of oven-sh/WebKit#741 found and that I reproduced. With The open review thread about the preview pin moves with the pin: I swap it to the merged SHA in #40169 when oven-sh/WebKit#741 merges. |
Stacked on #40169. The bun side of oven-sh/WebKit#741. The pin is its preview build until that PR merges.
Problem
bun build --bytecode, a native stack overflow still exits 139 with an empty stderr. Report native stack overflows on every thread #40169 putsSA_ONSTACKback inZig__GlobalObject__create, butbun buildcreates its first VM invmForBytecodeCache.NodeCompileCache.rs:907) never callsconfigure_thread(), so it has no alternate signal stack.Fix
CrashHandler__keepSignalHandlersOnAltStackand its once-flag.configure_thread().WTF::SignalHandlers::finalize(), whichever place creates the first VM.test/cli/run/run-crash-handler.test.ts(three new tests) on debug builds with and without ASAN, and the 14test-compile-cache-*.jstests.Background
SA_ONSTACKand chains to bun's. The kernel delivers a guard-page fault only to anSA_ONSTACKhandler, on a thread with an alternate stack.Bun::createVMwith a once-flag and a source lint (this PR's first shape). It restores a flag that our own fork drops at one line. V8 sets it at itssigactioncall (v8/v8@600c599bb0fb).Downsides
Notes
Why the fail-before proof needs the old WebKit build. The change of behaviour comes from the pinned WebKit build, which is outside
src/. With the new pin, the tests pass with and without thesrc/changes of this PR. The table has the proof across builds.The two tests with a preloaded library. No input overflows the native stack in these two places. A library loaded with
LD_PRELOADrecurses until the stack ends. Forbun build --bytecodeit does so inexit()andquick_exit(), after the bytecode is on disk. For the compile cache it does so inpthread_getattr_npon the thread namedBunCompileCache. JSC calls that function on every thread that runs it, to learn the stack bounds.bun build --bytecodeAddressSanitizer: stack-overflowpanic(main thread): Stack overflowpanic(BunCompileCache): Stack overflowAddressSanitizer: stack-overflowAddressSanitizer: stack-overflowASAN gives each thread an alternate stack of its own, so the compile cache test passes on an ASAN build of #40169 too.
The flags that JSC passes to
sigactionfor SIGSEGV and SIGBUS, logged with anLD_PRELOADshim, debug builds without ASAN, forbun -e 1and forbun build --bytecode --target=bun x.js:0x4(SA_SIGINFO) on main (36cd151),0x8000004(SA_SIGINFO | SA_ONSTACK) with this PR. No later call changes them.WebAssembly traps. The same shim wraps the handler and asks
sigaltstackif it runs on the alternate stack. 1,000 out-of-bounds loads on the main thread and 1,000 in a Worker: each throwsWebAssembly.RuntimeError. On main 0 of 2,000 handlers ran on the alternate stack, with this PR 2,000 of 2,000. The third new test keeps that outcome.The compile cache worker, logged with a shim for
NODE_COMPILE_CACHE=dir bun main.cjs. Before: no call. After:sigaltstack(query),mmapof 528384 bytes,mprotectof the guard page,sigaltstack(set, 524288 bytes). The worker lives until the process exits, so nothing is unmapped.Gone from the first shape of this PR:
Bun::createVMwith its once-flag,test/internal/source-lints/vm-creation.test.ts, and the rewrite ofsignal_stack::install_for_current_thread(configure_threadreturns early on a configured thread, so no caller registers twice).vmForBytecodeCachestill dereferences the result ofVM::tryCreatewith no null check. This PR does not change that: it is a different bug, and it is tracked on its own.The Bake production global is never the first VM:
init_bakecallsVirtualMachine::initfirst.When oven-sh/WebKit#741 merges, I swap the pin to the merged SHA. If #40169 is still open then, these commits can move onto its branch, so that the hook never reaches main. #40173 is the other open PR that removes the compile cache thread.
JSC's own threads (heap helpers, JIT worklist) are created by WTF and have no alternate stack. An overflow there stays silent.
Other suites: all of
test/cli/run/run-crash-handler.test.tspasses on a debug build with ASAN (33 pass). Its native stack overflow tests pass on a debug build without ASAN (6 pass).