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.

Pick-to: 6.4 6.4.1
Fixes: QTBUG-108218
Change-Id: Iacf8b8f458cca3152ff395e9a38e8df193534f46
Reviewed-by: Timur Pocheptsov <timur.pocheptsov@qt.io>

src/plugins/networkinformation/networklistmanager/qnetworklistmanagerevents.cpp

@@ -3,6 +3,10 @@
 
 #include "qnetworklistmanagerevents.h"
 
+#include <QtCore/qpointer.h>
+
+#include <mutex>
+
 #if QT_CONFIG(cpp_winrt)
 #include <winrt/base.h>
 #include <QtCore/private/qfactorycacheregistration_p.h>
@@ -102,11 +106,16 @@ bool QNetworkListManagerEvents::start()
 
 #if QT_CONFIG(cpp_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:
 
 #if QT_CONFIG(cpp_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()

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>
@@ -53,7 +54,7 @@ public:
     HRESULT STDMETHODCALLTYPE ConnectivityChanged(NLM_CONNECTIVITY newConnectivity) override;
 
     [[nodiscard]] bool start();
-    bool stop();
+    void stop();
 
     [[nodiscard]] bool checkBehindCaptivePortal();
 
@@ -70,6 +71,7 @@ private:
     void emitWinRTUpdates();
 
     winrt::event_token token;
+    QMutex winrtLock;
 #endif
 
     QAtomicInteger<ULONG> ref = 0;

src/plugins/networkinformation/networklistmanager/qnetworklistmanagernetworkinformationbackend.cpp

@@ -187,7 +187,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();