diff --git a/src/crash_handler/lib.rs b/src/crash_handler/lib.rs index a5cb8c8966ea..3b4a0f6e12b3 100644 --- a/src/crash_handler/lib.rs +++ b/src/crash_handler/lib.rs @@ -399,53 +399,54 @@ pub use bun_io::{FmtAdapter, Write}; /// `bun_sys::stderr_writer()` (not yet exposed by T1). /// Only impls `bun_io::Write` — `write!` resolves to `bun_io::Write::write_fmt` /// (alloc-free stack `Bridge`, async-signal-safe). +/// +/// Waits for a full stderr but never returns `Err` for an unwritable one: +/// callers `abort()` on `Err`, skipping the report upload and signal re-raise. +/// Signal-safe: `bun_sys::write`/`posix::poll` are bare syscalls (on Windows a +/// bare `WriteFile`, not the CRT, whose per-fd lock a VEH handler can deadlock +/// on) and `bun_sys::Error` does not allocate. pub(crate) struct StderrWriter; pub(crate) fn stderr_writer() -> StderrWriter { StderrWriter } impl Write for StderrWriter { - fn write_all(&mut self, bytes: &[u8]) -> bun_io::Result<()> { - #[cfg(windows)] - { - // On Windows this is `GetStdHandle(STD_ERROR_HANDLE)` + kernel32 - // `WriteFile`, NOT the CRT. Routing through MSVCRT `_write(2,…)` - // would (1) text-mode-translate `\n`→`\r\n` and (2) take the CRT - // per-fd lock, which can self-deadlock when the VEH crash handler - // fires on a thread that faulted *inside* CRT stdio. WriteFile is - // lock-free at the kernel32 layer. - // `WriteFile` is declared locally because `bun_windows_sys:: - // kernel32` does not (yet) export it (cf. src/sys/lib.rs). - #[link(name = "kernel32")] - unsafe extern "system" { - fn WriteFile( - hFile: bun_sys::windows::HANDLE, - lpBuffer: *const u8, - nNumberOfBytesToWrite: u32, - lpNumberOfBytesWritten: *mut u32, - lpOverlapped: *mut core::ffi::c_void, - ) -> i32; - } - let h = bun_sys::windows::kernel32::GetStdHandle(bun_sys::windows::STD_ERROR_HANDLE); - let mut written: u32 = 0; - // SAFETY: `h` is the cached stderr HANDLE (or INVALID_HANDLE_VALUE, - // in which case WriteFile fails harmlessly); `bytes` is valid for - // reads of `len`; `written` is a valid out-pointer; lpOverlapped - // is null for synchronous I/O. - unsafe { - WriteFile( - h, - bytes.as_ptr(), - bytes.len() as u32, - &mut written, - core::ptr::null_mut(), - ); - } - } + fn write_all(&mut self, mut bytes: &[u8]) -> bun_io::Result<()> { #[cfg(not(windows))] - { - // SAFETY: fd 2 is always open; libc::write is async-signal-safe. - unsafe { - libc::write(2, bytes.as_ptr().cast(), bytes.len() as _); + let stderr = bun_sys::Fd::stderr(); + // Not `Fd::stderr()`: its cache is filled by `Output`'s stdio init, + // which runs after the crash handler is installed. + #[cfg(windows)] + let Some(stderr) = bun_sys::windows::GetStdHandle(bun_sys::windows::STD_ERROR_HANDLE) + .map(bun_sys::Fd::from_system) + else { + return Ok(()); + }; + while !bytes.is_empty() { + match bun_sys::write(stderr, bytes) { + Ok(0) => break, + Ok(n) => bytes = &bytes[n..], + Err(err) => match err.get_errno() { + // `bun_sys::write` only retries EINTR on Linux; macOS issues + // `write$NOCANCEL` once. + #[cfg(unix)] + bun_sys::E::EINTR => {} + // A piped fd 2 is O_NONBLOCK once `process.stderr` has used + // it (the flag is on the shared open file description). + // Full only means the reader is behind: wait like a blocking + // stderr would instead of dropping the report. + #[cfg(unix)] + bun_sys::E::EAGAIN => { + let mut pfd = [bun_sys::posix::PollFd { + fd: stderr.native(), + events: bun_sys::posix::POLL_OUT, + revents: 0, + }]; + if bun_sys::posix::poll(&mut pfd, -1).is_err() { + break; + } + } + _ => break, + }, } } Ok(()) diff --git a/test/cli/run/run-crash-handler.test.ts b/test/cli/run/run-crash-handler.test.ts index 4d436bbedc7b..c556b89bcd08 100644 --- a/test/cli/run/run-crash-handler.test.ts +++ b/test/cli/run/run-crash-handler.test.ts @@ -1,7 +1,8 @@ import { crash_handler } from "bun:internal-for-testing"; import { describe, expect, test } from "bun:test"; import { bunEnv, bunExe, isASAN, isDebug, isLinux, isPosix, isWindows, mergeWindowEnvs, tempDir } from "harness"; -import { rmSync } from "node:fs"; +import { mkfifo } from "mkfifo"; +import { closeSync, constants, openSync, rmSync, writeSync } from "node:fs"; import path from "path"; const { getMachOImageZeroOffset } = crash_handler; @@ -129,6 +130,95 @@ describe.if(isPosix)("terminal signal reflects the crash cause", () => { }); }); +// The report is written to fd 2 with many small raw write(2) calls. fd 2 is +// often O_NONBLOCK: Bun puts the flag on a piped stderr as soon as +// process.stderr is used, and since it lives on the open file description a +// parent can hand down an fd that already has it. If the reader is behind at +// that moment every write fails with EAGAIN, and the handler used to drop the +// bytes and carry on, so the process died without printing anything. It has +// to wait for the reader instead, like a blocking stderr would. +describe.if(isPosix)("crash report reaches a full non-blocking stderr", () => { + const reportHeader = Buffer.alloc(60, "=").toString() + "\n"; + + test.concurrent.each([ + ["panic", "invoked crashByPanic() handler", "SIGABRT"], + ["segfault", "Segmentation fault at address 0xDEADBEEF", "SIGSEGV"], + ] as const)("%s", async (approach, expectedReason, expectedSignal) => { + using dir = tempDir("crash-stderr-full", {}); + // A fifo is a pipe whose read end this process holds without Bun.spawn + // draining it: the child's stderr has to stay full until the crash handler + // has tried to write to it. + const fifo = path.join(String(dir), "stderr"); + mkfifo(fifo); + const readEnd = openSync(fifo, constants.O_RDONLY | constants.O_NONBLOCK); + let writeEnd: number | undefined = openSync(fifo, constants.O_WRONLY | constants.O_NONBLOCK); + try { + const fill = Buffer.alloc(64 * 1024, "f"); + let filled = 0; + let fillStoppedBy: string | undefined; + while (fillStoppedBy === undefined) { + try { + filled += writeSync(writeEnd, fill); + } catch (e) { + fillStoppedBy = (e as NodeJS.ErrnoException).code; + } + } + expect({ fillStoppedBy, filled: filled > 0 }).toEqual({ fillStoppedBy: "EAGAIN", filled: true }); + + await using proc = Bun.spawn({ + cmd: [ + bunExe(), + "--debug-crash-handler-use-trace-string", + "-e", + // Both requires come first: loading bun:internal-for-testing takes + // over a second in debug builds, and the marker has to be as close + // as possible to the crash itself. + `const { crash_handler } = require("bun:internal-for-testing"); + const { writeSync } = require("node:fs"); + writeSync(1, "crashing\\n"); + crash_handler.${approach}();`, + ], + env: noReportEnv, + stdio: ["ignore", "pipe", writeEnd], + }); + // The child now holds the only write end, so EOF on the fifo means it died. + closeSync(writeEnd); + writeEnd = undefined; + + const { value: marker } = await proc.stdout.getReader().read(); + expect(new TextDecoder().decode(marker)).toBe("crashing\n"); + + // The child crashes right after the marker and every report write hits + // the full fifo within about a millisecond of it (measured on release and + // debug ASAN builds). A handler that waits is then blocked in poll(2), + // which nothing outside the process can observe, so the only evidence + // that it is waiting rather than dead is that it is still alive well + // after that point; a handler that dropped the report is gone within a + // few hundred milliseconds. The sleep is that margin, not a wait for the + // child to get going. + const exitedWithoutReader = await Promise.race([ + proc.exited.then(() => true), + Bun.sleep(1_000).then(() => false), + ]); + expect(exitedWithoutReader, "crash handler gave up on stderr instead of waiting for the reader").toBe(false); + + // Draining the fill unblocks the child; whatever follows it is the report. + const report = (await Bun.file(readEnd).text()).slice(filled); + await proc.exited; + + expect(report).toStartWith(reportHeader); + expect(report).toContain(`panic(main thread): ${expectedReason}\n`); + expect(report).toContain("oh no: Bun has crashed. This indicates a bug in Bun, not your code.\n"); + expect(report.trimEnd().split("\n").at(-1)).toMatch(/^ \/\d+\.\d+\.\d+\/\S+$/); + expect(report).toEndWith("\n\n"); + expect(proc.signalCode).toBe(expectedSignal); + } finally { + if (writeEnd !== undefined) closeSync(writeEnd); + closeSync(readEnd); + } + }); +}); + // POSIX-only: Windows refuses to remove a directory that is any process's cwd. describe.if(isPosix)("cwd deleted before startup", () => { test.concurrent.each(["install", "test"])("bun %s prints the cwd-deleted hint", async cmd => {