Disconnect signals from each QObject only once in QDBusConnectionPrivate

Because the moment we disconnect from the object's destroyed() signal,
it may get destroyed in another thread. If the same object appears more
than once in the object tree or in the signal hook table, we could be
accessing a dangling pointer.

Task-number: QTBUG-52988
Change-Id: Ifea6e497f11a461db432ffff14496f0f83889104
Reviewed-by: Weng Xuetian <wengxt@gmail.com>
Reviewed-by: Thiago Macieira <thiago.macieira@intel.com>

2 files changed, 21 insertions, 12 deletions

diff --git a/src/dbus/qdbusconnection_p.h b/src/dbus/qdbusconnection_p.h
index b733a68856..fff9f29b03 100644
--- a/src/dbus/qdbusconnection_p.h
+++ b/src/dbus/qdbusconnection_p.h
@@ -254,7 +254,7 @@ private:
                      const QVector<int> &metaTypes, int slotIdx);
 
     SignalHookHash::Iterator removeSignalHookNoLock(SignalHookHash::Iterator it);
-    void disconnectObjectTree(ObjectTreeNode &node);
+    void collectAllObjects(ObjectTreeNode &node, QSet<QObject *> &set);
 
     bool isServiceRegisteredByThread(const QString &serviceName);
 
diff --git a/src/dbus/qdbusintegrator.cpp b/src/dbus/qdbusintegrator.cpp
index d0468f4af0..147966b9b0 100644
--- a/src/dbus/qdbusintegrator.cpp
+++ b/src/dbus/qdbusintegrator.cpp
@@ -1071,17 +1071,18 @@ QDBusConnectionPrivate::~QDBusConnectionPrivate()
     }
 }
 
-void QDBusConnectionPrivate::disconnectObjectTree(QDBusConnectionPrivate::ObjectTreeNode &haystack)
+void QDBusConnectionPrivate::collectAllObjects(QDBusConnectionPrivate::ObjectTreeNode &haystack,
+                                               QSet<QObject *> &set)
 {
     QDBusConnectionPrivate::ObjectTreeNode::DataList::Iterator it = haystack.children.begin();
 
     while (it != haystack.children.end()) {
-        disconnectObjectTree(*it);
+        collectAllObjects(*it, set);
         it++;
     }
 
     if (haystack.obj)
-        haystack.obj->disconnect(this);
+        set.insert(haystack.obj);
 }
 
 void QDBusConnectionPrivate::closeConnection()
@@ -1110,15 +1111,23 @@ void QDBusConnectionPrivate::closeConnection()
 
     // Disconnect all signals from signal hooks and from the object tree to
     // avoid QObject::destroyed being sent to dbus daemon thread which has
-    // already quit.
-    SignalHookHash::iterator sit = signalHooks.begin();
-    while (sit != signalHooks.end()) {
-        sit.value().obj->disconnect(this);
-        sit++;
+    // already quit. We need to make sure we disconnect exactly once per
+    // object, because if we tried a second time, we might be hitting a
+    // dangling pointer.
+    QSet<QObject *> allObjects;
+    collectAllObjects(rootNode, allObjects);
+    SignalHookHash::const_iterator sit = signalHooks.constBegin();
+    while (sit != signalHooks.constEnd()) {
+        allObjects.insert(sit.value().obj);
+        ++sit;
+    }
+
+    // now disconnect ourselves
+    QSet<QObject *>::const_iterator oit = allObjects.constBegin();
+    while (oit != allObjects.constEnd()) {
+        (*oit)->disconnect(this);
+        ++oit;
     }
-
-    disconnectObjectTree(rootNode);
-    rootNode.children.clear();  // free resources
 }
 
 void QDBusConnectionPrivate::handleDBusDisconnection()