From 21b8d6ae3228a116c7181de9cc2672ec42357514 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?M=C3=A5rten=20Nordheim?= <marten.nordheim@qt.io>
Date: Mon, 7 Nov 2022 14:05:04 +0100
Subject: [PATCH] QNetworkInformation[Win]: Fix potential use-after/during-free

The WinRT NetworkStatusChanged callback may happen during or slightly
before we unregister our token, which we usually follow up by destroying
the object. So we have to avoid potentially doing work on a deallocated
object.
Do this using the old QPointer-trick. Neither me nor reporter can
reproduce it locally, so this is only a best-measure.
Further problems may be that the storage for the lambda has already
been destroyed and repurposed, in which case the pointer may be valid,
but junk, which would lead to another crash. But this is unavoidable as
long as MS does not synchronize callbacks with (un)registering new
callbacks. To attempt combatting this we hold our own lock around
unregistration and the "meat" of the callback.

Fixes: QTBUG-108218
Change-Id: Iacf8b8f458cca3152ff395e9a38e8df193534f46
Reviewed-by: Timur Pocheptsov <timur.pocheptsov@qt.io>
(cherry picked from commit 7898de4258e06671feb84cfd0b746009b64f7f0d)
---
 .../qnetworklistmanagerevents.cpp             | 33 +++++++++++++------
 .../qnetworklistmanagerevents.h               |  4 ++-
 ...rklistmanagernetworkinformationbackend.cpp |  1 -
 3 files changed, 26 insertions(+), 12 deletions(-)

diff --git a/src/plugins/networkinformation/networklistmanager/qnetworklistmanagerevents.cpp b/src/plugins/networkinformation/networklistmanager/qnetworklistmanagerevents.cpp
index 787259fd659..e93da4eaec8 100644
--- a/src/plugins/networkinformation/networklistmanager/qnetworklistmanagerevents.cpp
+++ b/src/plugins/networkinformation/networklistmanager/qnetworklistmanagerevents.cpp
@@ -3,6 +3,10 @@
 
 #include "qnetworklistmanagerevents.h"
 
+#include <QtCore/qpointer.h>
+
+#include <mutex>
+
 #ifdef SUPPORTS_WINRT
 #include <winrt/base.h>
 #include <QtCore/private/qfactorycacheregistration_p.h>
@@ -102,11 +106,16 @@ bool QNetworkListManagerEvents::start()
 
 #ifdef SUPPORTS_WINRT
     using namespace winrt::Windows::Networking::Connectivity;
+    using winrt::Windows::Foundation::IInspectable;
     // Register for changes in the network and store a token to unregister later:
     token = NetworkInformation::NetworkStatusChanged(
-            [this](const winrt::Windows::Foundation::IInspectable sender) {
+            [owner = QPointer(this)](const IInspectable sender) {
                 Q_UNUSED(sender);
-                emitWinRTUpdates();
+                if (owner) {
+                    std::scoped_lock locker(owner->winrtLock);
+                    if (owner->token)
+                        owner->emitWinRTUpdates();
+                }
             });
     // Emit initial state
     emitWinRTUpdates();
@@ -115,24 +124,28 @@ bool QNetworkListManagerEvents::start()
     return true;
 }
 
-bool QNetworkListManagerEvents::stop()
+void QNetworkListManagerEvents::stop()
 {
     Q_ASSERT(connectionPoint);
     auto hr = connectionPoint->Unadvise(cookie);
     if (FAILED(hr)) {
         qCWarning(lcNetInfoNLM) << "Failed to unsubscribe from network connectivity events:"
                                 << errorStringFromHResult(hr);
-        return false;
+    } else {
+        cookie = 0;
     }
-    cookie = 0;
+    // Even if we fail we should still try to unregister from winrt events:
 
 #ifdef SUPPORTS_WINRT
-    using namespace winrt::Windows::Networking::Connectivity;
-    // Pass the token we stored earlier to unregister:
-    NetworkInformation::NetworkStatusChanged(token);
-    token = {};
+    // Try to synchronize unregistering with potentially in-progress callbacks
+    std::scoped_lock locker(winrtLock);
+    if (token) {
+        using namespace winrt::Windows::Networking::Connectivity;
+        // Pass the token we stored earlier to unregister:
+        NetworkInformation::NetworkStatusChanged(token);
+        token = {};
+    }
 #endif
-    return true;
 }
 
 bool QNetworkListManagerEvents::checkBehindCaptivePortal()
diff --git a/src/plugins/networkinformation/networklistmanager/qnetworklistmanagerevents.h b/src/plugins/networkinformation/networklistmanager/qnetworklistmanagerevents.h
index b82d123fa43..67403d55758 100644
--- a/src/plugins/networkinformation/networklistmanager/qnetworklistmanagerevents.h
+++ b/src/plugins/networkinformation/networklistmanager/qnetworklistmanagerevents.h
@@ -8,6 +8,7 @@
 #include <QtCore/qstring.h>
 #include <QtCore/qobject.h>
 #include <QtCore/qloggingcategory.h>
+#include <QtCore/qmutex.h>
 
 #include <objbase.h>
 #include <netlistmgr.h>
@@ -57,7 +58,7 @@ public:
     HRESULT STDMETHODCALLTYPE ConnectivityChanged(NLM_CONNECTIVITY newConnectivity) override;
 
     [[nodiscard]] bool start();
-    bool stop();
+    void stop();
 
     [[nodiscard]] bool checkBehindCaptivePortal();
 
@@ -74,6 +75,7 @@ private:
     void emitWinRTUpdates();
 
     winrt::event_token token;
+    QMutex winrtLock;
 #endif
 
     QAtomicInteger<ULONG> ref = 0;
diff --git a/src/plugins/networkinformation/networklistmanager/qnetworklistmanagernetworkinformationbackend.cpp b/src/plugins/networkinformation/networklistmanager/qnetworklistmanagernetworkinformationbackend.cpp
index 9673d2cf696..e7d49b4591b 100644
--- a/src/plugins/networkinformation/networklistmanager/qnetworklistmanagernetworkinformationbackend.cpp
+++ b/src/plugins/networkinformation/networklistmanager/qnetworklistmanagernetworkinformationbackend.cpp
@@ -198,7 +198,6 @@ void QNetworkListManagerNetworkInformationBackend::stop()
 {
     if (monitoring) {
         Q_ASSERT(managerEvents);
-        // Can return false but realistically shouldn't since that would break everything:
         managerEvents->stop();
         monitoring = false;
         managerEvents.Reset();