From 2ac222f700e067783d593e0d37991374383066c0 Mon Sep 17 00:00:00 2001 From: Mat Sutcliffe Date: Tue, 28 Sep 2021 18:49:05 +0100 Subject: [PATCH] Issue #11 CEventLoop uses QPointer trick to guard against use-after-free See https://blogs.kde.org/2009/03/26/how-crash-almost-every-qtkde-application-and-how-fix-it-0 --- src/blackcore/airspacemonitor.cpp | 2 +- src/blackcore/application.cpp | 7 ++++++- src/blackcore/db/backgrounddataupdater.cpp | 2 +- src/blackmisc/eventloop.h | 22 ++++++++++++++++++++-- 4 files changed, 28 insertions(+), 5 deletions(-) diff --git a/src/blackcore/airspacemonitor.cpp b/src/blackcore/airspacemonitor.cpp index 3a64dbd1d..f4cdecd45 100644 --- a/src/blackcore/airspacemonitor.cpp +++ b/src/blackcore/airspacemonitor.cpp @@ -182,7 +182,7 @@ namespace BlackCore // with this little trick we try to make an asynchronous signal / slot based approach // a synchronous return value - CEventLoop eventLoop; + CEventLoop eventLoop(this); eventLoop.stopWhen(m_fsdClient, &CFSDClient::flightPlanReceived, [ = ](const auto &cs, const auto &) { return cs == callsign; }); if (eventLoop.exec(1500)) { diff --git a/src/blackcore/application.cpp b/src/blackcore/application.cpp index 9065c75de..acc8664fa 100644 --- a/src/blackcore/application.cpp +++ b/src/blackcore/application.cpp @@ -426,7 +426,7 @@ namespace BlackCore CStatusMessageList CApplication::waitForSetup(int timeoutMs) { if (!m_setupReader) { return CStatusMessage(this).error(u"No setup reader"); } - CEventLoop eventLoop; + CEventLoop eventLoop(this); eventLoop.stopWhen(this, &CApplication::setupHandlingCompleted); if (!m_setupReader->isSetupAvailable()) { @@ -435,6 +435,11 @@ namespace BlackCore // setup handling completed with success or failure, or we run into time out CStatusMessageList msgs; + if (!eventLoop.isGuardAlive()) + { + msgs.push_back(CStatusMessage(this).error(u"Setup not available, already shutting down.")); + return msgs; + } bool forced = false; if (!m_setupReader->isSetupAvailable()) { diff --git a/src/blackcore/db/backgrounddataupdater.cpp b/src/blackcore/db/backgrounddataupdater.cpp index a9364128a..c61f85436 100644 --- a/src/blackcore/db/backgrounddataupdater.cpp +++ b/src/blackcore/db/backgrounddataupdater.cpp @@ -152,7 +152,7 @@ namespace BlackCore::Db { // just give the system some time to relax, consolidation is time consuming if (!this->doWorkCheck()) { return; } - CEventLoop eventLoop; + CEventLoop eventLoop(this); eventLoop.exec(1000); } } diff --git a/src/blackmisc/eventloop.h b/src/blackmisc/eventloop.h index 686d0ec34..0dd77eb52 100644 --- a/src/blackmisc/eventloop.h +++ b/src/blackmisc/eventloop.h @@ -15,6 +15,7 @@ #include #include +#include #include namespace BlackMisc @@ -25,6 +26,16 @@ namespace BlackMisc class CEventLoop { public: + //! Constructor. + CEventLoop() : m_guard(&m_eventLoop) {} + + //! Constructor. Guard object must exist, and will be checked again when the loop quits. + CEventLoop(QObject *guard) : m_guard(guard) + { + Q_ASSERT(guard); + QObject::connect(guard, &QObject::destroyed, &m_eventLoop, [this] { m_eventLoop.exit(TimedOut); }); + } + //! Event loop will stop if the given signal is received. template void stopWhen(const T *sender, F signal) @@ -43,14 +54,20 @@ namespace BlackMisc } //! Begin processing events until the timeout or stop condition occurs. - //! \return True if the signal was received, false if it timed out. + //! \return True if the signal was received, false if it timed out or the guard object died. bool exec(int timeoutMs) { if (timeoutMs >= 0) { QTimer::singleShot(timeoutMs, &m_eventLoop, [this] { m_eventLoop.exit(TimedOut); }); } - return m_eventLoop.exec() == GotSignal; + return m_eventLoop.exec() == GotSignal && isGuardAlive(); + } + + //! True if the guard object still exists. + bool isGuardAlive() const + { + return m_guard; } private: @@ -60,6 +77,7 @@ namespace BlackMisc TimedOut, }; QEventLoop m_eventLoop; + QPointer m_guard; }; } // ns