From: Manuel Nickschas Date: Fri, 5 Oct 2018 21:07:04 +0000 (+0200) Subject: common: Don't use unsafe functions when handling POSIX signals X-Git-Tag: 0.13-rc2~5 X-Git-Url: https://git.quassel-irc.org/?p=quassel.git;a=commitdiff_plain;h=c015fbf8afaf64fbb5a8e2122e2a1ecf0325b1a1 common: Don't use unsafe functions when handling POSIX signals The set of functions we're allowed to safely use in a POSIX signal handler is very limited, and certainly does not include anything Qt. While our previous implementation seemingly worked anyway as long as all it did was quitting the application, we now start seeing issues when doing a proper shutdown. Fix this by providing a generic API for watching signal-like external events that can be specialized for the various platforms we support. Provide PosixSignalWatcher, which uses socket notification to safely hand over control from the signal handler to the Qt event loop. Also provide an implementation for Windows that still uses the previous approach; since Windows doesn't support socketpair(), we can't easily reuse the POSIX implementation. On the bright side, so far we don't know if signal handlers on Windows face the same limitations as on POSIX anyway... Also on Windows we now support console events, i.e. Ctrl+C as well as closing the console window. --- diff --git a/src/common/CMakeLists.txt b/src/common/CMakeLists.txt index 72227605..32b53708 100644 --- a/src/common/CMakeLists.txt +++ b/src/common/CMakeLists.txt @@ -1,6 +1,7 @@ # Builds the common module set(SOURCES + abstractsignalwatcher.h aliasmanager.cpp authhandler.cpp backlogmanager.cpp @@ -87,13 +88,13 @@ if (APPLE) endif() if (WIN32) - set(SOURCES ${SOURCES} logbacktrace_win.cpp) + set(SOURCES ${SOURCES} logbacktrace_win.cpp windowssignalwatcher.cpp) else() if (EXECINFO_FOUND) add_definitions(-DHAVE_EXECINFO) include_directories(${EXECINFO_INCLUDES}) endif() - set(SOURCES ${SOURCES} logbacktrace_unix.cpp) + set(SOURCES ${SOURCES} logbacktrace_unix.cpp posixsignalwatcher.cpp) endif() qt_add_resources(SOURCES ${COMMON_RCS}) diff --git a/src/common/abstractsignalwatcher.h b/src/common/abstractsignalwatcher.h new file mode 100644 index 00000000..21404794 --- /dev/null +++ b/src/common/abstractsignalwatcher.h @@ -0,0 +1,51 @@ +/*************************************************************************** + * Copyright (C) 2005-2018 by the Quassel Project * + * devel@quassel-irc.org * + * * + * This program 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) version 3. * + * * + * This program 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 this program; if not, write to the * + * Free Software Foundation, Inc., * + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. * + ***************************************************************************/ + +#pragma once + +#include +#include + +/** + * Interface for watching external, asynchronous events like POSIX signals. + * + * Abstracts the platform-specific details away, since we're only interested + * in the action to take. + */ +class AbstractSignalWatcher : public QObject +{ + Q_OBJECT + +public: + enum class Action + { + Reload, ///< Configuration should be reloaded (e.g. SIGHUP) + Terminate, ///< Application should be terminated (e.g. SIGTERM) + HandleCrash ///< Application is crashing (e.g. SIGSEGV) + }; + + using QObject::QObject; + +signals: + /// An event/signal was received and the given action should be taken + void handleSignal(AbstractSignalWatcher::Action action); +}; + +Q_DECLARE_METATYPE(AbstractSignalWatcher::Action) diff --git a/src/common/posixsignalwatcher.cpp b/src/common/posixsignalwatcher.cpp new file mode 100644 index 00000000..0b678978 --- /dev/null +++ b/src/common/posixsignalwatcher.cpp @@ -0,0 +1,109 @@ +/*************************************************************************** + * Copyright (C) 2005-2018 by the Quassel Project * + * devel@quassel-irc.org * + * * + * This program 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) version 3. * + * * + * This program 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 this program; if not, write to the * + * Free Software Foundation, Inc., * + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. * + ***************************************************************************/ + +#include "posixsignalwatcher.h" + +#include +#include +#include +#include +#include + +#include +#include + +#include +#include + +#include "logmessage.h" + +int PosixSignalWatcher::_sockpair[2]; + +PosixSignalWatcher::PosixSignalWatcher(QObject *parent) + : AbstractSignalWatcher{parent} +{ + if (::socketpair(AF_UNIX, SOCK_STREAM, 0, _sockpair)) { + qWarning() << "Could not setup POSIX signal watcher:" << ::strerror(errno); + return; + } + + _notifier = new QSocketNotifier(_sockpair[1], QSocketNotifier::Read, this); + connect(_notifier, SIGNAL(activated(int)), this, SLOT(onNotify(int))); + _notifier->setEnabled(true); + + registerSignal(SIGINT); + registerSignal(SIGTERM); + registerSignal(SIGHUP); + +#ifdef HAVE_EXECINFO + // we only handle crashes ourselves if coredumps are disabled + struct rlimit *limit = (rlimit *)malloc(sizeof(struct rlimit)); + int rc = getrlimit(RLIMIT_CORE, limit); + if (rc == -1 || !((long)limit->rlim_cur > 0 || limit->rlim_cur == RLIM_INFINITY)) { + registerSignal(SIGABRT); + registerSignal(SIGSEGV); + registerSignal(SIGBUS); + } + free(limit); +#endif +} + +void PosixSignalWatcher::registerSignal(int signal) +{ + struct sigaction sigact; + sigact.sa_handler = PosixSignalWatcher::signalHandler; + sigact.sa_flags = 0; + sigemptyset(&sigact.sa_mask); + sigact.sa_flags |= SA_RESTART; + if (sigaction(signal, &sigact, nullptr)) { + qWarning() << "Could not register handler for POSIX signal:" << ::strerror(errno); + } +} + +void PosixSignalWatcher::signalHandler(int signal) +{ + auto bytes = ::write(_sockpair[0], &signal, sizeof(signal)); + Q_UNUSED(bytes) +} + +void PosixSignalWatcher::onNotify(int sockfd) +{ + int signal; + auto bytes = ::read(sockfd, &signal, sizeof(signal)); + Q_UNUSED(bytes) + quInfo() << "Caught signal" << signal; + + switch (signal) { + case SIGHUP: + emit handleSignal(Action::Reload); + break; + case SIGINT: + case SIGTERM: + emit handleSignal(Action::Terminate); + break; + case SIGABRT: + case SIGSEGV: + case SIGBUS: + emit handleSignal(Action::HandleCrash); + break; + default: + ; + } +} diff --git a/src/common/posixsignalwatcher.h b/src/common/posixsignalwatcher.h new file mode 100644 index 00000000..d96760eb --- /dev/null +++ b/src/common/posixsignalwatcher.h @@ -0,0 +1,52 @@ +/*************************************************************************** + * Copyright (C) 2005-2018 by the Quassel Project * + * devel@quassel-irc.org * + * * + * This program 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) version 3. * + * * + * This program 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 this program; if not, write to the * + * Free Software Foundation, Inc., * + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. * + ***************************************************************************/ + +#pragma once + +#include + +#include "abstractsignalwatcher.h" + +class QSocketNotifier; + +/** + * Signal watcher/handler for POSIX systems. + * + * Uses a local socket to notify the main thread in a safe way. + */ +class PosixSignalWatcher : public AbstractSignalWatcher +{ + Q_OBJECT + +public: + PosixSignalWatcher(QObject *parent = nullptr); + +private: + static void signalHandler(int signal); + + void registerSignal(int signal); + +private slots: + void onNotify(int sockfd); + +private: + static int _sockpair[2]; + QSocketNotifier *_notifier{nullptr}; +}; diff --git a/src/common/quassel.cpp b/src/common/quassel.cpp index 646e8edd..57cbdcaa 100644 --- a/src/common/quassel.cpp +++ b/src/common/quassel.cpp @@ -23,13 +23,6 @@ #include #include -#include -#if !defined Q_OS_WIN && !defined Q_OS_MAC -# include -# include -# include -#endif - #include #include #include @@ -52,6 +45,12 @@ #include "syncableobject.h" #include "types.h" +#ifndef Q_OS_WIN +# include "posixsignalwatcher.h" +#else +# include "windowssignalwatcher.h" +#endif + #include "../../version.h" Quassel::Quassel() @@ -66,9 +65,8 @@ bool Quassel::init() if (instance()->_initialized) return true; // allow multiple invocations because of MonolithicApplication +#if 0 // Setup signal handling - // TODO: Don't use unsafe methods, see handleSignal() - // We catch SIGTERM and SIGINT (caused by Ctrl+C) to graceful shutdown Quassel. signal(SIGTERM, handleSignal); signal(SIGINT, handleSignal); @@ -97,6 +95,9 @@ bool Quassel::init() # endif /* Q_OS_WIN */ #endif /* Q_OS_WIN || HAVE_EXECINFO */ } +#endif + + instance()->setupSignalHandling(); instance()->_initialized = true; qsrand(QTime(0, 0, 0).secsTo(QTime::currentTime())); @@ -139,6 +140,7 @@ void Quassel::quit() // Protect against multiple invocations (e.g. triggered by MainWin::closeEvent()) if (!_quitting) { _quitting = true; + quInfo() << "Quitting..."; if (_quitHandlers.empty()) { QCoreApplication::quit(); } @@ -322,44 +324,40 @@ const Quassel::BuildInfo &Quassel::buildInfo() } -//! Signal handler for graceful shutdown. -//! @todo: Ensure this doesn't use unsafe methods (it does currently) -//! cf. QSocketNotifier, UnixSignalWatcher -void Quassel::handleSignal(int sig) +void Quassel::setupSignalHandling() { - switch (sig) { - case SIGTERM: - case SIGINT: - qWarning("%s", qPrintable(QString("Caught signal %1 - exiting.").arg(sig))); - instance()->quit(); - break; -#ifndef Q_OS_WIN -// Windows does not support SIGHUP - case SIGHUP: - // Most applications use this as the 'configuration reload' command, e.g. nginx uses it for - // graceful reloading of processes. - quInfo() << "Caught signal" << SIGHUP << "- reloading configuration"; - if (instance()->reloadConfig()) { - quInfo() << "Successfully reloaded configuration"; - } - break; -#endif - case SIGABRT: - case SIGSEGV: #ifndef Q_OS_WIN - case SIGBUS: + _signalWatcher = new PosixSignalWatcher(this); +#else + _signalWatcher = new WindowsSignalWatcher(this); #endif - instance()->logBacktrace(instance()->coreDumpFileName()); - exit(EXIT_FAILURE); - default: - ; - } + connect(_signalWatcher, SIGNAL(handleSignal(AbstractSignalWatcher::Action)), this, SLOT(handleSignal(AbstractSignalWatcher::Action))); } - -void Quassel::disableCrashHandler() +void Quassel::handleSignal(AbstractSignalWatcher::Action action) { - instance()->_handleCrashes = false; + switch (action) { + case AbstractSignalWatcher::Action::Reload: + // Most applications use this as the 'configuration reload' command, e.g. nginx uses it for graceful reloading of processes. + if (!_reloadHandlers.empty()) { + quInfo() << "Reloading configuration"; + if (reloadConfig()) { + quInfo() << "Successfully reloaded configuration"; + } + } + break; + case AbstractSignalWatcher::Action::Terminate: + if (!_quitting) { + quit(); + } + else { + quInfo() << "Already shutting down, ignoring signal"; + } + break; + case AbstractSignalWatcher::Action::HandleCrash: + logBacktrace(instance()->coreDumpFileName()); + exit(EXIT_FAILURE); + } } diff --git a/src/common/quassel.h b/src/common/quassel.h index 157386a8..77f7b870 100644 --- a/src/common/quassel.h +++ b/src/common/quassel.h @@ -32,6 +32,7 @@ #include #include "abstractcliparser.h" +#include "abstractsignalwatcher.h" #include "singleton.h" class QFile; @@ -228,7 +229,6 @@ protected: static void setDataDirPaths(const QStringList &paths); static QStringList findDataDirPaths(); - static void disableCrashHandler(); friend class CoreApplication; friend class QtUiApplication; @@ -237,6 +237,7 @@ protected: private: void setupEnvironment(); void registerMetaTypes(); + void setupSignalHandling(); /** * Requests a reload of relevant runtime configuration. @@ -250,13 +251,13 @@ private: void logBacktrace(const QString &filename); - static void handleSignal(int signal); +private slots: + void handleSignal(AbstractSignalWatcher::Action action); private: BuildInfo _buildInfo; RunMode _runMode; bool _initialized{false}; - bool _handleCrashes{true}; bool _quitting{false}; QString _coreDumpFileName; @@ -267,6 +268,7 @@ private: std::shared_ptr _cliParser; Logger *_logger; + AbstractSignalWatcher *_signalWatcher{nullptr}; std::vector _reloadHandlers; std::vector _quitHandlers; diff --git a/src/common/windowssignalwatcher.cpp b/src/common/windowssignalwatcher.cpp new file mode 100644 index 00000000..5c9a4815 --- /dev/null +++ b/src/common/windowssignalwatcher.cpp @@ -0,0 +1,83 @@ +/*************************************************************************** + * Copyright (C) 2005-2018 by the Quassel Project * + * devel@quassel-irc.org * + * * + * This program 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) version 3. * + * * + * This program 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 this program; if not, write to the * + * Free Software Foundation, Inc., * + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. * + ***************************************************************************/ + +#include "windowssignalwatcher.h" + +#include +#include + +#include + +#include "logmessage.h" + +// This handler is called by Windows in a different thread when a console event happens +// FIXME: When the console window is closed, the application is supposedly terminated as soon as +// this handler returns. We may want to block and wait for the main thread so set some +// condition variable once shutdown is complete... +static BOOL WINAPI consoleCtrlHandler(DWORD ctrlType) +{ + switch (ctrlType) { + case CTRL_C_EVENT: // Ctrl+C + case CTRL_CLOSE_EVENT: // Closing the console window + WindowsSignalWatcher::signalHandler(SIGTERM); + return TRUE; + default: + return FALSE; + } +} + +WindowsSignalWatcher::WindowsSignalWatcher(QObject *parent) + : AbstractSignalWatcher{parent} + , Singleton{this} +{ + static bool registered = []() { + qRegisterMetaType(); + return true; + }(); + Q_UNUSED(registered) + + // Use POSIX-style API to register standard signals. + // Not sure if this is safe to use, but it has worked so far... + signal(SIGTERM, signalHandler); + signal(SIGINT, signalHandler); + signal(SIGABRT, signalHandler); + signal(SIGSEGV, signalHandler); + + // React on console window events + SetConsoleCtrlHandler(consoleCtrlHandler, TRUE); +} + +void WindowsSignalWatcher::signalHandler(int signal) +{ + quInfo() << "Caught signal" << signal; + + switch (signal) { + case SIGINT: + case SIGTERM: + emit instance()->handleSignal(Action::Terminate); + break; + case SIGABRT: + case SIGSEGV: + emit instance()->handleSignal(Action::HandleCrash); + break; + default: + ; + } +} diff --git a/src/common/windowssignalwatcher.h b/src/common/windowssignalwatcher.h new file mode 100644 index 00000000..923d3341 --- /dev/null +++ b/src/common/windowssignalwatcher.h @@ -0,0 +1,36 @@ +/*************************************************************************** + * Copyright (C) 2005-2018 by the Quassel Project * + * devel@quassel-irc.org * + * * + * This program 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) version 3. * + * * + * This program 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 this program; if not, write to the * + * Free Software Foundation, Inc., * + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. * + ***************************************************************************/ + +#pragma once + +#include + +#include "abstractsignalwatcher.h" +#include "singleton.h" + +class WindowsSignalWatcher : public AbstractSignalWatcher, private Singleton +{ + Q_OBJECT + +public: + WindowsSignalWatcher(QObject *parent = nullptr); + + static void signalHandler(int signal); +}; diff --git a/src/core/coreapplication.cpp b/src/core/coreapplication.cpp index 9ad6ef09..0f6bb95f 100644 --- a/src/core/coreapplication.cpp +++ b/src/core/coreapplication.cpp @@ -23,10 +23,6 @@ CoreApplication::CoreApplication(int &argc, char **argv) : QCoreApplication(argc, argv) { -#ifdef Q_OS_MAC - Quassel::disableCrashHandler(); -#endif /* Q_OS_MAC */ - Quassel::setRunMode(Quassel::CoreOnly); Quassel::registerQuitHandler([this]() { connect(_core.get(), SIGNAL(shutdownComplete()), this, SLOT(onShutdownComplete())); diff --git a/src/qtui/monoapplication.cpp b/src/qtui/monoapplication.cpp index 128ab979..34890edc 100644 --- a/src/qtui/monoapplication.cpp +++ b/src/qtui/monoapplication.cpp @@ -23,6 +23,7 @@ #include "client.h" #include "core.h" #include "internalpeer.h" +#include "logmessage.h" #include "qtui.h" class InternalPeer; @@ -30,10 +31,6 @@ class InternalPeer; MonolithicApplication::MonolithicApplication(int &argc, char **argv) : QtUiApplication(argc, argv) { -#if defined(HAVE_KDE4) || defined(Q_OS_MAC) - Quassel::disableCrashHandler(); -#endif /* HAVE_KDE4 || Q_OS_MAC */ - Quassel::setRunMode(Quassel::Monolithic); } @@ -56,6 +53,7 @@ void MonolithicApplication::init() Quassel::QuitHandler MonolithicApplication::quitHandler() { return [this]() { + quInfo() << "Client shutting down..."; connect(_client.get(), SIGNAL(destroyed()), this, SLOT(onClientDestroyed())); _client.release()->deleteLater(); }; diff --git a/src/qtui/qtuiapplication.cpp b/src/qtui/qtuiapplication.cpp index 03f887b4..fc04f59b 100644 --- a/src/qtui/qtuiapplication.cpp +++ b/src/qtui/qtuiapplication.cpp @@ -30,6 +30,7 @@ #include "chatviewsettings.h" #include "cliparser.h" +#include "logmessage.h" #include "mainwin.h" #include "qtui.h" #include "qtuisettings.h" @@ -81,10 +82,6 @@ QtUiApplication::QtUiApplication(int &argc, char **argv) #endif /* HAVE_KDE4 */ -#if defined(HAVE_KDE4) || defined(Q_OS_MAC) - Quassel::disableCrashHandler(); -#endif /* HAVE_KDE4 || Q_OS_MAC */ - Quassel::setRunMode(Quassel::ClientOnly); #if QT_VERSION >= 0x050000 @@ -132,6 +129,7 @@ Quassel::QuitHandler QtUiApplication::quitHandler() { // Wait until the Client instance is destroyed before quitting the event loop return [this]() { + quInfo() << "Client shutting down..."; connect(_client.get(), SIGNAL(destroyed()), QCoreApplication::instance(), SLOT(quit())); _client.release()->deleteLater(); };