From f274f91cebb0a4fd2ebe37bb3a605c47d6acd404 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?M=C3=A5rten=20Nordheim?= <marten.nordheim@qt.io>
Date: Fri, 5 Mar 2021 14:33:01 +0100
Subject: [PATCH] QEventDispatcher(Win): Always honor interrupted status to
 avoid races

There may be a race where e.g. thread 'B' is woken up by a queued invoke.
At the same time thread 'A' asks 'B' to quit, which will set various
atomics (some important ones are 'interrupt' in the dispatcher and
'exit' in the event loop), but it does _not_ try to send another wake
since there is already an unhandled wake triggered by 'B' itself.
Sadly 'B' reads the 'exit' atomic before 'A' updates it.
Then, slightly before, 'B' sets 'interrupt' back to 0, 'A' write 1 to
it, meaning 'A's interrupt is ignored. Then, since there is no
interrupt, 'B' goes back to waiting for events, leaving the thread alive
and running instead of quitting.

Maybe this has unforeseen consequences (one consequence is that it will
return and re-enter the event dispatcher once more, possible
unnecessarily)

Fixes: QTBUG-91539
Pick-to: 6.1 6.0 5.15
Change-Id: Ie6f861f42ffddf4817d5c8af2d764abe9d9103c2
Reviewed-by: Alex Trotsenko <alex1973tr@gmail.com>
Reviewed-by: Volker Hilsheimer <volker.hilsheimer@qt.io>
Reviewed-by: Thiago Macieira <thiago.macieira@intel.com>
---
 src/corelib/kernel/qeventdispatcher_win.cpp   |  6 ++++-
 .../qeventdispatcher/tst_qeventdispatcher.cpp | 27 +++++++++++++++++++
 2 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/src/corelib/kernel/qeventdispatcher_win.cpp b/src/corelib/kernel/qeventdispatcher_win.cpp
index 19604de32fe..80297feef14 100644
--- a/src/corelib/kernel/qeventdispatcher_win.cpp
+++ b/src/corelib/kernel/qeventdispatcher_win.cpp
@@ -475,13 +475,17 @@ bool QEventDispatcherWin32::processEvents(QEventLoop::ProcessEventsFlags flags)
 {
     Q_D(QEventDispatcherWin32);
 
-    d->interrupt.storeRelaxed(false);
+    // We don't know _when_ the interrupt occurred so we have to honor it.
+    const bool wasInterrupted = d->interrupt.fetchAndStoreRelaxed(false);
     emit awake();
 
     // To prevent livelocks, send posted events once per iteration.
     // QCoreApplication::sendPostedEvents() takes care about recursions.
     sendPostedEvents();
 
+    if (wasInterrupted)
+        return false;
+
     auto threadData = d->threadData.loadRelaxed();
     bool canWait;
     bool retVal = false;
diff --git a/tests/auto/corelib/kernel/qeventdispatcher/tst_qeventdispatcher.cpp b/tests/auto/corelib/kernel/qeventdispatcher/tst_qeventdispatcher.cpp
index 8d69314fa51..21aee94e5df 100644
--- a/tests/auto/corelib/kernel/qeventdispatcher/tst_qeventdispatcher.cpp
+++ b/tests/auto/corelib/kernel/qeventdispatcher/tst_qeventdispatcher.cpp
@@ -71,6 +71,7 @@ private slots:
     void processEventsOnlySendsQueuedEvents();
     void postedEventsPingPong();
     void eventLoopExit();
+    void interruptTrampling();
 };
 
 bool tst_QEventDispatcher::event(QEvent *e)
@@ -421,5 +422,31 @@ void tst_QEventDispatcher::eventLoopExit()
     QVERIFY(!timeoutObserved);
 }
 
+// Based on QTBUG-91539: In the event dispatcher on Windows we overwrite the
+// interrupt once we start processing events (this pattern is also in the 'unix' dispatcher)
+// which would lead the dispatcher to accidentally ignore certain interrupts and,
+// as in the bug report, would not quit, leaving the thread alive and running.
+void tst_QEventDispatcher::interruptTrampling()
+{
+    class WorkerThread : public QThread
+    {
+        void run() override {
+            auto dispatcher = eventDispatcher();
+            QVERIFY(dispatcher);
+            dispatcher->processEvents(QEventLoop::AllEvents);
+            QTimer::singleShot(0, [dispatcher]() {
+                dispatcher->wakeUp();
+            });
+            dispatcher->processEvents(QEventLoop::WaitForMoreEvents);
+            dispatcher->interrupt();
+            dispatcher->processEvents(QEventLoop::WaitForMoreEvents);
+        }
+    };
+    WorkerThread thread;
+    thread.start();
+    QVERIFY(thread.wait(1000));
+    QVERIFY(thread.isFinished());
+}
+
 QTEST_MAIN(tst_QEventDispatcher)
 #include "tst_qeventdispatcher.moc"