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"