From 3fe0a999d7a89141c7c03ca797fc7d65e07e9c52 Mon Sep 17 00:00:00 2001
From: Robert Griebl <robert.griebl@qt.io>
Date: Mon, 28 Feb 2022 23:40:21 +0100
Subject: 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>
---
 src/common-lib/unixsignalhandler.cpp | 30 ++++++++++++++++++------------
 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
-- 
cgit v1.2.1