diff options
author | Robert Griebl <robert.griebl@qt.io> | 2022-02-28 23:40:21 +0100 |
---|---|---|
committer | Robert Griebl <robert.griebl@qt.io> | 2022-03-02 16:09:52 +0000 |
commit | 3fe0a999d7a89141c7c03ca797fc7d65e07e9c52 (patch) | |
tree | 741b13b27fc76d8afea082735bcdfc0e4adc26b0 | |
parent | ab742a88b744bd4ded2a1af91417e2bd6b99215b (diff) | |
download | qtapplicationmanager-3fe0a999d7a89141c7c03ca797fc7d65e07e9c52.tar.gz |
Avoid race condition during Unix signal handling
Handling a signal in the "Forwarded" handler while simultaneously
applying a signal mask reset in the low-level handler would lead to
a race condition accessing m_handlers.
Change-Id: If7d27b7c3515faef894c585cd73bc461e8444ee5
Reviewed-by: Dominik Holland <dominik.holland@qt.io>
(cherry picked from commit ed404c4fdb7d1c74dca554dc9ae4d5d0903c9bc2)
Reviewed-by: Qt CI Bot <qt_ci_bot@qt-project.org>
-rw-r--r-- | src/common-lib/unixsignalhandler.cpp | 30 | ||||
-rw-r--r-- | src/common-lib/unixsignalhandler.h | 1 |
2 files changed, 19 insertions, 12 deletions
diff --git a/src/common-lib/unixsignalhandler.cpp b/src/common-lib/unixsignalhandler.cpp index d74e5e44..802e0db8 100644 --- a/src/common-lib/unixsignalhandler.cpp +++ b/src/common-lib/unixsignalhandler.cpp @@ -139,8 +139,8 @@ bool UnixSignalHandler::install(Type handlerType, const std::initializer_list<in auto that = UnixSignalHandler::instance(); that->m_currentSignal = sig; - for (const auto &h : that->m_handlers) { - if (h.m_signal == sig) { + for (const auto &h : qAsConst(that->m_handlers)) { + if ((h.m_signal == sig) && !h.m_disabled) { if (!h.m_qt) { h.m_handler(sig); } else { @@ -157,10 +157,14 @@ bool UnixSignalHandler::install(Type handlerType, const std::initializer_list<in } } if (that->m_resetSignalMask) { - // someone called resetToDefault - now's a good time to handle it - that->m_handlers.remove_if([that](const SigHandler &h) { - return that->m_resetSignalMask & am_sigmask(h.m_signal); - }); + // Someone called resetToDefault - now's a good time to handle it. + // We can not remove the entries in the list, because that would (a) allocate and (b) + // step on code that might be iterating over the list in the "Forwarded" handler. + + for (const auto &h : qAsConst(that->m_handlers)) { + if (that->m_resetSignalMask & am_sigmask(h.m_signal)) + h.m_disabled = true; + } that->m_resetSignalMask = 0; } that->m_currentSignal = 0; @@ -186,8 +190,8 @@ bool UnixSignalHandler::install(Type handlerType, const std::initializer_list<in return; } - for (const auto &h : UnixSignalHandler::instance()->m_handlers) { - if (h.m_qt && h.m_signal == sig) + for (const auto &h : qAsConst(m_handlers)) { + if (h.m_qt && (h.m_signal == sig) && !h.m_disabled) h.m_handler(sig); } }); @@ -200,8 +204,8 @@ bool UnixSignalHandler::install(Type handlerType, const std::initializer_list<in // this lambda is the "signal handler" multiplexer within the Qt event loop m_winLock.lock(); for (const int &sig : qAsConst(m_signalsForEventLoop)) { - for (const auto &h : UnixSignalHandler::instance()->m_handlers) { - if (h.m_qt && h.m_signal == sig) + for (const auto &h : qAsConst(m_handlers)) { + if (h.m_qt && (h.m_signal == sig) && !h.m_disabled) h.m_handler(sig); } } @@ -212,12 +216,14 @@ bool UnixSignalHandler::install(Type handlerType, const std::initializer_list<in #else qCWarning(LogSystem) << "Unix signal handling via 'ForwardedToEventLoopHandler' is not " "supported on this platform"; - //TODO: use the private QWindowsPipe{Reader,Writer} classes in QtBase on Windows return false; #endif } - // STL'ish for append with construct in place + // This is UB! We cannot guarantee that the signal handler is not currently executing and + // iterating over this list. In practice, this is a none-issue in the AM however, because all + // install() calls are done right at startup time. + // To do it right, we would need a lock-free list structure for m_handlers. for (int sig : sigs) m_handlers.emplace_back(sig, handlerType == ForwardedToEventLoopHandler, handler); diff --git a/src/common-lib/unixsignalhandler.h b/src/common-lib/unixsignalhandler.h index f1de44f1..eaa33109 100644 --- a/src/common-lib/unixsignalhandler.h +++ b/src/common-lib/unixsignalhandler.h @@ -93,6 +93,7 @@ private: int m_signal; bool m_qt; std::function<void(int)> m_handler; + mutable QAtomicInteger<int> m_disabled; }; std::list<SigHandler> m_handlers; // we're using STL to avoid (accidental) implicit copies |