Repository navigation
Conversation
…ad that has one SignalHandlers::finalize() installed jscSignalHandler with SA_SIGINFO only. That handler replaces the handler of the embedder and chains to it. An embedder that registers an alternate stack (sigaltstack) and installs its own handler with SA_ONSTACK lost the flag when the first VM was created. After a native stack overflow the kernel then has no room for a signal frame and ends the process before any handler runs. The flag has no effect on a thread without an alternate stack.
|
Preview build of 242085d: |
|
@robobun wake up!! |
…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`.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (3)
Included review availability: This review used your included allowance. Your plan provides up to 5 included reviews per hour; 2 remain after this review. WalkthroughSignal handling exposes interrupted registers in eligible Bun JSC builds. POSIX thread suspension can use these registers when the interrupted stack pointer is outside the thread’s stack. ChangesSignal register capture and thread suspension
Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to No merge-blocking issue is established for the signal-handler and suspension changes; normal validation remains appropriate. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Warning Git: CodeRabbit could not clone the repository, so clone-backed analysis was skipped and this review may be incomplete. Verify repository clone access, such as SSH credentials, before requesting another full review. If clone access is intentionally unavailable, use Comment |
…n be suspended With SA_ONSTACK, jscSignalHandler runs on the alternate signal stack. A suspension that arrives meanwhile runs its handler on that stack too, and Thread::signalHandlerSuspendResume() backs off, because the stack pointer is not in the thread's stack. Thread::suspend() retries with no limit. The handlers of Signal::AccessFault wait for locks that the sampling profiler holds while it suspends the thread: the NativeCalleeRegistry lock and the codeBlockSet lock. Each thread then waits for the other. jscSignalHandler publishes the registers that it interrupted on the thread's own stack, and the suspension uses them. The thread is suspended as if the request arrived immediately before the fault.
There was a problem hiding this comment.
Beyond the inline finding, I also checked two things in the new registersInterruptedBySignalHandler() plumbing and they hold up: the published pointer targets the kernel's ucontext frame on the alternate stack (registersFromUContext returns a reference, not a local copy), so it stays valid for the whole handler including the chained old handler, and the scope exit clears it before return; and the differing guards (OS(UNIX) in Signals.h vs. none in Signals.cpp, no !OS(DARWIN) at the ThreadingPOSIX.cpp call) are harmless because Signals.cpp is wholly inside #if OS(UNIX) and signalHandlerSuspendResume sits inside #if !OS(DARWIN).
Extended reasoning...
The second push adds a thread_local pointer published by jscSignalHandler and consumed by Thread::signalHandlerSuspendResume so a suspend landing on the alternate stack records the interrupted registers instead of retrying forever. The pointer lifetime and the platform-guard consistency across the three files were examined and ruled out as problems; the remaining inline finding about Linux uc_stack.ss_flags semantics is what still needs a human look.
The latest push brings back an issue flagged in an earlier review; replied in that comment's thread.
Still open from earlier reviews (1):
- 🔴
Source/WTF/wtf/threads/Signals.cpp:624—A Bun process whose sampling profiler is on can hang forever when a VM trap or Wasm fault fires on a thread that has an…
If you have decided not to act on one of these findings, resolve its thread (a reply alone leaves it open) and the next review stops counting it. To review this commit again now, use Re-run on its "Claude Code Review" check.
|
I measured two effects of this flag that need a change in WTF next to it. I have both changes on a branch on top of this one and will open it as a pull request against this branch. 1. With the flag, the fault handlers of JSC run on the alternate stack of the thread.
Bun is the case that shows it, because its main thread has an alternate stack. In 2. In an ASan build the VM trap handler overflows ASan's alternate stack. ASan gives every thread an alternate stack of 58,112 bytes (on my machine) without a guard page. In an ASan debug build of Bun with the flag, 5 of 84 runs of five Scripts// wasmtrap.mjs: bun --cpu-prof wasmtrap.mjs 300000
const bytes = new Uint8Array([
0x00,0x61,0x73,0x6d, 0x01,0x00,0x00,0x00,
0x01,0x06,0x01,0x60,0x01,0x7f,0x01,0x7f,
0x03,0x02,0x01,0x00,
0x05,0x03,0x01,0x00,0x01,
0x07,0x05,0x01,0x01,0x66,0x00,0x00,
0x0a,0x09,0x01,0x07,0x00,0x20,0x00,0x28,0x02,0x00,0x0b,
]);
const { f } = new WebAssembly.Instance(new WebAssembly.Module(bytes)).exports;
for (let i = 0; i < 200000; i++) f(i & 0xfff0);
const N = Number(process.argv[2] || 100000);
let traps = 0;
for (let i = 0; i < N; i++) {
try { f(0x10000 + (i & 0xffff)); } catch (e) { traps++; }
}
console.log(`traps=${traps}/${N}`);// vmtrap.js: bun --cpu-prof vmtrap.js 20
const vm = require("node:vm");
let timeouts = 0;
const N = Number(process.argv[2] || 20);
for (let k = 0; k < N; k++) {
try { vm.runInNewContext("let i = 0; while (true) { i = (i + 1) | 0; }", {}, { timeout: 60 }); }
catch (e) { if (String(e.code || e.message).includes("TIMEOUT") || /timed out/i.test(e.message)) timeouts++; else throw e; }
}
console.log("timeouts", timeouts, "of", N);// shim.c: gcc -O2 -shared -fPIC -o shim.so shim.c -ldl, then LD_PRELOAD=./shim.so bun --cpu-prof wasmtrap.mjs 300000
#define _GNU_SOURCE
#include <dlfcn.h>
#include <signal.h>
int sigaction(int sig, const struct sigaction* act, struct sigaction* old)
{
static int (*real)(int, const struct sigaction*, struct sigaction*);
if (!real)
real = dlsym(RTLD_NEXT, "sigaction");
if (act && (sig == SIGSEGV || sig == SIGBUS) && act->sa_flags == SA_SIGINFO) {
struct sigaction copy = *act;
copy.sa_flags |= SA_ONSTACK;
return real(sig, ©, old);
}
return real(sig, act, old);
}The branch is |
|
Correction to my comment above: I wrote it without the second commit here (5586daa) in view. That commit covers point 1.
#742 no longer has a change of |
…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`.
robobun
changed the title
…eep SA_ONSTACK off ASan builds jscSignalHandler decided from uc_stack.ss_flags if the code that it interrupted ran on the alternate stack. Since Linux 4.7 that field holds the flags that the thread passed to sigaltstack(), so the test was the same for every signal on a thread. The outermost invocation on a thread publishes its registers, and registersInterruptedBySignalHandler() returns them only when their stack pointer is in the stack of the thread. A fault inside a handler on the alternate stack leaves the suspension to back off, as before. ASan gives every thread an alternate stack of about 58 KB without a guard page, and the VMTraps handler of an ASan build needs more. ASan builds keep SA_SIGINFO alone. The change is the one of #742.
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline finding, I also checked two things on this push and ruled them out: the guard mismatch between Signals.h (OS(UNIX) && !OS(DARWIN)) and the call site in ThreadingPOSIX.cpp:202 (HAVE(MACHINE_CONTEXT) only) is harmless, since that call sits inside the file's #if !OS(DARWIN) block (lines 104-255); and the published registers pointer stays valid for the suspending thread, because signalHandlerSuspendResume runs nested inside jscSignalHandler and the scope-exit clears it before that frame returns.
Extended reasoning...
The change adds SA_ONSTACK to WTF's SIGSEGV/SIGBUS handler under USE(BUN_JSC_ADDITIONS) and publishes the interrupted registers via a thread_local so Thread::suspend can complete while the thread sits on its alternate stack. One confirmed inline finding covers the platforms where stackPointerOf() returns nullptr; this note records the two additional concerns examined and ruled out from the diff.
…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`.
|
A finding from the work on #742 that also applies to this PR and to oven-sh/bun#40169. What. A thread that has an alternate signal stack cannot be suspended when the suspend handler has Who sets the flag. The runtime of Go, when a process loads it as a shared library: Measured, with a shim that adds the flag, each run with a limit of 30 to 40 s:
So the hang exists on main for the main thread, and the alternate stacks of oven-sh/bun#40169 extend it to Workers. #742 extended it to the threads of WTF, which is how I found it. Fix. de67286, the first commit of #742, directly on top of 242085d. When the handler is not on the thread's own stack, it reads the interrupted stack pointer from its context. If that stack pointer is in the thread's own stack, it publishes the registers of the context. Otherwise it uses the published registers of this PR, as before. It is the check of #235 on top of the handler of this PR. What this PR can take. The first two commits of #742 do not depend on the rest of it:
Both apply as they are: b37f482 is two commits ahead of 242085d, so a fast-forward of this branch takes them. If they stay in #742, oven-sh/bun#40169 needs the build of #742 for the Worker case. |
2 participants
The bun side is oven-sh/bun#40169, pinned to the preview build of this PR.
Problem
WTF::SignalHandlers::finalize()(Source/WTF/wtf/threads/Signals.cpp:597) installsjscSignalHandlerfor SIGSEGV and SIGBUS withoutSA_ONSTACK, over bun's handler, which has the flag.Fix
sa_flags = SA_SIGINFO | SA_ONSTACK, underUSE(BUN_JSC_ADDITIONS), not in ASan builds. The kernel reads the flag only for a thread with an alternate stack.jscSignalHandlerpublishes the registers that it interrupted.Thread::signalHandlerSuspendResume()uses them on the alternate stack when their stack pointer is in the thread's stack: they are the state of that stack at the fault.run-crash-handler.test.tsand the vm, Worker, cpu-prof and fs.watch suites pass.Background
SA_ONSTACK.Thread::suspend()retries with no limit when the suspend handler backs off. JSC's fault handlers wait for locks that the sampling profiler holds while it suspends the thread. With the flag alone,bun --cpu-profhung at a WebAssembly trap.sigactioncall (v8/v8@600c599bb0fb). A hook in bun that adds the flag after the first VM missesbun build --bytecode.Downsides
Notes
The hang with the flag alone (the first commit).
bun --cpu-prof --cpu-prof-interval=100on a loop of out-of-bounds WebAssembly loads hangs at the first traps. The stacks:jscSignalHandler, then the handler atWasmFaultSignalHandler.cpp:100, thenWTF::Lock::lockSlow. It runs on the alternate stack.SamplingProfiler::takeSample(SamplingProfiler.cpp:391), thenThread::suspend, thenThread::yield.With the three commits the loop completes (20,000 traps, 5 of 5 runs), and a loop of 20
node:vmtimeouts under the profiler completes too (3 of 3 runs). bun 1.4.3, where the handler runs on the thread's own stack, completes it too. The bun testwhile the sampling profiler suspends the threadtimes out on the build with the flag alone (5 of 5 runs) and passes with the three commits. The handler atVMTraps.cpp:242waits for the codeBlockSet lock in the same way. Its comment names the sampling profiler as the one that can hold that lock.Which registers. The outermost invocation of
jscSignalHandleron a thread publishes its registers.registersInterruptedBySignalHandler(const StackBounds&)returns them only when their stack pointer is in the stack of the thread, which is the condition thatcaptureStackof the thread that suspends depends on. A fault inside a handler on the alternate stack gives a stack pointer outside of it, and the suspension backs off as before. The back-off also stays for a handler that is notjscSignalHandler. The pointer is athread_local: both handlers run on the thread that is suspended.stackPointerOf()reads the stack pointer for x86_64 and arm64 on Linux and FreeBSD, the platforms where bun suspends a thread with a signal. On another platform nothing is published, and a port has to add its case. The second commit testeduc_stack.ss_flagsof the signal frame forSS_ONSTACK. Since Linux 4.7 that field holds the flags that the thread passed tosigaltstack(), so the third commit replaced the test.ASan builds. ASan gives every thread an alternate stack of about 58 KB with no guard page. In an ASan debug build of bun the handler of a VM trap used 72,000 bytes and overflowed it: 5 of 84 runs of five
node:vmtimeouts died with SIGSEGV. #742 measured this and has the guard, and the third commit here has the same guard, so that this PR is safe when it merges alone. In an ASan build of bun with this PR the handler has flags0x4, and 0 of 1,000 WebAssembly traps run their handler on the alternate stack.What the thread that suspends sees. Before this PR, a suspension inside
jscSignalHandlergave the registers of the handler code. Now it gives the registers of the fault: the program counter of the instruction that faulted, or the one that a handler set. The sampling profiler then walks the JS stack from the fault.VMTraps::SignalSendersees a thread that is about to execute that instruction.Cost for each trap. The second commit against the flag alone: 217.9 µs (143.7 to 222.5) and 214.7 µs (161.2 to 231.8). The flag alone against bun main: 208.5 µs (186.2 to 221.4) and 204.6 µs (183.5 to 213.5). Debug builds, 7 interleaved runs of 20,000 traps. The spread is larger than each difference.
The two handlers of
Signal::AccessFault(VMTraps.cpp:215,WasmFaultSignalHandler.cpp:160) read and change the saved registers of the interrupted thread. They do not depend on the stack that they run on. In bun the alternate stack of a thread is 512 KiB.The flags that
finalize()passes tosigactionfor SIGSEGV and SIGBUS in bun, logged with anLD_PRELOADshim:0x4before,0x8000004with this PR. The same shim wraps the handler and askssigaltstackif it runs on the alternate stack. 1,000 out-of-bounds WebAssembly loads on the main thread and 1,000 in a Worker: each throwsRuntimeError. Before, 0 of 2,000 handlers ran on the alternate stack. With this PR, 2,000 of 2,000.bun on main registers an alternate stack on the main thread only. oven-sh/bun#40169 registers one on each bun thread. With this PR and that one, bun reports a native stack overflow on the main thread, in a Worker, and at the exit of
bun build --bytecode:panic(main thread): Stack overflow. Before, the process ended with exit 139 and an empty stderr.Only
Signal::AccessFaulthas handlers in the library, so the flag lands on SIGSEGV and SIGBUS. Thejscshell registers more signals for its own options.The handler for thread suspend and resume is a separate
sigactioncall (Source/WTF/wtf/posix/ThreadingPOSIX.cpp). It keepsSA_RESTART | SA_SIGINFO.macOS installs no signal handler here when it uses Mach exceptions (
handlers.useMach), and it suspends a thread withthread_suspend.WTF creates its own threads (heap helpers, JIT worklist) with no alternate stack. The flag changes nothing for them, and a stack overflow there stays silent.
stress/sampling-profiler-bound-function-name.js(ftl-eager-no-cjit) failed once on linux-arm64 and passed when the job ran again. Thejscshell registers no alternate stack, so the new branch of the suspension does not run there.