From 0646f9ca4f500f9a34d3ee706baaba8085f963bf Mon Sep 17 00:00:00 2001 From: ispyisail Date: Thu, 17 Sep 2026 22:44:59 +1200 Subject: [PATCH] Harden the crash reporter: keep every dump, and say what crashed Three weaknesses, all visible in ChuckNr11's report on #898 -- "the report appeared only once despite there being 10 or more crashes". One dump per run instead of one per install ------------------------------------------- crashDumpPath() was a single fixed crash_dump.log, and the handler opens it O_TRUNC, so each crash destroyed the evidence from the one before. Ten crashes left one dump. Dumps now go to a crashes/ directory named crash__.log, and every pending one is offered together, newest first, with a banner saying how many there are. A crash that repeats is exactly the case where the earlier dumps matter, because the difference between them is the evidence. The name is built in normal context and handed to CrashHandler::install(), which copies it into a preallocated buffer as before -- the handler still writes to one fixed path, so its no-allocation invariant is untouched. The dump now says which signal fired ------------------------------------ The header is built once at install(), so every dump looked identical no matter what killed the process -- and SIGSEGV and SIGABRT point at very different bugs. Written with an async-signal-safe integer formatter into a stack buffer, since snprintf is not on the POSIX safe list. ...and where it was ------------------- The ring said what the program was doing; nothing said where it died. The dump now carries a backtrace. backtrace() is warmed once in install() so its first-call lazy resolution cannot allocate inside the handler, and backtrace_symbols_fd() writes straight to the fd -- unlike backtrace_symbols(), which mallocs and must never be used here. Guarded on __has_include() so platforms without it are unaffected. QET's own frames currently resolve as offsets rather than names, since the binary does not export its dynamic symbols. They are still resolvable offline: the header records the exact git SHA. Building with -rdynamic would give names directly, but that is a build-flag decision for its own change. Deliberately unchanged: the four invariants in crashhandler.h. Nothing added here allocates, blocks, takes a lock, or swallows the crash. Verified: three consecutive SIGSEGVs now leave three separate dumps, each carrying "Signal: 11" and a backtrace with resolved Qt frames; launching afterwards offers all three in one dialog, newest first, and clears them once shown. ctest 8/8. Co-Authored-By: Claude Opus 5 --- sources/logging/crashhandler.cpp | 83 +++++++++++++++++++- sources/logging/qetlogger.cpp | 128 ++++++++++++++++++++++++++++--- sources/logging/qetlogger.h | 8 +- 3 files changed, 207 insertions(+), 12 deletions(-) diff --git a/sources/logging/crashhandler.cpp b/sources/logging/crashhandler.cpp index 1a66e3a0d..860dc9b88 100644 --- a/sources/logging/crashhandler.cpp +++ b/sources/logging/crashhandler.cpp @@ -36,6 +36,10 @@ #include #include #include +#if __has_include() +#include +#define QET_CRASH_BACKTRACE 1 +#endif #endif namespace { @@ -64,6 +68,39 @@ char g_altstack[65536]; const int kHandledSignals[] = {SIGSEGV, SIGABRT, SIGBUS, SIGFPE, SIGILL}; +#ifdef QET_CRASH_BACKTRACE +// Preallocated here for the same reason as everything else in this block: +// backtrace() fills a caller-supplied array, so it needs no heap of its +// own, and backtrace_symbols_fd() writes straight to the fd (unlike +// backtrace_symbols(), which mallocs and is therefore unusable here). +void *g_backtrace_frames[64]; +#endif + +// Async-signal-safe decimal formatting: write() takes a buffer, and there +// is no snprintf on the POSIX async-signal-safe list. Writes into a +// caller-owned buffer (stack, not heap) and returns the length used. +int formatInt(char *buffer, int size, int value) +{ + if (size <= 0) return 0; + if (value == 0) { + buffer[0] = '0'; + return 1; + } + char scratch[16]; + int n = 0; + bool negative = value < 0; + unsigned int v = negative ? static_cast(-(value + 1)) + 1u + : static_cast(value); + while (v > 0 && n < static_cast(sizeof(scratch))) { + scratch[n++] = static_cast('0' + (v % 10)); + v /= 10; + } + int len = 0; + if (negative && len < size) buffer[len++] = '-'; + while (n > 0 && len < size) buffer[len++] = scratch[--n]; + return len; +} + void restoreDefaultAndReraise(int sig) { struct sigaction sa {}; @@ -85,13 +122,46 @@ void signalHandler(int sig) return; } - // open/write/close are all on the POSIX async-signal-safe function - // list; nothing else is called here. + // open/write/close, and backtrace_symbols_fd, are all on the POSIX + // async-signal-safe function list; nothing else is called here. const int fd = ::open(g_dump_path, O_WRONLY | O_CREAT | O_TRUNC, 0600); if (fd >= 0) { if (g_header_len > 0) { ::write(fd, g_header, static_cast(g_header_len)); } + + // Which signal killed it. The header is built once at install() + // and is therefore identical for every crash, so without this the + // dump never said what actually happened -- SIGSEGV and SIGABRT + // point at very different bugs. + char line[64]; + int len = 0; + const char kSignalLabel[] = "Signal: "; + for (unsigned i = 0 ; i < sizeof(kSignalLabel) - 1 ; ++i) { + line[len++] = kSignalLabel[i]; + } + len += formatInt(line + len, static_cast(sizeof(line)) - len - 1, sig); + line[len++] = '\n'; + ::write(fd, line, static_cast(len)); + +#ifdef QET_CRASH_BACKTRACE + // The log ring says what the program was doing; this says where it + // was when it died. backtrace() is warmed in install() so its + // first-call lazy resolution cannot allocate here, and + // backtrace_symbols_fd() writes to the fd without allocating -- + // unlike backtrace_symbols(), which mallocs and must not be used. + const char kBacktraceLabel[] = "--- backtrace ---\n"; + ::write(fd, kBacktraceLabel, sizeof(kBacktraceLabel) - 1); + const int frames = ::backtrace(g_backtrace_frames, + static_cast(sizeof(g_backtrace_frames) + / sizeof(g_backtrace_frames[0]))); + if (frames > 0) { + ::backtrace_symbols_fd(g_backtrace_frames, frames, fd); + } + const char kRingLabel[] = "--- log ---\n"; + ::write(fd, kRingLabel, sizeof(kRingLabel) - 1); +#endif + if (g_ring) { g_ring->dumpToFd(fd); } @@ -159,6 +229,15 @@ void CrashHandler::install(const LogRing *ring, const QString &dump_path) ss.ss_flags = 0; sigaltstack(&ss, nullptr); +#ifdef QET_CRASH_BACKTRACE + // Warm the unwinder. backtrace()'s *first* call resolves dynamic + // linker state and may allocate; every call after that does not. Doing + // it here, in normal context, is what lets the handler call it without + // breaking invariant 2. The result is deliberately discarded. + void *warmup[4]; + (void) ::backtrace(warmup, 4); +#endif + struct sigaction sa {}; sa.sa_handler = signalHandler; sigemptyset(&sa.sa_mask); diff --git a/sources/logging/qetlogger.cpp b/sources/logging/qetlogger.cpp index 88bf4d4d9..610a4cc21 100644 --- a/sources/logging/qetlogger.cpp +++ b/sources/logging/qetlogger.cpp @@ -21,6 +21,7 @@ #include "../qetapp.h" #include "../qetversion.h" +#include #include #include #include @@ -104,12 +105,81 @@ void QetLogger::installCrashHandler() if (m_disabled) { return; } - CrashHandler::install(&m_ring, crashDumpPath()); + //Fixed for the life of the process: the handler copies it into a + //preallocated buffer, and pendingCrashDumpFiles() needs to know + //which file is this run's own so it doesn't offer it back. + m_crash_dump_path = buildCrashDumpPath(); + CrashHandler::install(&m_ring, m_crash_dump_path); } -QString QetLogger::crashDumpPath() const +/** + @brief QetLogger::crashDumpDir + @return the directory holding crash dumps, created if missing. + + A directory rather than a single file, because dumps are per-run and + several can be waiting at once. +*/ +QString QetLogger::crashDumpDir() const { - return m_log_dir % QStringLiteral("/crash_dump.log"); + const QString dir = m_log_dir % QStringLiteral("/crashes"); + QDir().mkpath(dir); + return dir; +} + +/** + @brief QetLogger::buildCrashDumpPath + @return where this run would write a crash dump. + + One file per run, rather than a single fixed crash_dump.log. That old + scheme opened one path with O_TRUNC, so a second crash overwrote the + first: someone who crashed ten times still ended up with exactly one + dump, the most recent. Reported on #898 -- "the report appeared only + once despite there being 10 or more crashes" -- where losing the + earlier dumps mattered as much as never being shown them. + + Built here in normal context and handed to CrashHandler::install(), + which copies it into a preallocated buffer, so the handler still + writes to one fixed path and its no-allocation invariant is untouched. +*/ +QString QetLogger::buildCrashDumpPath() const +{ + return crashDumpDir() + % QStringLiteral("/crash_") + % QDateTime::currentDateTime().toString(QStringLiteral("yyyyMMdd-hhmmss")) + % QStringLiteral("_") + % QString::number(QCoreApplication::applicationPid()) + % QStringLiteral(".log"); +} + +/** + @brief QetLogger::pendingCrashDumpFiles + @return dumps left by previous runs, newest first. + + This run's own path is excluded: it does not exist yet unless this run + is itself crashing, and a handler mid-crash is in no position to be + offered a dialog. +*/ +QStringList QetLogger::pendingCrashDumpFiles() const +{ + QDir dir(crashDumpDir()); + dir.setNameFilters({QStringLiteral("crash_*.log")}); + dir.setFilter(QDir::Files); + dir.setSorting(QDir::Time); + + QStringList files; + const QFileInfoList entries = dir.entryInfoList(); + for (const QFileInfo &info : entries) + { + if (info.size() <= 0) { + continue; + } + if (!m_crash_dump_path.isEmpty() + && info.absoluteFilePath() == QFileInfo(m_crash_dump_path).absoluteFilePath()) { + continue; + } + files << info.absoluteFilePath(); + } + return files; } QString QetLogger::currentLogFilePath() const @@ -362,22 +432,62 @@ bool QetLogger::hasPendingCrashDump() const if (m_disabled) { return false; } - const QFileInfo info(crashDumpPath()); - return info.exists() && info.isFile() && info.size() > 0; + return !pendingCrashDumpFiles().isEmpty(); } +/** + @brief QetLogger::pendingCrashDumpContents + @return every pending dump, newest first, concatenated. + + All of them rather than only the latest: a crash that repeats is the + case where the earlier dumps are most worth having, since the + difference between them is the evidence. They are separated by a + banner so a reader can tell where one ends and the next begins, and + the whole thing is redacted as a single pass. +*/ QByteArray QetLogger::pendingCrashDumpContents() const { - QFile file(crashDumpPath()); - if (!file.open(QIODevice::ReadOnly)) { + const QStringList files = pendingCrashDumpFiles(); + if (files.isEmpty()) { return QByteArray(); } - return redact(file.readAll()); + + QByteArray all; + if (files.size() > 1) { + all += QByteArray("QET: ") + QByteArray::number(files.size()) + + " crash dumps pending, newest first.\n\n"; + } + + for (const QString &path : files) + { + QFile file(path); + if (!file.open(QIODevice::ReadOnly)) { + continue; + } + all += "===== " + QFileInfo(path).fileName().toUtf8() + " =====\n"; + all += file.readAll(); + if (!all.endsWith('\n')) { + all += '\n'; + } + all += '\n'; + } + + return redact(all); } +/** + @brief QetLogger::clearPendingCrashDump + Drop the dumps that have just been offered. + + Only those: a dump written by a run that crashed after this list was + taken would otherwise be deleted without ever being seen. +*/ void QetLogger::clearPendingCrashDump() { - QFile::remove(crashDumpPath()); + const QStringList files = pendingCrashDumpFiles(); + for (const QString &path : files) { + QFile::remove(path); + } } QByteArray QetLogger::buildDiagnosticsReport() const diff --git a/sources/logging/qetlogger.h b/sources/logging/qetlogger.h index a4596096a..a72c9a0e6 100644 --- a/sources/logging/qetlogger.h +++ b/sources/logging/qetlogger.h @@ -136,7 +136,9 @@ class QetLogger void rotateLocked(); void writeToFile(const QByteArray &line, QtMsgType type); QString rotatedPath(int index) const; - QString crashDumpPath() const; + QString crashDumpDir() const; + QString buildCrashDumpPath() const; + QStringList pendingCrashDumpFiles() const; QString currentLogFilePath() const; static QByteArray sanitize(const QByteArray &input); @@ -147,6 +149,10 @@ class QetLogger QString m_log_dir; QString m_base_name; // e.g. "20260803", resolved once in init() + /// This run's own dump path, fixed at installCrashHandler(): + /// the handler writes here, and it is excluded when collecting + /// dumps left by previous runs. + QString m_crash_dump_path; QMutex m_file_mutex; QFile m_file;