Merge pull request #905 from ispyisail/harden/crash-reporter

Harden the crash reporter: keep every dump, and say what crashed

sources/logging/crashhandler.cpp

@@ -36,6 +36,13 @@
 #include <csignal>
 #include <fcntl.h>
 #include <unistd.h>
+	// QET_CRASH_BACKTRACE is defined by CMake, via find_package(Backtrace),
+	// not by probing for the header here. <execinfo.h> 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 <execinfo.h>
+#endif
 #endif
 
 namespace {
@@ -64,6 +71,14 @@ char g_altstack[65536];
 
 const int kHandledSignals[] = {SIGSEGV, SIGABRT, SIGBUS, SIGFPE, SIGILL};
 
+#ifdef QET_CRASH_BACKTRACE
+// Preallocated here for the same reason as everything else in this block:
+// backtrace() fills a caller-supplied array, so it needs no heap of its
+// own, and backtrace_symbols_fd() writes straight to the fd (unlike
+// backtrace_symbols(), which mallocs and is therefore unusable here).
+void *g_backtrace_frames[64];
+#endif
+
 void restoreDefaultAndReraise(int sig)
 {
 	struct sigaction sa {};
@@ -85,16 +100,63 @@ void signalHandler(int sig)
 		return;
 	}
 
-	// open/write/close are all on the POSIX async-signal-safe function
-	// list; nothing else is called here.
+	// open/write/close, and backtrace_symbols_fd, are all on the POSIX
+	// async-signal-safe function list; nothing else is called here.
+	//
+	// 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) {
 			::write(fd, g_header, static_cast<size_t>(g_header_len));
 		}
+
+		// Which signal killed it. The header is built once at install()
+		// and is therefore identical for every crash, so without this the
+		// dump never said what actually happened -- SIGSEGV and SIGABRT
+		// point at very different bugs.
+		char line[64];
+		int len = 0;
+		const char kSignalLabel[] = "Signal: ";
+		for (unsigned i = 0 ; i < sizeof(kSignalLabel) - 1 ; ++i) {
+			line[len++] = kSignalLabel[i];
+		}
+		len += CrashHandler::formatInt(line + len, static_cast<int>(sizeof(line)) - len - 1, sig);
+		line[len++] = '\n';
+		::write(fd, line, static_cast<size_t>(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
+		// 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,
+					       static_cast<int>(sizeof(g_backtrace_frames)
+								/ sizeof(g_backtrace_frames[0])));
+		if (frames > 0) {
+			::backtrace_symbols_fd(g_backtrace_frames, frames, fd);
+		}
+#endif
+
 		::close(fd);
 	}
 
@@ -134,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<unsigned int>(-(value + 1)) + 1u
+				  : static_cast<unsigned int>(value);
+	while (v > 0 && n < static_cast<int>(sizeof(scratch))) {
+		scratch[n++] = static_cast<char>('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;
@@ -159,6 +249,15 @@ void CrashHandler::install(const LogRing *ring, const QString &dump_path)
 	ss.ss_flags = 0;
 	sigaltstack(&ss, nullptr);
 
+#ifdef QET_CRASH_BACKTRACE
+	// Warm the unwinder. backtrace()'s *first* call resolves dynamic
+	// linker state and may allocate; every call after that does not. Doing
+	// it here, in normal context, is what lets the handler call it without
+	// breaking invariant 2. The result is deliberately discarded.
+	void *warmup[4];
+	(void) ::backtrace(warmup, 4);
+#endif
+
 	struct sigaction sa {};
 	sa.sa_handler = signalHandler;
 	sigemptyset(&sa.sa_mask);

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;
 };

sources/logging/qetlogger.cpp

@@ -21,10 +21,12 @@
 #include "../qetapp.h"
 #include "../qetversion.h"
 
+#include <QCoreApplication>
 #include <QDateTime>
 #include <QDir>
 #include <QFileInfo>
 #include <QSysInfo>
+#include <cctype>
 #include <cstdio>
 
 namespace {
@@ -104,12 +106,166 @@ void QetLogger::installCrashHandler()
 	if (m_disabled) {
 		return;
 	}
-	CrashHandler::install(&m_ring, crashDumpPath());
+		//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.
+	m_crash_dump_path = buildCrashDumpPath();
+	CrashHandler::install(&m_ring, m_crash_dump_path);
 }
 
-QString QetLogger::crashDumpPath() const
+/**
+	@brief QetLogger::crashDumpDir
+	@return the directory holding crash dumps.
+
+	A directory rather than a single file, because dumps are per-run and
+	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
 {
-	return m_log_dir % QStringLiteral("/crash_dump.log");
+	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.
+
+	One file per run, rather than a single fixed crash_dump.log. That old
+	scheme opened one path with O_TRUNC, so a second crash overwrote the
+	first: someone who crashed ten times still ended up with exactly one
+	dump, the most recent. Reported on #898 -- "the report appeared only
+	once despite there being 10 or more crashes" -- where losing the
+	earlier dumps mattered as much as never being shown them.
+
+	Built here in normal context and handed to CrashHandler::install(),
+	which copies it into a preallocated buffer, so the handler still
+	writes to one fixed path and its no-allocation invariant is untouched.
+*/
+QString QetLogger::buildCrashDumpPath() const
+{
+	return ensureCrashDumpDir()
+		% QStringLiteral("/crash_")
+		% QDateTime::currentDateTime().toString(QStringLiteral("yyyyMMdd-hhmmss"))
+		% QStringLiteral("_")
+		% QString::number(QCoreApplication::applicationPid())
+		% QStringLiteral(".log");
+}
+
+/**
+	@brief QetLogger::pendingCrashDumpFiles
+	@return dumps left by previous runs, newest first, 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);
+
+	QStringList files;
+	const QFileInfoList entries = dir.entryInfoList();
+	for (const QFileInfo &info : entries)
+	{
+		if (info.size() <= 0) {
+			continue;
+		}
+		if (!m_crash_dump_path.isEmpty()
+				&& info.absoluteFilePath() == QFileInfo(m_crash_dump_path).absoluteFilePath()) {
+			continue;
+		}
+		files << info.absoluteFilePath();
+		if (files.size() >= kMaxPendingCrashDumps) {
+			break;
+		}
+	}
+	return files;
 }
 
 QString QetLogger::currentLogFilePath() const
@@ -362,22 +518,67 @@ bool QetLogger::hasPendingCrashDump() const
 	if (m_disabled) {
 		return false;
 	}
-	const QFileInfo info(crashDumpPath());
-	return info.exists() && info.isFile() && info.size() > 0;
+	return !pendingCrashDumpFiles().isEmpty();
 }
 
-QByteArray QetLogger::pendingCrashDumpContents() const
+/**
+	@brief QetLogger::pendingCrashDumpContents
+	@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
+	difference between them is the evidence. They are separated by a
+	banner so a reader can tell where one ends and the next begins, and
+	the whole thing is redacted as a single pass.
+*/
+QByteArray QetLogger::pendingCrashDumpContents(const QStringList &files) const
 {
-	QFile file(crashDumpPath());
-	if (!file.open(QIODevice::ReadOnly)) {
+	if (files.isEmpty()) {
 		return QByteArray();
 	}
-	return redact(file.readAll());
+
+	QByteArray all;
+	if (files.size() > 1) {
+		all += QByteArray("QET: ") + QByteArray::number(files.size())
+			+ " crash dumps pending, newest first.\n\n";
+	}
+
+	for (const QString &path : files)
+	{
+		QFile file(path);
+		if (!file.open(QIODevice::ReadOnly)) {
+			continue;
+		}
+		all += "===== " + QFileInfo(path).fileName().toUtf8() + " =====\n";
+		all += file.readAll();
+		if (!all.endsWith('\n')) {
+			all += '\n';
+		}
+		all += '\n';
+	}
+
+	return redact(all);
 }
 
-void QetLogger::clearPendingCrashDump()
+/**
+	@brief QetLogger::clearPendingCrashDump
+	@param files exactly the dumps that were offered
+
+	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(const QStringList &files)
 {
-	QFile::remove(crashDumpPath());
+	for (const QString &path : files) {
+		QFile::remove(path);
+	}
 }
 
 QByteArray QetLogger::buildDiagnosticsReport() const
@@ -421,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<unsigned char>(out.at(end)))) {
+			++end;
+		}
+		out.replace(at, end - at, QByteArrayLiteral("<appimage>"));
+		at = out.indexOf(mount_prefix, at + 10);
+	}
+
 	return out;
 }

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
+		/// "<appimage>" -- 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,7 +149,18 @@ class QetLogger
 		void rotateLocked();
 		void writeToFile(const QByteArray &line, QtMsgType type);
 		QString rotatedPath(int index) const;
-		QString crashDumpPath() 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;
+			/// 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);
@@ -147,6 +171,10 @@ class QetLogger
 
 		QString m_log_dir;
 		QString m_base_name; // e.g. "20260803", resolved once in init()
+			/// This run's own dump path, fixed at installCrashHandler():
+			/// the handler writes here, and it is excluded when collecting
+			/// dumps left by previous runs.
+		QString m_crash_dump_path;
 
 		QMutex m_file_mutex;
 		QFile m_file;

sources/qetapp.cpp

@@ -2759,11 +2759,16 @@ void QETApp::offerBackupFiles(const QList<KAutoSaveFile *> &stale_files)
 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"),
@@ -2775,7 +2780,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);
 }
 
 /**