crash: a panic record must not be able to hang the process it is recording

Adversarial re-review, confirmed against std's source and then measured.

`captureCurrentStackTrace` is not the safe half of `writeCurrentStackTrace`.
`StackIterator.init` picks the `.di` strategy whenever `SelfInfo` can unwind,
`stratOk` accepts `.di` regardless of `allow_unsafe_unwind`, and `.di` takes
`SelfInfo`'s rwlock EXCLUSIVELY on its first call — the only kind of call a
panic record makes — across `dl_iterate_phdr`, a DWARF CFI machine and an
allocation. A panic in there (a smashed stack is a leading reason to be in a
panic handler at all) leaves the lock held, because the unlock is a `defer` in
a frame that never returns, and `defaultPanic` then waits on it for the life of
the process. A crash becomes a hang, which is worse than what this file was
added to improve on. The frames stay on stderr, where defaultPanic prints them
under the staging that makes them safe; the record keeps what can be gathered
without asking the process any questions.

ONE record per process, never released. With the guard released on the way out,
one panic wrote two records: the real message, then "reached unreachable code"
under it. That second panic is this handler's own `vaxis.recover()` running a
second time — it closes the vaxis tty and never clears the global saying there
is one, so the double close is `recoverableOsBugDetected` and an `unreachable`
in a Debug build. Guarded now in both the panic and the segfault handler; that
half is a fix older than the crash file.

`clock_gettime`'s return is checked, unlike dump.zig's, because a failure here
leaves `ts` undefined and an undefined large-positive `sec` walks
`calculateYearDay`'s u16 year past 65535 and overflow-panics inside the panic
handler. Debug fills it with 0xaa and lands in 1970, which is why it reads as
harmless.

Verified end to end with a temporary probe: one panic, one record.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_016Q4RATpafkwahrovHQLKRf

1 files changed, 78 insertions, 56 deletions

diff --git a/src/crash.zig b/src/crash.zig
index 8de2abed..d9c47715 100644
--- a/src/crash.zig
+++ b/src/crash.zig
@@ -10,9 +10,10 @@
 //! that produced it: a bug report needs the version and the commit, and a user
 //! who runs pardes already knows where its config lives.
 //!
-//! The record is the metadata line, the panic message, and the RETURN
-//! ADDRESSES — not the symbolised trace stderr gets. `record` says why in as
-//! many words: symbolising from out here hangs the process, measured.
+//! The record is the metadata line and the panic message, and NOT the frames:
+//! every way of collecting those from out here can wedge the process instead of
+//! ending it. `record` says exactly why, at length, because the reasoning is
+//! not visible from the call it does not make.
 //!
 //! Panics only. A SIGSEGV never reaches a panic handler (main.zig `debug`
 //! hooks that path separately) and unwinding one needs the signal's saved cpu
@@ -56,20 +57,18 @@ else
 /// the fd is opened `O_APPEND` and one `write(2)` of the lot is what keeps two
 /// crashing processes from interleaving their records line by line. Static
 /// rather than a local: a panic handler may be running on a thread stack of a
-/// few pages.
+/// few pages. Two lines fit several times over — the panic message is the only
+/// part with no bound of its own, and a longer one is simply cut.
 var scratch: [8 << 10]u8 = undefined;
 
-/// Deep enough to reach `main` through any of this program's loops, and the
-/// only sizing decision here: the addresses are 8 bytes each and cost a line.
-var addr_buf: [64]usize = undefined;
-
-/// One at a time, and never inside itself. `defaultPanic` gets both properties
-/// from a private `panic_stage` and from the stderr lock, and neither is
-/// reachable from out here: two threads panicking at once would interleave
-/// `@memcpy`s into one `scratch` and each write out the mixture, and a panic
-/// raised INSIDE this function — the walk below is the plausible place — would
-/// re-enter it with the fd still open. Both end here instead, and the loser
-/// falls through to `defaultPanic`, which still says everything on stderr.
+/// One record per process, taken by whoever panics first. `defaultPanic` gets
+/// the same property from a private `panic_stage` and from the stderr lock, and
+/// neither is reachable from out here: two threads panicking at once would
+/// interleave `@memcpy`s into one `scratch` and each write out the mixture, and
+/// the nested panic `defaultPanic`'s own trace printer can raise comes back
+/// through the handler and appends its wreckage under the real message. Both
+/// end here instead, and the loser falls through to `defaultPanic`, which still
+/// says everything on stderr.
 var recording: std.atomic.Value(bool) = .init(false);
 
 /// Append one crash record, or silently do nothing. Called FROM the panic
@@ -77,12 +76,15 @@ var recording: std.atomic.Value(bool) = .init(false);
 /// own, and does its own file I/O: a static buffer, one `open`, one `write`.
 /// Every failure is swallowed — a crash file that could not be written must not
 /// become the crash, and stderr is about to get the message either way.
-pub fn record(msg: []const u8, first_trace_addr: ?usize) void {
+pub fn record(msg: []const u8) void {
     if (dir_len == 0) return;
+    // ONCE PER PROCESS, and never released. A process panics once and dies; a
+    // SECOND record is always the wreckage of the first. Observed with the
+    // flag released on the way out: `defaultPanic`'s own trace printing raised
+    // a nested panic, which came back through the handler and appended
+    // "panic: reached unreachable code" under the real message — burying the
+    // one line the file exists to keep. A test resets it deliberately.
     if (recording.swap(true, .seq_cst)) return;
-    // Released on the way out, which matters to nobody at panic time — the
-    // process is about to abort — and lets a test call this twice.
-    defer recording.store(false, .seq_cst);
     const config_dir = dir_buf[0..dir_len];
     var path_buf: [1024:0]u8 = undefined;
     if (config_dir.len + 1 + name.len >= path_buf.len) return;
@@ -113,8 +115,14 @@ pub fn record(msg: []const u8, first_trace_addr: ?usize) void {
     // The metadata line. Same clock and the same civil-time arithmetic as
     // dump.zig's filename, printed as UTC because a crash file is read by
     // whoever the reporter sends it to and their zone is not the reporter's.
-    var ts: std.c.timespec = undefined;
-    _ = std.c.clock_gettime(.REALTIME, &ts);
+    //
+    // The return IS checked, unlike dump.zig's, because this one runs where a
+    // panic cannot be afforded: a failed `clock_gettime` leaves `ts` undefined,
+    // and an undefined large-positive `sec` walks `calculateYearDay`'s `u16`
+    // year past 65535 and overflow-panics INSIDE the panic handler. Debug fills
+    // it with 0xaa and lands in 1970, which is why it reads as harmless.
+    var ts: std.c.timespec = .{ .sec = 0, .nsec = 0 };
+    if (std.c.clock_gettime(.REALTIME, &ts) != 0) ts = .{ .sec = 0, .nsec = 0 };
     const es: std.time.epoch.EpochSeconds = .{ .secs = @intCast(@max(0, ts.sec)) };
     const yd = es.getEpochDay().calculateYearDay();
     const md = yd.calculateMonthDay();
@@ -134,34 +142,43 @@ pub fn record(msg: []const u8, first_trace_addr: ?usize) void {
         @as(u32, @intCast(std.c.getpid())),
     }) catch {};
 
-    // ...and under it the message stderr is about to print, then the return
-    // addresses behind it.
+    // ...and under it the line stderr is about to print.
+    //
+    // NO STACK TRACE, AND NOT EVEN THE RAW RETURN ADDRESSES. This is the whole
+    // design decision in this file, and it was arrived at by measurement, so it
+    // is written down here rather than left to be rediscovered.
+    //
+    // `std.debug.writeCurrentStackTrace` — the call `defaultPanic` makes a
+    // moment later, into a writer of its own — HANGS THE PROCESS when it is
+    // made from a panic handler BEFORE `defaultPanic` has run. Observed at 0%
+    // CPU in `futex_do_wait` with libc's debug info half-open, identically in a
+    // test binary and in a standalone build carrying this program's
+    // `std_options_debug_io`. Calling `debug_io.vtable.crashHandler` first does
+    // not help, and neither does holding `lockStderr` around it.
     //
-    // ADDRESSES AND NOT THE SYMBOLISED TRACE, which is the one thing here that
-    // is not simply "the same bytes stderr gets". `std.debug.writeCurrentStack
-    // Trace` — the call `defaultPanic` makes a moment later, into a writer of
-    // its own — HANGS THE PROCESS when it is made from a panic handler before
-    // `defaultPanic` has run. Symbolising reads DWARF, and on a machine with
-    // debug info to fetch that read can itself panic; `defaultPanic` survives
-    // that because its private `panic_stage` turns the nested panic into
-    // "aborting due to recursive panic" and an abort. Outside it there is no
-    // such staging, and the recursion ends in a futex nobody will post: a
-    // panicking pardes stopped dead instead of dying. Measured, not guessed —
-    // with the stack-trace call the process wedged at 0% CPU with libc's debug
-    // info half-open, and it did it identically inside a test binary and in a
-    // standalone build with this program's `std_options_debug_io`.
+    // `captureCurrentStackTrace` looks like the safe half of that and is NOT.
+    // `StackIterator.init` picks the `.di` strategy whenever `SelfInfo` can
+    // unwind, `stratOk` accepts `.di` no matter what `allow_unsafe_unwind`
+    // says, and `.di` unwinding takes `SelfInfo`'s rwlock EXCLUSIVELY on its
+    // first call — the only kind of call a panic record ever makes — while it
+    // runs `dl_iterate_phdr` and a DWARF CFI machine and allocates. A panic in
+    // there (a smashed stack is a leading cause of getting here at all) leaves
+    // that lock held forever, because the unlock is a `defer` in a frame that
+    // never returns. `defaultPanic` then asks for the same lock and waits on it
+    // for the life of the process: A CRASH BECOMES A HANG, which is strictly
+    // worse than the behaviour this file was added to improve on. What makes
+    // `defaultPanic` itself survive that is its own PRIVATE `panic_stage`,
+    // reachable from nowhere out here.
     //
-    // `captureCurrentStackTrace` only walks frames, so it is safe here, and the
-    // addresses resolve with `addr2line -e <the pardes binary>` against the
-    // build the line above names. stderr still gets the symbolised trace from
-    // `defaultPanic`, unchanged.
+    // So the frames stay on stderr, where `defaultPanic` prints them under the
+    // staging that makes them safe, and this file keeps what it can gather
+    // without asking the process any questions: which build, when, where, and
+    // what it said. That is the half a reporter cannot reconstruct afterwards.
     w.print("panic: {s}\n", .{msg}) catch {};
-    const trace = std.debug.captureCurrentStackTrace(.{
-        .first_address = first_trace_addr orelse @returnAddress(),
-        .allow_unsafe_unwind = true,
-    }, &addr_buf);
-    for (trace.return_addresses) |a| w.print("  0x{x}\n", .{a}) catch {};
 
+    // One `write(2)`, and the loop is for a short write rather than for a
+    // second record: `O_APPEND` puts it at the end whatever else is writing,
+    // and nothing above here can fail in a way that leaves it half-built.
     const written = w.buffered();
     var off: usize = 0;
     while (off < written.len) {
@@ -187,8 +204,12 @@ test "a crash record names the build, appends, and carries the panic message" {
     defer dir_len = saved_len;
     setDir(config_dir);
 
-    record("first boom", null);
-    record("second boom", null);
+    record("first boom");
+    // Deliberately released, which the panic path never does — see `recording`.
+    // Two records is how the APPEND is proved, and this is the only caller
+    // allowed to ask for a second one.
+    recording.store(false, .seq_cst);
+    record("second boom");
 
     const path = try std.fs.path.join(gpa, &.{ config_dir, name });
     defer gpa.free(path);
@@ -207,19 +228,20 @@ test "a crash record names the build, appends, and carries the panic message" {
     try std.testing.expect(stamp < second);
     try std.testing.expect(std.mem.indexOf(u8, bytes, "1970-") == null);
 
-    // ...and at least one return address under the FIRST message, which is the
-    // only part of this that can quietly produce nothing: an unwinder that
-    // refuses the strategy hands back an empty trace and the loop writes no
-    // lines at all. Asserted between the two records so it cannot be satisfied
-    // by the second one's.
-    const first_msg = std.mem.indexOf(u8, bytes, "panic: first boom\n").?;
-    const addr = std.mem.indexOfPos(u8, bytes, first_msg, "\n  0x").?;
-    try std.testing.expect(addr < second);
+    // Two lines per record and no third: the frames deliberately do NOT come
+    // here (see `record`), and a future edit that starts collecting them again
+    // is the thing this pins. Counted rather than pattern-matched, because the
+    // failure being guarded is "something extra appeared", which no assertion
+    // about known content can see.
+    // Three newlines a record: the blank one that separates records, the end of
+    // the metadata line, and the end of the `panic:` line.
+    try std.testing.expectEqual(@as(usize, 6), std.mem.count(u8, bytes, "\n"));
 
     // No directory, no file: a core that never resolved a config directory
     // panics exactly as it did before this existed.
     dir_len = 0;
-    record("unrecorded", null);
+    recording.store(false, .seq_cst);
+    record("unrecorded");
     const again = try std.Io.Dir.cwd().readFileAlloc(io, path, gpa, .limited(1 << 20));
     defer gpa.free(again);
     try std.testing.expectEqual(bytes.len, again.len);