Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 47 additions & 9 deletions src/bun.js/bindings/FuzzilliREPRL.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
#include <cstdlib>
#include <cstring>
#include <fcntl.h>
#include <mutex>
#include <sanitizer/asan_interface.h>
#include <sys/mman.h>
#include <sys/stat.h>
Expand All @@ -19,20 +20,49 @@

extern "C" {

// Signal handler to ensure output is flushed before crash
static void fuzzilliSignalHandler(int sig)
// Previous handlers (JSC's jscSignalHandler, which itself chains to ASAN's
// handler). We flush stdio so Fuzzilli captures buffered output, then forward
// to the prior handler so JSC's VMTraps/WASM fault handling keeps working and
// ASAN can print its report. Using signal()+SIG_DFL here used to swallow both,
// leaving crash reports with just "TERMSIG: 11" and no stack.
static struct sigaction fuzzilliOldActions[NSIG];

static void fuzzilliSignalHandler(int sig, siginfo_t* info, void* ucontext)
{
// Flush all output
fflush(stdout);
fflush(stderr);
fsync(STDOUT_FILENO);
fsync(STDERR_FILENO);
Comment on lines 32 to 36

@coderabbitai coderabbitai Bot May 2, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🌐 Web query:

Is fflush async-signal-safe according to POSIX, and what is the recommended way to ensure crash output is visible from a signal handler?

💡 Result:

No, fflush is not async-signal-safe according to POSIX. The official POSIX list of async-signal-safe functions does not include fflush or any stdio functions like printf. Instead, use low-level write to STDERR_FILENO (fd 2) or STDOUT_FILENO (fd 1) for output in signal handlers, followed by fsync if needed to ensure visibility, then _exit. This avoids stdio buffering issues and ensures the output is flushed to the kernel without relying on process termination cleanup, which may not occur reliably on crashes like SIGSEGV.

Citations:


Replace fflush() calls with async-signal-safe write() to avoid deadlock in signal handler.

The fflush(stdout) and fflush(stderr) calls at lines 31–35 are not async-signal-safe. If the signal interrupts code that holds the stdio lock, the handler will deadlock before the chained JSC/ASAN handler runs, defeating the purpose of this patch. Use write(STDOUT_FILENO, ...) and write(STDERR_FILENO, ...) instead; the fsync() calls that follow are async-signal-safe and can remain.

Current code
    // Flush all output
    fflush(stdout);
    fflush(stderr);
    fsync(STDOUT_FILENO);
    fsync(STDERR_FILENO);
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/bun.js/bindings/FuzzilliREPRL.cpp` around lines 31 - 35, Replace the
non-async-signal-safe fflush(stdout) and fflush(stderr) calls with
async-signal-safe write() calls: write(STDOUT_FILENO, <buffer>, <len>) and
write(STDERR_FILENO, <buffer>, <len>) using the appropriate output buffers or a
small newline/flush string; keep the existing fsync(STDOUT_FILENO) and
fsync(STDERR_FILENO) calls. Locate the block containing fflush(stdout);
fflush(stderr); fsync(STDOUT_FILENO); fsync(STDERR_FILENO) in FuzzilliREPRL.cpp
and change the two fflush calls to write calls (using STDOUT_FILENO and
STDERR_FILENO) so the handler remains async-signal-safe.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fflush() calls are pre-existing (present before this PR) and intentional — the whole point of this handler is to drain stdio's userspace buffers so Fuzzilli captures buffered output before the process dies. write(STDOUT_FILENO, …) can only push bytes we already hold; it cannot flush FILE* buffers, so swapping them in would make the handler a no-op for its stated purpose.

The async-signal-safety risk (deadlock if we crash while holding stdio's lock) is accepted here: the process is terminating regardless, and the next handler in the chain (WTF::jscSignalHandler, ASAN's reporter) is already non-signal-safe — they allocate, walk stacks, and write through stdio themselves. This is the standard crash-reporting trade-off.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@robobun, that's a fair and well-reasoned explanation. You're right that write() cannot drain FILE* userspace buffers, so substituting it would silently drop exactly the buffered output Fuzzilli needs to see. And the signal-safety concern is moot in practice here because the process is already on its way out and the downstream handlers (WTF::jscSignalHandler, ASAN) are themselves non-signal-safe by necessity. This is the standard crash-reporter trade-off. Happy to dismiss this comment.

(・ω・)っ🐇


✏️ Learnings added
Learnt from: robobun
Repo: oven-sh/bun PR: 30092
File: src/bun.js/bindings/FuzzilliREPRL.cpp:31-35
Timestamp: 2026-05-02T02:52:13.970Z
Learning: In `src/bun.js/bindings/FuzzilliREPRL.cpp` (oven-sh/bun), the `fuzzilliSignalHandler` intentionally calls `fflush(stdout)` and `fflush(stderr)` before forwarding to the chained handler. This is by design: the purpose is to drain `FILE*` userspace buffers so Fuzzilli captures buffered output before the process dies. `write(STDOUT_FILENO, …)` cannot flush `FILE*` buffers and would be a no-op for this purpose. The async-signal-safety risk (deadlock if the signal fires while the stdio lock is held) is an accepted trade-off because (a) the process is terminating regardless, and (b) the downstream chained handlers (`WTF::jscSignalHandler`, ASAN's reporter) are themselves non-async-signal-safe. Do not flag these `fflush()` calls as unsafe in future reviews.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.


🧠 Learnings used
Learnt from: robobun
Repo: oven-sh/bun PR: 29389
File: src/bun.js/bindings/V8HeapSnapshotBuilder.cpp:592-594
Timestamp: 2026-04-17T04:59:15.762Z
Learning: When reviewing C++ code that uses WebKit Template Framework (WTF)/JSC types, do not treat `if (str.isEmpty()) str = String();` as dead code or a no-op. In WTF, `WTF::String::isEmpty()` returns true for both null strings and empty non-null strings, while `String()` constructs a null string. This conditional therefore normalizes empty non-null strings into null strings and may change behavior for downstream callers that distinguish null vs empty—so it should not be flagged as redundant solely on that basis.

Learnt from: robobun
Repo: oven-sh/bun PR: 29867
File: packages/bun-uws/src/App.h:284-285
Timestamp: 2026-04-28T22:49:27.619Z
Learning: When reviewing C++ move constructors/assignments, do not require calling `.clear()` on objects that are being moved-from when they are `std::vector` (or other standard containers) members. Moving a `std::vector` transfers ownership safely; the moved-from vector remains in a valid state and should not cause double-free by itself. Only flag issues if there are non-standard ownership/raw-pointer/double-destruction patterns beyond the default `std::vector` move behavior.


// Re-raise the signal with default handler
struct sigaction& old = fuzzilliOldActions[sig];
if (old.sa_flags & SA_SIGINFO) {
if (old.sa_sigaction) {
old.sa_sigaction(sig, info, ucontext);
return;
}
} else if (old.sa_handler && old.sa_handler != SIG_DFL && old.sa_handler != SIG_IGN) {
old.sa_handler(sig);
return;
}

// No previous handler: re-raise with default handler.
signal(sig, SIG_DFL);
raise(sig);
}

static void installFuzzilliSignalHandler(int sig)
{
struct sigaction action;
action.sa_sigaction = fuzzilliSignalHandler;
sigfillset(&action.sa_mask);
// SA_ONSTACK so stack-overflow faults are delivered on the sigaltstack
// ASAN/JSC already set up; otherwise the kernel can't push the frame and
// the process dies with a bare SIGSEGV before we reach the chain.
action.sa_flags = SA_SIGINFO | SA_ONSTACK;
sigaction(sig, &action, &fuzzilliOldActions[sig]);
}
Comment on lines +54 to +64

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# First, verify the file exists and read the relevant section
head -70 src/bun.js/bindings/FuzzilliREPRL.cpp | tail -25

Repository: oven-sh/bun

Length of output: 1030


🏁 Script executed:

# Search for sigaction usage in the codebase to see error handling patterns
rg "sigaction" --type cpp -B2 -A5

Repository: oven-sh/bun

Length of output: 10135


🏁 Script executed:

# Check for similar error handling patterns with system calls in C++ files
rg "if.*!=.*0\)" src/bun.js/bindings --type cpp -B2 -A2 | head -40

Repository: oven-sh/bun

Length of output: 2960


🏁 Script executed:

# Look for struct initialization patterns in the file
rg "struct.*=" src/bun.js/bindings/FuzzilliREPRL.cpp --type cpp

Repository: oven-sh/bun

Length of output: 153


Initialize struct sigaction and handle sigaction() errors.

The function leaves struct sigaction uninitialized and ignores sigaction() return values, creating two issues: uninitialized struct fields can cause undefined behavior when chained in fuzzilliSignalHandler, and silent failures disable handler installation. Other signal handling code in the codebase (c-bindings.cpp, SigintWatcher.cpp, BunProcess.cpp) consistently zero-initializes sigaction structs using either aggregate initialization or memset(), and c-bindings.cpp checks the sigaction() return value.

Proposed fix
 static void installFuzzilliSignalHandler(int sig)
 {
-    struct sigaction action;
+    struct sigaction action = {};
     action.sa_sigaction = fuzzilliSignalHandler;
     sigfillset(&action.sa_mask);
     // SA_ONSTACK so stack-overflow faults are delivered on the sigaltstack
     // ASAN/JSC already set up; otherwise the kernel can't push the frame and
     // the process dies with a bare SIGSEGV before we reach the chain.
     action.sa_flags = SA_SIGINFO | SA_ONSTACK;
-    sigaction(sig, &action, &fuzzilliOldActions[sig]);
+    if (sigaction(sig, &action, &fuzzilliOldActions[sig]) != 0) {
+        fprintf(stderr, "[fuzzilli] sigaction(%d) failed: %s\n", sig, strerror(errno));
+        std::abort();
+    }
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/bun.js/bindings/FuzzilliREPRL.cpp` around lines 54 - 64, The
installFuzzilliSignalHandler function leaves its local struct sigaction 'action'
uninitialized and ignores the return value of sigaction(), which can cause UB
and silent failures; fix it by zero-initializing the struct (e.g.,
aggregate-init or memset) before setting fields, ensure sa_mask is initialized
(you can keep sigfillset), set sa_sigaction=fuzzilliSignalHandler and
sa_flags=SA_SIGINFO|SA_ONSTACK as done now, then call sigaction(sig, &action,
&fuzzilliOldActions[sig]) and check its return value, logging or handling errors
on failure (consistent with other files like c-bindings.cpp) so handler
installation failures aren’t ignored.


// Implementation of the global fuzzilli() function for Bun
// This function is used by Fuzzilli to:
// 1. Test crash detection with fuzzilli('FUZZILLI_CRASH', type)
Expand Down Expand Up @@ -259,12 +289,20 @@ void Bun__REPRL__registerFuzzilliFunctions(Zig::GlobalObject* globalObject)
{
JSC::VM& vm = globalObject->vm();

// Install signal handlers to ensure output is flushed before crashes
// This is important for ASAN output to be captured
signal(SIGABRT, fuzzilliSignalHandler);
signal(SIGSEGV, fuzzilliSignalHandler);
signal(SIGILL, fuzzilliSignalHandler);
signal(SIGFPE, fuzzilliSignalHandler);
// Install signal handlers to ensure output is flushed before crashes.
// Chain to the previous handler (JSC's jscSignalHandler → ASAN) so
// VMTraps/WASM fault handling keeps working and ASAN reports are printed.
// Signal dispositions are process-wide; this function runs once per
// GlobalObject (main thread, macros, and every Worker), so guard with a
// once-flag — re-installing would save ourselves into fuzzilliOldActions
// and recurse on the next signal.
static std::once_flag installOnce;
std::call_once(installOnce, [] {
installFuzzilliSignalHandler(SIGABRT);
installFuzzilliSignalHandler(SIGSEGV);
installFuzzilliSignalHandler(SIGILL);
installFuzzilliSignalHandler(SIGFPE);
});

globalObject->putDirectNativeFunction(
vm,
Expand Down
90 changes: 90 additions & 0 deletions test/internal/fuzzilli-signal-chain.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,90 @@
// The fuzzilli REPRL setup installs SIGSEGV/SIGILL/SIGFPE/SIGABRT handlers so
// buffered stdio is flushed before a crash. Those handlers must chain to the
// previously-installed handler (WTF::jscSignalHandler → ASAN) instead of
// re-raising with SIG_DFL, otherwise:
// • ASAN never prints a report for null derefs / wild accesses, so every
// signal-based fuzzer crash shows up as a bare "TERMSIG: 11".
// • JSC's signal-based VMTraps and WASM fault handling are broken, turning
// intentional JIT trap breakpoints into hard process crashes.
//
// Only runs when the binary under test was built with FUZZILLI_ENABLED (the
// `fuzzilli()` global exists). Normal debug/release builds skip.

import { expect, test } from "bun:test";
import { bunEnv, bunExe } from "harness";

declare const fuzzilli: unknown;
const isFuzzilliBuild = typeof fuzzilli === "function";

// Skip symbolization so ASAN writes its report and exits immediately instead
// of shelling out to llvm-symbolizer for every frame (several seconds on the
// fuzz binary). The presence of "AddressSanitizer: SEGV" is enough to prove
// the handler chain reached ASAN. allow_user_segv_handler keeps JSC from
// disabling its own fault handling when it sees ASAN_OPTIONS is set.
const fastCrashEnv = {
...bunEnv,
ASAN_OPTIONS: [bunEnv.ASAN_OPTIONS, "allow_user_segv_handler=1", "symbolize=0"].filter(Boolean).join(":"),
};

test.skipIf(!isFuzzilliBuild)("fuzzilli crash signal handler chains to JSC/ASAN for SIGSEGV", async () => {
// FUZZILLI_CRASH type 5 writes to a volatile null pointer. With a working
// handler chain the fault reaches jscSignalHandler → ASAN and we get an
// "AddressSanitizer: SEGV" report on stderr; with signal()+SIG_DFL the
// process dies with stderr containing only the [COV] banner.
await using proc = Bun.spawn({
cmd: [bunExe(), "-e", 'fuzzilli("FUZZILLI_CRASH", 5);'],
env: fastCrashEnv,
stdout: "pipe",
stderr: "pipe",
});

const [stdout, stderr] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);

expect(stdout).toContain("FUZZILLI_CRASH: 5");
expect(stderr).toContain("AddressSanitizer: SEGV");
// ASAN aborts after printing; a bare SIGSEGV would surface as signalCode
// "SIGSEGV" with nothing useful on stderr.
expect(proc.signalCode).not.toBe("SIGSEGV");
});

test.skipIf(!isFuzzilliBuild)("fuzzilli crash signal handler survives Worker global creation", async () => {
// Bun__REPRL__registerFuzzilliFunctions runs for every GlobalObject (main,
// macros, Workers). Without a once-guard the Worker's call re-installs the
// handler, saving fuzzilliSignalHandler itself into fuzzilliOldActions and
// turning the next SIGSEGV into infinite self-recursion → bare TERMSIG 11.
await using proc = Bun.spawn({
cmd: [
bunExe(),
"-e",
'const w = new Worker("data:text/javascript,postMessage(0)"); await new Promise(r => (w.onmessage = r)); w.terminate(); fuzzilli("FUZZILLI_CRASH", 5);',
],
env: fastCrashEnv,
stdout: "pipe",
stderr: "pipe",
});

const [stdout, stderr] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);

expect(stdout).toContain("FUZZILLI_CRASH: 5");
expect(stderr).toContain("AddressSanitizer: SEGV");
expect(proc.signalCode).not.toBe("SIGSEGV");
});

test.skipIf(!isFuzzilliBuild)("fuzzilli crash signal handler still terminates for SIGABRT", async () => {
// FUZZILLI_CRASH type 0 is std::abort(). No JSC/ASAN handler is registered
// for SIGABRT by default, so the chain falls through to SIG_DFL and the
// process terminates with SIGABRT — Fuzzilli's crash detection relies on
// this.
await using proc = Bun.spawn({
cmd: [bunExe(), "-e", 'fuzzilli("FUZZILLI_CRASH", 0);'],
env: fastCrashEnv,
stdout: "pipe",
stderr: "pipe",
});

const [stdout, , exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);

expect(stdout).toContain("FUZZILLI_CRASH: 0");
expect(exitCode).not.toBe(0);
expect(proc.signalCode).toBe("SIGABRT");
Comment on lines +85 to +89

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Make the exit-code assertion last in this subprocess test.

Please move expect(exitCode).not.toBe(0) after the signalCode assertion to match the repo’s failure-diagnostics style.

Proposed fix
   expect(stdout).toContain("FUZZILLI_CRASH: 0");
-  expect(exitCode).not.toBe(0);
   expect(proc.signalCode).toBe("SIGABRT");
+  expect(exitCode).not.toBe(0);
 });

Based on learnings: in test/**/*.test.ts, when asserting subprocess results, keep the exit-code assertion last to preserve more useful failure output.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const [stdout, , exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
expect(stdout).toContain("FUZZILLI_CRASH: 0");
expect(exitCode).not.toBe(0);
expect(proc.signalCode).toBe("SIGABRT");
const [stdout, , exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
expect(stdout).toContain("FUZZILLI_CRASH: 0");
expect(proc.signalCode).toBe("SIGABRT");
expect(exitCode).not.toBe(0);
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@test/internal/fuzzilli-signal-chain.test.ts` around lines 85 - 89, Reorder
the assertions in the test so the subprocess signal assertion runs before the
exit-code check: move the line containing expect(exitCode).not.toBe(0) to after
the expect(proc.signalCode).toBe("SIGABRT") call so the test asserts stdout,
then proc.signalCode, then exitCode (using the existing variables stdout, proc,
signalCode, exitCode).

});
Loading