mirror of
https://github.com/qelectrotech/qelectrotech-source-mirror.git
synced 2026-09-26 20:04:12 +02:00
ispyisail
f8c1b5206a
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(<execinfo.h>). 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_<timestamp>_<pid> 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 <noreply@anthropic.com>
103 lines
4.6 KiB
C++
103 lines
4.6 KiB
C++
/*
|
|
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 <http://www.gnu.org/licenses/>.
|
|
*/
|
|
#ifndef CRASHHANDLER_H
|
|
#define CRASHHANDLER_H
|
|
|
|
#include <QString>
|
|
|
|
class LogRing;
|
|
|
|
/**
|
|
@brief The CrashHandler class
|
|
Discussion #644, step 4: on a fatal crash, flush the in-memory
|
|
LogRing to a fixed file before the process dies, so the last N log
|
|
lines leading up to the crash survive it -- today they only exist in
|
|
memory and are lost with the process.
|
|
|
|
This is the highest-risk piece of the whole logging rework (the
|
|
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 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
|
|
time (the dump path, the header, the ring's own storage) is
|
|
preallocated by install(), which runs once at startup in normal
|
|
(non-signal) context.
|
|
3. The handler must not swallow the crash. After writing the dump it
|
|
restores the default disposition for the signal and re-raises, so
|
|
the OS still produces a core dump (POSIX) / Windows Error
|
|
Reporting still sees the exception. A handler that "fixed" the
|
|
crash by not re-raising would destroy the post-mortem evidence a
|
|
core dump provides.
|
|
4. Only the *first* crash writes a dump. An atomic test-and-set
|
|
guards against two threads faulting simultaneously (or the handler
|
|
itself faulting while dumping) producing an interleaved or
|
|
truncated file; every crash after the first goes straight to
|
|
restore-and-re-raise.
|
|
|
|
Tested in this environment: POSIX/Linux only (sigaction, sigaltstack,
|
|
SIGSEGV/SIGABRT/SIGBUS/SIGFPE/SIGILL). The Windows path
|
|
(SetUnhandledExceptionFilter) and macOS-specific behaviour (signal
|
|
handling itself is POSIX and shares the Linux code path, but sandbox
|
|
profiles can affect where the dump file may be written) are
|
|
implemented per the discussion's guidance but could not be exercised
|
|
here -- there is no Windows or macOS build available in this sandbox.
|
|
Please sanity-check both before relying on them in the field.
|
|
*/
|
|
class CrashHandler
|
|
{
|
|
public:
|
|
/// Installs the crash handler. Must be called from normal
|
|
/// (non-signal) startup code, after the LogRing it will dump
|
|
/// exists, and only once. `ring` must outlive the process (in
|
|
/// practice: the LogRing owned by QetLogger's function-local
|
|
/// static instance, which is never destroyed before exit).
|
|
/// `dump_path` is resolved and copied into a fixed-size internal
|
|
/// buffer here; nothing under the actual signal/exception path
|
|
/// 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;
|
|
};
|
|
|
|
#endif // CRASHHANDLER_H
|