From f8c1b5206a07ed7d5e762a59f9bf672e78bb7142 Mon Sep 17 00:00:00 2001 From: ispyisail Date: Fri, 18 Sep 2026 14:31:49 +1200 Subject: [PATCH] Address review on #905: offered-list semantics, dump ordering, FreeBSD, legacy dumps Five things raised in review, plus tests for the parts that were only described in prose. clearPendingCrashDump() did not do what its comment said. It called pendingCrashDumpFiles() again at clear time, so it deleted whatever was in the directory then, not what had been offered. The offer sits inside a modal dialog that stays open as long as the user reads it, and SingleApplication keys its socket on the binary path, so a second QElectroTech build running alongside is a separate process that can crash and write a dump in that window. Re-listing deleted that dump unseen -- the exact failure this change exists to fix. The list is now taken once in QETApp::checkCrashDump() and passed to both pendingCrashDumpContents() and clearPendingCrashDump(). The ring is now written before the backtrace. backtrace() unwinds through libgcc, which calls dl_iterate_phdr and takes the loader lock; warming it in install() removes the allocation but not the lock. Crashing inside dlopen() (Qt plugin loading), or on a corrupted stack, could therefore hang or re-fault the handler at the backtrace and lose the ring with it. Order is now header, signal, ring, backtrace, so the cheapest and most valuable part is already on disk before anything that can block. The class comment claimed the handler takes no locks; that was not strictly true and now says so. QET_CRASH_BACKTRACE comes from find_package(Backtrace) rather than __has_include(). The header exists on FreeBSD but backtrace() lives in libexecinfo there, so the probe compiled and the link failed. A crash_dump.log left by a pre-#905 version is migrated into crashes/ at startup, named from its own mtime. Otherwise upgrading stranded it: the new code never looks at that path, so the dump from the crash that prompted the upgrade would sit there unoffered forever. Also from the review: dumps are capped at the 10 newest, so a crash loop cannot fill the log directory before any dialog is shown; crashDumpDir() no longer creates the directory as a side effect of a const getter (ensureCrashDumpDir() does that for the callers that write); and redact() now masks an AppImage's per-run /tmp/.mount_XXXXXX prefix, which backtrace_symbols_fd() writes into every frame. Two test executables, both of which were checked to fail against the behaviour they replace: - tst_crashhandler covers CrashHandler::formatInt(), which had no coverage at all despite running only inside a signal handler, where nothing can assert: zero, negatives, INT_MIN (negated through unsigned, since -INT_MIN is UB), INT_MAX, truncation and a zero-sized buffer, each checked against a sentinel-filled buffer so a write past the reported length fails. - tst_crashdumps covers the bookkeeping: ordering, empty dumps, the exclusion of this run's own path, the cap, concatenation of every offered dump, that clearing deletes only what was offered, and what redact() masks. qetlogger.cpp needs exactly one symbol from the application, QETApp::dataDir(), which the test supplies itself. Not addressed here: the timestamp in crash__ is the launch time, not the crash time -- correct as observed, and the commit message that implied otherwise was the thing that was wrong. Resolvable QET frames for AppImage/Flatpak/Snap/Debian need -rdynamic and archived debug symbols, which is a packaging discussion, not this change. Co-Authored-By: Claude Opus 5 --- CMakeLists.txt | 15 ++ sources/logging/crashhandler.cpp | 96 +++++---- sources/logging/crashhandler.h | 27 ++- sources/logging/qetlogger.cpp | 146 ++++++++++++-- sources/logging/qetlogger.h | 48 +++-- sources/qetapp.cpp | 11 +- tests/qttest/CMakeLists.txt | 47 +++++ tests/qttest/tst_crashdumps.cpp | 312 ++++++++++++++++++++++++++++++ tests/qttest/tst_crashhandler.cpp | 149 ++++++++++++++ 9 files changed, 774 insertions(+), 77 deletions(-) create mode 100644 tests/qttest/tst_crashdumps.cpp create mode 100644 tests/qttest/tst_crashhandler.cpp diff --git a/CMakeLists.txt b/CMakeLists.txt index 4fc0fffd4..c60400dcb 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -101,6 +101,21 @@ else() message(STATUS "Qt Qml module not available: JavaScript scripting (--run) disabled") endif() +# The crash handler writes a backtrace into the dump. Detecting this with +# __has_include() is not enough: the header is present on FreeBSD +# too, but backtrace() lives in a separate libexecinfo there, so the compile +# succeeds and the link fails. FindBacktrace resolves both the header and +# whichever library actually provides the symbol, so gate on it instead - see +# the QET_CRASH_BACKTRACE guard in sources/logging/crashhandler.cpp. +find_package(Backtrace QUIET) +if(Backtrace_FOUND) + list(APPEND QET_PRIVATE_LIBRARIES ${Backtrace_LIBRARIES}) + include_directories(${Backtrace_INCLUDE_DIRS}) + add_compile_definitions(QET_CRASH_BACKTRACE) +else() + message(STATUS "backtrace() not available: crash dumps will carry the log ring without a backtrace") +endif() + find_package(SQLite3 REQUIRED) # CMake < 4.3 only creates the SQLite::SQLite3 target (no SQLite3::SQLite3 diff --git a/sources/logging/crashhandler.cpp b/sources/logging/crashhandler.cpp index 860dc9b88..d498fb77f 100644 --- a/sources/logging/crashhandler.cpp +++ b/sources/logging/crashhandler.cpp @@ -36,9 +36,12 @@ #include #include #include -#if __has_include() + // QET_CRASH_BACKTRACE is defined by CMake, via find_package(Backtrace), + // not by probing for the header here. exists on FreeBSD as + // well, but backtrace() is in a separate libexecinfo there, so a header + // probe compiles and then fails to link. +#ifdef QET_CRASH_BACKTRACE #include -#define QET_CRASH_BACKTRACE 1 #endif #endif @@ -76,31 +79,6 @@ const int kHandledSignals[] = {SIGSEGV, SIGABRT, SIGBUS, SIGFPE, SIGILL}; 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 {}; @@ -124,6 +102,16 @@ void signalHandler(int sig) // open/write/close, and backtrace_symbols_fd, are all on the POSIX // async-signal-safe function list; nothing else is called here. + // + // Async-signal-safe is not the same as lock-free, which is why the + // order below matters. backtrace() unwinds through libgcc, which calls + // dl_iterate_phdr and takes the loader lock. Warming it in install() + // removes the allocation, not the lock -- so a crash that happens + // inside dlopen() (Qt plugin loading), or on a corrupted heap or + // stack, can leave this handler deadlocked or faulting a second time + // at the backtrace. Everything cheaper and more valuable is therefore + // written and flushed first: header, signal, then the log ring. If the + // backtrace never completes, the dump is still there and still useful. const int fd = ::open(g_dump_path, O_WRONLY | O_CREAT | O_TRUNC, 0600); if (fd >= 0) { if (g_header_len > 0) { @@ -140,16 +128,25 @@ void signalHandler(int sig) for (unsigned i = 0 ; i < sizeof(kSignalLabel) - 1 ; ++i) { line[len++] = kSignalLabel[i]; } - len += formatInt(line + len, static_cast(sizeof(line)) - len - 1, sig); + len += CrashHandler::formatInt(line + len, static_cast(sizeof(line)) - len - 1, sig); line[len++] = '\n'; ::write(fd, line, static_cast(len)); + // The ring first: it is the part that says what the program was + // doing, it costs one write, and it takes no lock. + const char kRingLabel[] = "--- log ---\n"; + ::write(fd, kRingLabel, sizeof(kRingLabel) - 1); + if (g_ring) { + g_ring->dumpToFd(fd); + } + #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. + // Then where it was when it died. Last, deliberately: see the + // note above about the loader lock. 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, @@ -158,13 +155,8 @@ void signalHandler(int sig) 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); - } ::close(fd); } @@ -204,6 +196,34 @@ LONG WINAPI windowsExceptionFilter(EXCEPTION_POINTERS *) } // namespace +// 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. +// +// Defined as CrashHandler::formatInt rather than a file-local helper only +// so tst_crashhandler can reach it; it is not called anywhere else. +int CrashHandler::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 CrashHandler::install(const LogRing *ring, const QString &dump_path) { g_ring = ring; diff --git a/sources/logging/crashhandler.h b/sources/logging/crashhandler.h index 9baf96d62..ceda880b2 100644 --- a/sources/logging/crashhandler.h +++ b/sources/logging/crashhandler.h @@ -33,11 +33,15 @@ class LogRing; discussion's own words: "lands last, behind its own switch"), so its invariants are worth restating plainly: - 1. The handler must never block. It takes no locks -- LogRing itself - is lock-free for exactly this reason (see logring.h). A handler - that can hang is worse than no handler: it turns a clean crash - (which at least produces a core dump) into a hung process that has - to be force-killed, producing neither a core dump nor a ring dump. + 1. The handler must never block. It takes no locks of its own -- + LogRing is lock-free for exactly this reason (see logring.h). A + handler that can hang is worse than no handler: it turns a clean + crash (which at least produces a core dump) into a hung process + that has to be force-killed, producing neither a core dump nor a + ring dump. The one exception is deliberate and comes last: + backtrace() unwinds through libgcc, which takes the loader lock, + so it is written after the ring rather than before it. A crash + inside dlopen() then costs the backtrace, not the whole dump. 2. The handler must never allocate. Under heap corruption -- a plausible *cause* of the very crash being handled -- malloc may itself deadlock or fault. Every buffer this code touches at crash @@ -78,6 +82,19 @@ class CrashHandler /// touches QString. static void install(const LogRing *ring, const QString &dump_path); + /// Writes `value` as decimal into `buffer`, at most `size` + /// bytes, and returns how many were written. The handler + /// needs this because write() takes a buffer and snprintf() + /// is not on the async-signal-safe list; `buffer` is caller- + /// owned (the handler's stack), so nothing is allocated. + /// Truncates rather than overflowing when `size` is too + /// small, and writes nothing for `size <= 0`. + /// + /// Public only so tests can reach it -- see + /// tests/qttest/tst_crashhandler.cpp. Nothing else in the + /// application calls it. + static int formatInt(char *buffer, int size, int value); + private: CrashHandler() = delete; }; diff --git a/sources/logging/qetlogger.cpp b/sources/logging/qetlogger.cpp index 610a4cc21..f3ee07a88 100644 --- a/sources/logging/qetlogger.cpp +++ b/sources/logging/qetlogger.cpp @@ -26,6 +26,7 @@ #include #include #include +#include #include namespace { @@ -105,6 +106,12 @@ void QetLogger::installCrashHandler() if (m_disabled) { return; } + //Both run here, in normal startup context, before this run's own + //dump path is fixed: a dump left by a pre-#905 version is moved + //in so it can still be offered, and any backlog is trimmed. + migrateLegacyCrashDump(); + pruneCrashDumps(); + //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. @@ -114,18 +121,86 @@ void QetLogger::installCrashHandler() /** @brief QetLogger::crashDumpDir - @return the directory holding crash dumps, created if missing. + @return the directory holding crash dumps. A directory rather than a single file, because dumps are per-run and - several can be waiting at once. + several can be waiting at once. Creates nothing: a getter that made a + directory as a side effect surprised a reviewer on #905, and the + readers here (listing, pruning) have no business creating it. */ QString QetLogger::crashDumpDir() const { - const QString dir = m_log_dir % QStringLiteral("/crashes"); + return m_log_dir % QStringLiteral("/crashes"); +} + +/** + @brief QetLogger::ensureCrashDumpDir + @return crashDumpDir(), created if missing. + + For the callers that are about to write into it. +*/ +QString QetLogger::ensureCrashDumpDir() const +{ + const QString dir = crashDumpDir(); QDir().mkpath(dir); return dir; } +/** + @brief QetLogger::migrateLegacyCrashDump + + Before #905 the handler wrote to a single m_log_dir/crash_dump.log. + After upgrading, nothing looks at that path any more: the dump of the + crash that quite possibly prompted the upgrade would sit there unseen + and undeleted forever. Move it into crashes/ under a name the + crash_*.log filter matches, so it is offered exactly once like any + other. Named from its own mtime, so it sorts by when it was written + rather than when it was moved. +*/ +void QetLogger::migrateLegacyCrashDump() const +{ + const QFileInfo legacy(m_log_dir % QStringLiteral("/crash_dump.log")); + if (!legacy.exists() || !legacy.isFile() || legacy.size() <= 0) { + return; + } + + const QString target = ensureCrashDumpDir() + % QStringLiteral("/crash_") + % legacy.lastModified().toString(QStringLiteral("yyyyMMdd-hhmmss")) + % QStringLiteral("_legacy.log"); + + if (QFile::exists(target)) { + //Migrated already by an earlier run of this version. + QFile::remove(legacy.absoluteFilePath()); + return; + } + QFile::rename(legacy.absoluteFilePath(), target); +} + +/** + @brief QetLogger::pruneCrashDumps + + A crash that repeats on startup would otherwise write one dump per + attempt without limit, since nothing is deleted until a dialog is + actually shown and answered. Keep the newest kMaxPendingCrashDumps -- + enough to see a pattern, bounded however long the loop runs. +*/ +void QetLogger::pruneCrashDumps() const +{ + QDir dir(crashDumpDir()); + if (!dir.exists()) { + return; + } + dir.setNameFilters({QStringLiteral("crash_*.log")}); + dir.setFilter(QDir::Files); + dir.setSorting(QDir::Time); + + const QFileInfoList entries = dir.entryInfoList(); + for (int i = kMaxPendingCrashDumps ; i < entries.size() ; ++i) { + QFile::remove(entries.at(i).absoluteFilePath()); + } +} + /** @brief QetLogger::buildCrashDumpPath @return where this run would write a crash dump. @@ -143,7 +218,7 @@ QString QetLogger::crashDumpDir() const */ QString QetLogger::buildCrashDumpPath() const { - return crashDumpDir() + return ensureCrashDumpDir() % QStringLiteral("/crash_") % QDateTime::currentDateTime().toString(QStringLiteral("yyyyMMdd-hhmmss")) % QStringLiteral("_") @@ -153,15 +228,23 @@ QString QetLogger::buildCrashDumpPath() const /** @brief QetLogger::pendingCrashDumpFiles - @return dumps left by previous runs, newest first. + @return dumps left by previous runs, newest first, at most + kMaxPendingCrashDumps of them. 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. + + Callers take this list once and pass it on to + pendingCrashDumpContents() and clearPendingCrashDump(), rather than + each of those re-reading the directory. See clearPendingCrashDump(). */ QStringList QetLogger::pendingCrashDumpFiles() const { QDir dir(crashDumpDir()); + if (!dir.exists()) { + return QStringList(); + } dir.setNameFilters({QStringLiteral("crash_*.log")}); dir.setFilter(QDir::Files); dir.setSorting(QDir::Time); @@ -178,6 +261,9 @@ QStringList QetLogger::pendingCrashDumpFiles() const continue; } files << info.absoluteFilePath(); + if (files.size() >= kMaxPendingCrashDumps) { + break; + } } return files; } @@ -437,7 +523,8 @@ bool QetLogger::hasPendingCrashDump() const /** @brief QetLogger::pendingCrashDumpContents - @return every pending dump, newest first, concatenated. + @param files the list from pendingCrashDumpFiles() + @return those dumps, in the order given, 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 @@ -445,9 +532,8 @@ bool QetLogger::hasPendingCrashDump() const 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 +QByteArray QetLogger::pendingCrashDumpContents(const QStringList &files) const { - const QStringList files = pendingCrashDumpFiles(); if (files.isEmpty()) { return QByteArray(); } @@ -477,14 +563,19 @@ QByteArray QetLogger::pendingCrashDumpContents() const /** @brief QetLogger::clearPendingCrashDump - Drop the dumps that have just been offered. + @param files exactly the dumps that were offered - Only those: a dump written by a run that crashed after this list was - taken would otherwise be deleted without ever being seen. + Deletes the list it is given rather than re-reading the directory. + The offer sits inside a modal dialog that can stay open for as long + as the user cares to read it, and dumps are per-run: a second + QElectroTech -- SingleApplication keys its socket on the binary path, + so a different build is a separate instance -- can crash and write a + new dump while that dialog is up. Re-listing at this point would + delete that fresh dump without anyone ever having seen it, which is + the failure this whole change is about. */ -void QetLogger::clearPendingCrashDump() +void QetLogger::clearPendingCrashDump(const QStringList &files) { - const QStringList files = pendingCrashDumpFiles(); for (const QString &path : files) { QFile::remove(path); } @@ -531,11 +622,30 @@ QByteArray QetLogger::buildDiagnosticsReport() const */ QByteArray QetLogger::redact(const QByteArray &input) { - const QByteArray home = QDir::homePath().toUtf8(); - if (home.isEmpty()) { - return input; - } QByteArray out = input; - out.replace(home, QByteArrayLiteral("~")); + + const QByteArray home = QDir::homePath().toUtf8(); + if (!home.isEmpty()) { + out.replace(home, QByteArrayLiteral("~")); + } + + //backtrace_symbols_fd() writes the absolute path of each module, + //which for an AppImage is the per-run mount point + ///tmp/.mount_QElectXXXXXX. Not identifying on its own, but it is + //noise in a bug report and it is a path the user never typed, so + //fold it to a stable name. Done after the home replacement above + //because the mount point is not under $HOME. + const QByteArray mount_prefix("/tmp/.mount_"); + int at = out.indexOf(mount_prefix); + while (at >= 0) + { + int end = at + mount_prefix.size(); + while (end < out.size() && out.at(end) != '/' && !isspace(static_cast(out.at(end)))) { + ++end; + } + out.replace(at, end - at, QByteArrayLiteral("")); + at = out.indexOf(mount_prefix, at + 10); + } + return out; } diff --git a/sources/logging/qetlogger.h b/sources/logging/qetlogger.h index a72c9a0e6..cdc859ee9 100644 --- a/sources/logging/qetlogger.h +++ b/sources/logging/qetlogger.h @@ -76,6 +76,7 @@ class QetLogger static constexpr qint64 kMaxFileBytes = 2 * 1024 * 1024; // 2 MiB per file static constexpr int kRotationKeep = 4; // .1.log .. .4.log static constexpr int kMaxMessageBytes = 4096; // per-message truncation + static constexpr int kMaxPendingCrashDumps = 10; // newest kept, rest pruned static QetLogger &instance(); @@ -106,15 +107,24 @@ class QetLogger /// dump behind. bool hasPendingCrashDump() const; - /// Raw contents of the pending crash dump, or an empty array if - /// there isn't one. Does not delete it -- call - /// clearPendingCrashDump() once it has been offered to the user. - QByteArray pendingCrashDumpContents() const; + /// Dumps left by previous runs, newest first, capped at + /// kMaxPendingCrashDumps. This run's own dump path is never + /// included. Take this list once and pass the same list to + /// pendingCrashDumpContents() and clearPendingCrashDump(): that + /// is what makes "only what was offered gets deleted" true, + /// rather than re-reading the directory at each step and + /// deleting a dump that arrived in between unseen. + QStringList pendingCrashDumpFiles() const; - /// Deletes the pending crash dump file. Call after the user has - /// been offered it (whether they chose to save it or not) so it - /// is never offered a second time. - void clearPendingCrashDump(); + /// Raw contents of `files`, newest first, concatenated and + /// redacted. Deletes nothing -- pass the same list to + /// clearPendingCrashDump() once it has been offered. + QByteArray pendingCrashDumpContents(const QStringList &files) const; + + /// Deletes exactly `files`, nothing else. Call after the user + /// has been offered them (whether they chose to save them or + /// not) so they are never offered a second time. + void clearPendingCrashDump(const QStringList &files); /// Builds a redacted diagnostics bundle from the *current* session /// (header + this session's log file so far) for the manual @@ -122,10 +132,13 @@ class QetLogger /// which is about a *previous*, already-terminated session. QByteArray buildDiagnosticsReport() const; - /// Replaces occurrences of the user's home directory with "~". - /// Applied to both the crash dump and buildDiagnosticsReport() - /// before they are ever shown to the user, since both are - /// destined for a public bug tracker. + /// Replaces occurrences of the user's home directory with "~", + /// and an AppImage's per-run /tmp/.mount_XXXXXX prefix with + /// "" -- the latter because backtrace_symbols_fd() + /// writes absolute module paths into the dump. Applied to both + /// the crash dump and buildDiagnosticsReport() before they are + /// ever shown to the user, since both are destined for a public + /// bug tracker. static QByteArray redact(const QByteArray &input); private: @@ -136,9 +149,18 @@ class QetLogger void rotateLocked(); void writeToFile(const QByteArray &line, QtMsgType type); QString rotatedPath(int index) const; + /// Pure path getter: creates nothing. Callers that are about + /// to write there call ensureCrashDumpDir() instead. QString crashDumpDir() const; + QString ensureCrashDumpDir() const; QString buildCrashDumpPath() const; - QStringList pendingCrashDumpFiles() const; + /// Moves a crash_dump.log left by a pre-#905 version into + /// crashes/, so upgrading does not strand it unoffered. + void migrateLegacyCrashDump() const; + /// Keeps the newest kMaxPendingCrashDumps dumps and deletes + /// the rest, so a crash loop cannot fill the log directory + /// before anyone gets the chance to see a dialog. + void pruneCrashDumps() const; QString currentLogFilePath() const; static QByteArray sanitize(const QByteArray &input); diff --git a/sources/qetapp.cpp b/sources/qetapp.cpp index d8e625d4d..a505155e8 100644 --- a/sources/qetapp.cpp +++ b/sources/qetapp.cpp @@ -2728,11 +2728,16 @@ void QETApp::checkBackupFiles() void QETApp::checkCrashDump() { QetLogger &logger = QetLogger::instance(); - if (!logger.hasPendingCrashDump()) { + + // Listed once, then used both to build the contents and to delete + // below. Re-listing after the dialog closes would delete a dump + // written while it was open, unseen -- see clearPendingCrashDump(). + const QStringList offered = logger.pendingCrashDumpFiles(); + if (offered.isEmpty()) { return; } - const QByteArray content = logger.pendingCrashDumpContents(); + const QByteArray content = logger.pendingCrashDumpContents(offered); DiagnosticsReportDialog dialog( tr("Rapport de plantage"), @@ -2744,7 +2749,7 @@ void QETApp::checkCrashDump() // Offered once, then marked retrieved -- regardless of whether the // user chose to save it -- so it is never offered a second time. - logger.clearPendingCrashDump(); + logger.clearPendingCrashDump(offered); } /** diff --git a/tests/qttest/CMakeLists.txt b/tests/qttest/CMakeLists.txt index 855a9acd2..21e5a1a55 100644 --- a/tests/qttest/CMakeLists.txt +++ b/tests/qttest/CMakeLists.txt @@ -48,6 +48,18 @@ find_package( Test REQUIRED) +# tst_crashhandler compiles crashhandler.cpp, which on a non-Windows build +# may pull in backtrace(). Resolved the same way as in the top-level +# CMakeLists so this directory also configures standalone -- on FreeBSD the +# symbol lives in libexecinfo, not libc. +if(NOT DEFINED Backtrace_FOUND) + find_package(Backtrace QUIET) + if(Backtrace_FOUND) + include_directories(${Backtrace_INCLUDE_DIRS}) + add_compile_definitions(QET_CRASH_BACKTRACE) + endif() +endif() + include(../../cmake/fetch_kdeaddons.cmake) include(../../cmake/fetch_singleapplication.cmake) include(../../cmake/fetch_pugixml.cmake) @@ -110,6 +122,41 @@ add_test(NAME tst_qetstrings COMMAND tst_qetstrings) target_include_directories(tst_qetstrings PRIVATE ${QET_DIR}/sources) target_link_libraries(tst_qetstrings PRIVATE Qt::Test Qt::Widgets Qt::Xml) +# CrashHandler::formatInt() -- the async-signal-safe decimal formatter the +# signal handler uses for the "Signal: N" line of a crash dump. Compiles +# crashhandler.cpp and logring.cpp alongside; the handler deliberately +# depends on nothing heavier than QByteArray. +add_executable( + tst_crashhandler + tst_crashhandler.cpp + ${QET_DIR}/sources/logging/crashhandler.cpp + ${QET_DIR}/sources/logging/logring.cpp + ${QET_DIR}/sources/qetversion.cpp) +add_test(NAME tst_crashhandler COMMAND tst_crashhandler) +target_include_directories(tst_crashhandler PRIVATE ${QET_DIR}/sources) +# Qt::Xml because crashhandler.cpp includes qetversion.h for the header it +# builds at install() time, and that pulls in QDomElement. +target_link_libraries(tst_crashhandler PRIVATE Qt::Test Qt::Xml ${Backtrace_LIBRARIES}) + +# The crash-dump bookkeeping from #905: which dumps get listed, offered and +# deleted, and what redact() masks. qetlogger.cpp needs exactly one symbol +# from the application (QETApp::dataDir()), which the test supplies itself, +# so this links the logging sources rather than the whole program. +add_executable( + tst_crashdumps + tst_crashdumps.cpp + ${QET_DIR}/sources/logging/qetlogger.cpp + ${QET_DIR}/sources/logging/crashhandler.cpp + ${QET_DIR}/sources/logging/logring.cpp + ${QET_DIR}/sources/qetversion.cpp) +add_test(NAME tst_crashdumps COMMAND tst_crashdumps) +target_include_directories(tst_crashdumps PRIVATE + ${QET_DIR} + ${QET_DIR}/sources + ${QET_DIR}/sources/NameList + ${QET_DIR}/pugixml/src) +target_link_libraries(tst_crashdumps PRIVATE Qt::Test Qt::Widgets Qt::Xml ${Backtrace_LIBRARIES}) + add_executable( tst_menubarkeyboard tst_menubarkeyboard.cpp) diff --git a/tests/qttest/tst_crashdumps.cpp b/tests/qttest/tst_crashdumps.cpp new file mode 100644 index 000000000..c6823bd86 --- /dev/null +++ b/tests/qttest/tst_crashdumps.cpp @@ -0,0 +1,312 @@ +/* + Copyright 2006-2026 The QElectroTech Team + This file is part of QElectroTech. + + QElectroTech is free software: you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation, either version 2 of the License, or + (at your option) any later version. + + QElectroTech is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with QElectroTech. If not, see . +*/ + +#include "logging/qetlogger.h" + +#include "qetapp.h" + +#include +#include +#include +#include + +/** + QetLogger::init() asks QETApp for the data directory, and that is the + only thing it needs from the application. Standing in for it here is + what lets the crash-dump bookkeeping be tested without linking (or + starting) the whole of QElectroTech. +*/ +namespace { + QString g_data_dir; +} + +QString QETApp::dataDir() +{ + return g_data_dir; +} + +/** + @brief The tst_CrashDumps class + + Covers the crash-dump bookkeeping added in #905: one file per run + instead of a single crash_dump.log that each crash overwrote (#898), + and the rules about which of those files get offered and deleted. +*/ +class tst_CrashDumps : public QObject +{ + Q_OBJECT + + private slots: + void init(); + + void listsNothingWhenThereHasBeenNoCrash(); + void listsDumpsNewestFirst(); + void skipsEmptyDumps(); + void excludesThisRunsOwnDump(); + void capsTheListAtTenDumps(); + void concatenatesEveryOfferedDump(); + void clearsOnlyWhatWasOffered(); + + void redactsTheHomeDirectory(); + void redactsAnAppImageMountPoint(); + void redactsBothInOneString(); + + private: + QTemporaryDir m_dir; + QString crashesDir() const {return g_data_dir + QStringLiteral("/crashes");} + void writeDump(const QString &name, const QByteArray &body, + const QDateTime &when = QDateTime()); +}; + +void tst_CrashDumps::init() +{ + QVERIFY(m_dir.isValid()); + // A fresh subdirectory per test: QetLogger is a singleton, so the + // tests share one instance and must not share its files. + static int n = 0; + g_data_dir = m_dir.path() + QStringLiteral("/run") + QString::number(++n); + QVERIFY(QDir().mkpath(g_data_dir)); + QVERIFY(QDir().mkpath(crashesDir())); +} + +void tst_CrashDumps::writeDump(const QString &name, const QByteArray &body, + const QDateTime &when) +{ + const QString path = crashesDir() + QStringLiteral("/") + name; + { + QFile f(path); + QVERIFY(f.open(QIODevice::WriteOnly)); + f.write(body); + f.close(); + } + if (!when.isValid()) { + return; + } + // Separately, and only once the write is closed: setFileTime() needs + // an open handle, but closing a file that was just written sets the + // modification time to now, which would undo it. + QFile stamp(path); + QVERIFY(stamp.open(QIODevice::ReadWrite)); + QVERIFY(stamp.setFileTime(when, QFileDevice::FileModificationTime)); + stamp.close(); +} + +void tst_CrashDumps::listsNothingWhenThereHasBeenNoCrash() +{ + QetLogger::instance().init(); + QVERIFY(QetLogger::instance().pendingCrashDumpFiles().isEmpty()); + QVERIFY(!QetLogger::instance().hasPendingCrashDump()); +} + +void tst_CrashDumps::listsDumpsNewestFirst() +{ + const QDateTime base = QDateTime::currentDateTime(); + writeDump(QStringLiteral("crash_a.log"), "oldest", base.addSecs(-300)); + writeDump(QStringLiteral("crash_b.log"), "middle", base.addSecs(-200)); + writeDump(QStringLiteral("crash_c.log"), "newest", base.addSecs(-100)); + + QetLogger::instance().init(); + const QStringList files = QetLogger::instance().pendingCrashDumpFiles(); + + QCOMPARE(files.size(), 3); + QVERIFY(files.at(0).endsWith(QStringLiteral("crash_c.log"))); + QVERIFY(files.at(1).endsWith(QStringLiteral("crash_b.log"))); + QVERIFY(files.at(2).endsWith(QStringLiteral("crash_a.log"))); +} + +/** + A zero-length dump means the handler opened the file and died before + writing anything. There is nothing to show, and offering an empty + report would be worse than offering none. +*/ +void tst_CrashDumps::skipsEmptyDumps() +{ + writeDump(QStringLiteral("crash_empty.log"), QByteArray()); + writeDump(QStringLiteral("crash_real.log"), "something"); + + QetLogger::instance().init(); + const QStringList files = QetLogger::instance().pendingCrashDumpFiles(); + + QCOMPARE(files.size(), 1); + QVERIFY(files.at(0).endsWith(QStringLiteral("crash_real.log"))); +} + +/** + The dump this run would write if it crashed must never appear in the + list of dumps from *previous* runs -- otherwise a process that crashed + could be offered its own dump, mid-crash. +*/ +void tst_CrashDumps::excludesThisRunsOwnDump() +{ + writeDump(QStringLiteral("crash_previous.log"), "from a previous run"); + + QetLogger &logger = QetLogger::instance(); + logger.init(); + logger.installCrashHandler(); // this is what fixes our own path + + // installCrashHandler() picked crash__.log for + // this process but has not created it -- the handler only writes when + // the process actually dies. Standing in for that here: fill in every + // name it could have chosen, so whichever one it picked now exists + // and is non-empty. The exact second does not have to be guessed. + const qint64 pid = QCoreApplication::applicationPid(); + const QDateTime now = QDateTime::currentDateTime(); + for (int offset = -2 ; offset <= 0 ; ++offset) { + writeDump(QStringLiteral("crash_%1_%2.log") + .arg(now.addSecs(offset).toString(QStringLiteral("yyyyMMdd-hhmmss"))) + .arg(pid), + "this run's own dump, mid-crash"); + } + + // Exactly one of those three is the path install() actually chose, + // and exactly that one must be missing from the list. The other two + // are ordinary files as far as the logger is concerned. + const QDir dir(crashesDir()); + const int on_disk = dir.entryList({QStringLiteral("crash_*_") + QString::number(pid) + + QStringLiteral(".log")}, + QDir::Files).size(); + QCOMPARE(on_disk, 3); + + const QStringList files = logger.pendingCrashDumpFiles(); + int offered_own = 0; + for (const QString &path : files) { + if (path.contains(QString::number(pid))) { + ++offered_own; + } + } + QCOMPARE(offered_own, 2); // one of the three was excluded + QCOMPARE(files.size(), 3); // those two, plus crash_previous.log + QVERIFY(files.last().endsWith(QStringLiteral("crash_previous.log"))); +} + +/** + A crash loop writes one dump per restart. The list is capped so the + dialog cannot be handed an unbounded amount of text. +*/ +void tst_CrashDumps::capsTheListAtTenDumps() +{ + const QDateTime base = QDateTime::currentDateTime(); + for (int i = 0 ; i < 15 ; ++i) { + writeDump(QStringLiteral("crash_%1.log").arg(i, 2, 10, QChar('0')), + QByteArray("dump ") + QByteArray::number(i), + base.addSecs(-1000 + i)); + } + + QetLogger::instance().init(); + QCOMPARE(QetLogger::instance().pendingCrashDumpFiles().size(), 10); + + // The ten kept are the newest ten, i.e. 05..14. + const QStringList files = QetLogger::instance().pendingCrashDumpFiles(); + QVERIFY(files.at(0).endsWith(QStringLiteral("crash_14.log"))); + QVERIFY(files.at(9).endsWith(QStringLiteral("crash_05.log"))); +} + +/** + #898: every dump is offered, not just the most recent one. The whole + point of keeping them is that a repeating crash is where the earlier + dumps carry the most information. +*/ +void tst_CrashDumps::concatenatesEveryOfferedDump() +{ + const QDateTime base = QDateTime::currentDateTime(); + writeDump(QStringLiteral("crash_one.log"), "FIRST CRASH BODY", base.addSecs(-200)); + writeDump(QStringLiteral("crash_two.log"), "SECOND CRASH BODY", base.addSecs(-100)); + + QetLogger &logger = QetLogger::instance(); + logger.init(); + + const QStringList offered = logger.pendingCrashDumpFiles(); + const QByteArray content = logger.pendingCrashDumpContents(offered); + + QVERIFY(content.contains("FIRST CRASH BODY")); + QVERIFY(content.contains("SECOND CRASH BODY")); + QVERIFY(content.contains("crash_one.log")); + QVERIFY(content.contains("crash_two.log")); + QVERIFY(content.contains("2 crash dumps pending")); +} + +/** + The reason clearPendingCrashDump() takes the list rather than looking + the directory up again: the offer sits in a modal dialog, and a dump + that arrives while it is open has never been seen by anybody. +*/ +void tst_CrashDumps::clearsOnlyWhatWasOffered() +{ + writeDump(QStringLiteral("crash_offered.log"), "offered to the user"); + + QetLogger &logger = QetLogger::instance(); + logger.init(); + + const QStringList offered = logger.pendingCrashDumpFiles(); + QCOMPARE(offered.size(), 1); + + // ... the dialog is open, and a second instance crashes. + writeDump(QStringLiteral("crash_arrived_later.log"), "nobody has seen this yet"); + + logger.clearPendingCrashDump(offered); + + const QStringList left = logger.pendingCrashDumpFiles(); + QCOMPARE(left.size(), 1); + QVERIFY(left.at(0).endsWith(QStringLiteral("crash_arrived_later.log"))); +} + +void tst_CrashDumps::redactsTheHomeDirectory() +{ + const QByteArray home = QDir::homePath().toUtf8(); + QVERIFY(!home.isEmpty()); + + const QByteArray in = home + "/projects/secret.qet failed to load"; + const QByteArray out = QetLogger::redact(in); + + QVERIFY(!out.contains(home)); + QVERIFY(out.startsWith("~/projects/secret.qet")); +} + +/** + backtrace_symbols_fd() writes absolute module paths, which for an + AppImage is a per-run mount point under /tmp/.mount_. Raised in review + on #905. +*/ +void tst_CrashDumps::redactsAnAppImageMountPoint() +{ + const QByteArray in = + "/tmp/.mount_QElect6Yh2Kz/usr/bin/qelectrotech(+0x9b098e) [0x5ecf]\n" + "/tmp/.mount_QElect6Yh2Kz/usr/lib/libQt6Core.so.6(+0x1234) [0x7cfa]\n"; + const QByteArray out = QetLogger::redact(in); + + QVERIFY(!out.contains(".mount_QElect6Yh2Kz")); + QVERIFY(out.contains("/usr/bin/qelectrotech")); + QVERIFY(out.contains("/usr/lib/libQt6Core.so.6")); + // The frame offsets are the useful part and must survive. + QVERIFY(out.contains("(+0x9b098e) [0x5ecf]")); +} + +void tst_CrashDumps::redactsBothInOneString() +{ + const QByteArray home = QDir::homePath().toUtf8(); + const QByteArray in = home + "/Documents/a.qet\n/tmp/.mount_AbCdEf/usr/bin/qet\n"; + const QByteArray out = QetLogger::redact(in); + + QVERIFY(!out.contains(home)); + QVERIFY(!out.contains(".mount_AbCdEf")); + QVERIFY(out.contains("~/Documents/a.qet")); + QVERIFY(out.contains("/usr/bin/qet")); +} + +QTEST_MAIN(tst_CrashDumps) +#include "tst_crashdumps.moc" diff --git a/tests/qttest/tst_crashhandler.cpp b/tests/qttest/tst_crashhandler.cpp new file mode 100644 index 000000000..704e167cd --- /dev/null +++ b/tests/qttest/tst_crashhandler.cpp @@ -0,0 +1,149 @@ +/* + Copyright 2006-2026 The QElectroTech Team + This file is part of QElectroTech. + + QElectroTech is free software: you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation, either version 2 of the License, or + (at your option) any later version. + + QElectroTech is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with QElectroTech. If not, see . +*/ + +#include "logging/crashhandler.h" + +#include +#include +#include +#include + +/** + @brief The tst_CrashHandler class + + Covers CrashHandler::formatInt(), the decimal formatter the signal + handler uses to write "Signal: 11" into a crash dump. + + It is worth a test out of proportion to its size. It cannot use + snprintf(), which is not on the POSIX async-signal-safe list, so it is + hand-rolled; it runs only inside a signal handler, where nothing can + assert and a fault produces no diagnostic; and it is exercised only + when the application is already crashing, so a defect here would + corrupt or truncate exactly the dumps that matter and would never be + noticed in ordinary use. + + The negation is the interesting part: -value on INT_MIN is undefined + behaviour, so the implementation goes through unsigned. +*/ +class tst_CrashHandler : public QObject +{ + Q_OBJECT + + private slots: + void formatsZero(); + void formatsPositive(); + void formatsTheHandledSignals(); + void formatsNegative(); + void formatsIntMinWithoutOverflow(); + void formatsIntMax(); + void truncatesRatherThanOverflowing(); + void writesNothingWhenThereIsNoRoom(); + + private: + /// Formats into a buffer poisoned with a sentinel, and fails if + /// anything past the returned length was touched. + static QByteArray format(int value, int size = 32); +}; + +QByteArray tst_CrashHandler::format(int value, int size) +{ + char buffer[64]; + memset(buffer, '\xAB', sizeof(buffer)); + + const int len = CrashHandler::formatInt(buffer, size, value); + + // Nothing may be written past what was reported, nor past `size`. + for (int i = qMax(len, 0) ; i < static_cast(sizeof(buffer)) ; ++i) { + if (buffer[i] != '\xAB') { + return QByteArray("WROTE PAST END at ") + QByteArray::number(i); + } + } + if (len < 0 || len > size) { + return QByteArray("BAD LENGTH ") + QByteArray::number(len); + } + return QByteArray(buffer, len); +} + +void tst_CrashHandler::formatsZero() +{ + QCOMPARE(format(0), QByteArray("0")); +} + +void tst_CrashHandler::formatsPositive() +{ + QCOMPARE(format(1), QByteArray("1")); + QCOMPARE(format(9), QByteArray("9")); + QCOMPARE(format(10), QByteArray("10")); + QCOMPARE(format(1234567), QByteArray("1234567")); +} + +/** + The values this actually sees in the field: kHandledSignals, as + written into the "Signal: N" line of every dump. +*/ +void tst_CrashHandler::formatsTheHandledSignals() +{ + QCOMPARE(format(SIGSEGV), QByteArray::number(SIGSEGV)); + QCOMPARE(format(SIGABRT), QByteArray::number(SIGABRT)); + QCOMPARE(format(SIGBUS), QByteArray::number(SIGBUS)); + QCOMPARE(format(SIGFPE), QByteArray::number(SIGFPE)); + QCOMPARE(format(SIGILL), QByteArray::number(SIGILL)); +} + +void tst_CrashHandler::formatsNegative() +{ + QCOMPARE(format(-1), QByteArray("-1")); + QCOMPARE(format(-42), QByteArray("-42")); +} + +/** + -INT_MIN is undefined behaviour; the implementation negates through + unsigned instead. A build that got this wrong would either trap under + -ftrapv/UBSan or silently print the wrong number. +*/ +void tst_CrashHandler::formatsIntMinWithoutOverflow() +{ + QCOMPARE(format(INT_MIN), QByteArray::number(INT_MIN)); +} + +void tst_CrashHandler::formatsIntMax() +{ + QCOMPARE(format(INT_MAX), QByteArray::number(INT_MAX)); +} + +/** + A buffer too small must be filled and stopped at, never run past -- + the handler passes a fixed 64-byte stack buffer and subtracts what it + has already used. +*/ +void tst_CrashHandler::truncatesRatherThanOverflowing() +{ + QCOMPARE(format(12345, 3), QByteArray("123")); + QCOMPARE(format(-12345, 3), QByteArray("-12")); + QCOMPARE(format(7, 1), QByteArray("7")); +} + +void tst_CrashHandler::writesNothingWhenThereIsNoRoom() +{ + QCOMPARE(format(123, 0), QByteArray()); + QCOMPARE(format(0, 0), QByteArray()); + QCOMPARE(format(-5, 0), QByteArray()); +} + +QTEST_APPLESS_MAIN(tst_CrashHandler) +#include "tst_crashhandler.moc"