From 50aba545ff4b17a9c20f2be88d7969310f59acff Mon Sep 17 00:00:00 2001
From: Ievgenii Meshcheriakov <ievgenii.meshcheriakov@qt.io>
Date: Tue, 8 Aug 2023 14:18:49 +0200
Subject: QtDBus: Make access to spy hook list thread safe

Introduce a mutex that protects access to the spy hooks list.

Also don't pass around raw pointers to the spy hook list,
those can change when new hooks are added and storage is
reallocated. Change the type of the hooks list to QList
to take advantage of copy-on-write in common case.

Check whether the global static is destroyed in qDBusAddSpyHook().

Change-Id: I440a88ce088d6fb5817660c8e1e02901281b842f
Reviewed-by: Thiago Macieira <thiago.macieira@intel.com>
---
 src/dbus/qdbusintegrator.cpp | 42 +++++++++++++++++++++++++++++++-----------
 src/dbus/qdbusintegrator_p.h |  9 +++------
 2 files changed, 34 insertions(+), 17 deletions(-)

diff --git a/src/dbus/qdbusintegrator.cpp b/src/dbus/qdbusintegrator.cpp
index 826b274d84..11c867613f 100644
--- a/src/dbus/qdbusintegrator.cpp
+++ b/src/dbus/qdbusintegrator.cpp
@@ -101,7 +101,12 @@ void qdbusDefaultThreadDebug(int action, int condition, QDBusConnectionPrivate *
 qdbusThreadDebugFunc qdbusThreadDebug = nullptr;
 #endif
 
-typedef QVarLengthArray<QDBusSpyCallEvent::Hook, 4> QDBusSpyHookList;
+struct QDBusSpyHookList
+{
+    QBasicMutex lock;
+    QList<QDBusSpyCallEvent::Hook> list;
+};
+
 Q_GLOBAL_STATIC(QDBusSpyHookList, qDBusSpyHookList)
 
 extern "C" {
@@ -455,7 +460,12 @@ static QDBusConnectionPrivate::ArgMatchRules matchArgsForService(const QString &
 extern Q_DBUS_EXPORT void qDBusAddSpyHook(QDBusSpyCallEvent::Hook);
 void qDBusAddSpyHook(QDBusSpyCallEvent::Hook hook)
 {
-    qDBusSpyHookList()->append(hook);
+    auto *hooks = qDBusSpyHookList();
+    if (!hooks)
+        return;
+
+    const auto locker = qt_scoped_lock(hooks->lock);
+    hooks->list.append(hook);
 }
 
 QDBusSpyCallEvent::~QDBusSpyCallEvent()
@@ -470,14 +480,25 @@ QDBusSpyCallEvent::~QDBusSpyCallEvent()
 
 void QDBusSpyCallEvent::placeMetaCall(QObject *)
 {
-    invokeSpyHooks(msg, hooks, hookCount);
+    invokeSpyHooks(msg);
 }
 
-inline void QDBusSpyCallEvent::invokeSpyHooks(const QDBusMessage &msg, const Hook *hooks, int hookCount)
+inline void QDBusSpyCallEvent::invokeSpyHooks(const QDBusMessage &msg)
 {
-    // call the spy hook list
-    for (int i = 0; i < hookCount; ++i)
-        hooks[i](msg);
+    if (!qDBusSpyHookList.exists())
+        return;
+
+    // Create a copy of the hook list here, so that the hooks can be called
+    // without holding the lock.
+    QList<Hook> hookListCopy;
+    {
+        auto *hooks = qDBusSpyHookList();
+        const auto locker = qt_scoped_lock(hooks->lock);
+        hookListCopy = hooks->list;
+    }
+
+    for (auto hook : std::as_const(hookListCopy))
+        hook(msg);
 }
 
 extern "C" {
@@ -526,15 +547,14 @@ bool QDBusConnectionPrivate::handleMessage(const QDBusMessage &amsg)
         // a) if it's a local message, we're in the caller's thread, so invoke the filter directly
         // b) if it's an external message, post to the main thread
         if (Q_UNLIKELY(qDBusSpyHookList.exists()) && qApp) {
-            const QDBusSpyHookList &list = *qDBusSpyHookList;
             if (isLocal) {
                 Q_ASSERT(QThread::currentThread() != thread());
                 qDBusDebug() << this << "invoking message spies directly";
-                QDBusSpyCallEvent::invokeSpyHooks(amsg, list.constData(), list.size());
+                QDBusSpyCallEvent::invokeSpyHooks(amsg);
             } else {
                 qDBusDebug() << this << "invoking message spies via event";
-                QCoreApplication::postEvent(qApp, new QDBusSpyCallEvent(this, QDBusConnection(this),
-                                                                        amsg, list.constData(), list.size()));
+                QCoreApplication::postEvent(
+                        qApp, new QDBusSpyCallEvent(this, QDBusConnection(this), amsg));
 
                 // we'll be called back, so return
                 return true;
diff --git a/src/dbus/qdbusintegrator_p.h b/src/dbus/qdbusintegrator_p.h
index 275735b5d5..f997bb97b6 100644
--- a/src/dbus/qdbusintegrator_p.h
+++ b/src/dbus/qdbusintegrator_p.h
@@ -117,18 +117,15 @@ class QDBusSpyCallEvent : public QAbstractMetaCallEvent
 {
 public:
     typedef void (*Hook)(const QDBusMessage&);
-    QDBusSpyCallEvent(QDBusConnectionPrivate *cp, const QDBusConnection &c, const QDBusMessage &msg,
-                      const Hook *hooks, int count)
-        : QAbstractMetaCallEvent(cp, 0), conn(c), msg(msg), hooks(hooks), hookCount(count)
+    QDBusSpyCallEvent(QDBusConnectionPrivate *cp, const QDBusConnection &c, const QDBusMessage &msg)
+        : QAbstractMetaCallEvent(cp, 0), conn(c), msg(msg)
     {}
     ~QDBusSpyCallEvent() override;
     void placeMetaCall(QObject *) override;
-    static inline void invokeSpyHooks(const QDBusMessage &msg, const Hook *hooks, int hookCount);
+    static inline void invokeSpyHooks(const QDBusMessage &msg);
 
     QDBusConnection conn;   // keeps the refcount in QDBusConnectionPrivate up
     QDBusMessage msg;
-    const Hook *hooks;
-    int hookCount;
 };
 
 QT_END_NAMESPACE
-- 
cgit v1.2.3