From f06686fa093c4a2d7d6ffa8d219cc8cc265868bc Mon Sep 17 00:00:00 2001 From: Rex Dieter Date: Thu, 10 Mar 2016 10:44:24 -0600 Subject: [PATCH] candidate fixes for various QtDBus deadlocks (QTBUG-51648,QTBUG-51649,QTBUG-51676) --- ...signal-hooks-and-object-tree-in-clos.patch | 88 +++++++++++ ...l-pending-call-with-error-if-disconn.patch | 142 ++++++++++++++++++ ...nchrnoize-local-message-in-daemon-th.patch | 37 +++++ qt5-qtbase.spec | 18 ++- 4 files changed, 284 insertions(+), 1 deletion(-) create mode 100644 QTBUG-51648-QtDBus-clean-up-signal-hooks-and-object-tree-in-clos.patch create mode 100644 QTBUG-51649-QtDBus-finish-all-pending-call-with-error-if-disconn.patch create mode 100644 QTBUG-51676-QtDBus-do-not-synchrnoize-local-message-in-daemon-th.patch diff --git a/QTBUG-51648-QtDBus-clean-up-signal-hooks-and-object-tree-in-clos.patch b/QTBUG-51648-QtDBus-clean-up-signal-hooks-and-object-tree-in-clos.patch new file mode 100644 index 0000000..26c15d0 --- /dev/null +++ b/QTBUG-51648-QtDBus-clean-up-signal-hooks-and-object-tree-in-clos.patch @@ -0,0 +1,88 @@ +From b024fbe83863fc57364a52c717d5b43d654bdb5d Mon Sep 17 00:00:00 2001 +From: Weng Xuetian +Date: Sat, 5 Mar 2016 12:23:21 -0800 +Subject: [PATCH] QtDBus: clean up signal hooks and object tree in + closeConnection + +If a QObject is added or passed as receiver to QDBusConnection::connect() +and it is managed by Q_GLOBAL_STATIC or similar mechanism, it is +possible that when that its destructor is called after the dbus daemon +thread ends. In that case, QObject::destroyed connected via +Qt::BlockingQueuedConnection to QDBusConnectionPrivate will cause dead +lock since the thread is no longer processing events. + +Task-number: QTBUG-51648 +Change-Id: I1a1810a6d6d0234af0269d5f3fc1f54101bf1547 +--- + src/dbus/qdbusconnection_p.h | 1 + + src/dbus/qdbusintegrator.cpp | 28 +++++++++++++++++++++++++++- + 2 files changed, 28 insertions(+), 1 deletion(-) + +diff --git a/src/dbus/qdbusconnection_p.h b/src/dbus/qdbusconnection_p.h +index c77daf7..565eb83 100644 +--- a/src/dbus/qdbusconnection_p.h ++++ b/src/dbus/qdbusconnection_p.h +@@ -254,6 +254,7 @@ private: + const QVector &metaTypes, int slotIdx); + + SignalHookHash::Iterator removeSignalHookNoLock(SignalHookHash::Iterator it); ++ void disconnectObjectTree(ObjectTreeNode &node); + + bool isServiceRegisteredByThread(const QString &serviceName); + +diff --git a/src/dbus/qdbusintegrator.cpp b/src/dbus/qdbusintegrator.cpp +index cd44861..a3cd47b 100644 +--- a/src/dbus/qdbusintegrator.cpp ++++ b/src/dbus/qdbusintegrator.cpp +@@ -1030,7 +1030,6 @@ QDBusConnectionPrivate::~QDBusConnectionPrivate() + qPrintable(name)); + + closeConnection(); +- rootNode.children.clear(); // free resources + qDeleteAll(cachedMetaObjects); + + if (mode == ClientMode || mode == PeerMode) { +@@ -1052,6 +1051,20 @@ QDBusConnectionPrivate::~QDBusConnectionPrivate() + } + } + ++void QDBusConnectionPrivate::disconnectObjectTree(QDBusConnectionPrivate::ObjectTreeNode &haystack) ++{ ++ QDBusConnectionPrivate::ObjectTreeNode::DataList::Iterator it = haystack.children.begin(); ++ ++ while (it != haystack.children.end()) { ++ disconnectObjectTree(*it); ++ it++; ++ } ++ ++ if (haystack.obj) { ++ haystack.obj->disconnect(this); ++ } ++} ++ + void QDBusConnectionPrivate::closeConnection() + { + QDBusWriteLocker locker(CloseConnectionAction, this); +@@ -1075,6 +1088,19 @@ void QDBusConnectionPrivate::closeConnection() + } + + qDeleteAll(pendingCalls); ++ ++ // clean up all signal hook and object tree, to avoid QObject::destroyed ++ // being activated to dbus daemon thread which already quits. ++ // dbus connection is already closed, so there is nothing we could do be clean ++ // up everything here. ++ SignalHookHash::iterator sit = signalHooks.begin(); ++ while (sit != signalHooks.end()) { ++ sit.value().obj->disconnect(this); ++ sit++; ++ } ++ ++ disconnectObjectTree(rootNode); ++ rootNode.children.clear(); // free resources + } + + void QDBusConnectionPrivate::checkThread() +-- +2.5.0 + diff --git a/QTBUG-51649-QtDBus-finish-all-pending-call-with-error-if-disconn.patch b/QTBUG-51649-QtDBus-finish-all-pending-call-with-error-if-disconn.patch new file mode 100644 index 0000000..a3794b8 --- /dev/null +++ b/QTBUG-51649-QtDBus-finish-all-pending-call-with-error-if-disconn.patch @@ -0,0 +1,142 @@ +From 136eeec876ed5b995e7c27bcdcefe0199f5f183d Mon Sep 17 00:00:00 2001 +From: Weng Xuetian +Date: Thu, 3 Mar 2016 21:56:53 -0800 +Subject: [PATCH] QtDBus: finish all pending call with error if disconnected + +libdbus will send a local signal if connection gets disconnected. When +this happens, end all pending calls with QDBusError::Disconnected. + +Task-number: QTBUG-51649 +Change-Id: I5c7d2a468bb5da746d0c0e53e458c1e376f186a9 +--- + src/dbus/qdbusintegrator.cpp | 26 +++++++++++++++++----- + src/dbus/qdbusutil_p.h | 6 +++++ + .../dbus/qdbusconnection/tst_qdbusconnection.cpp | 22 ++++++++++++++++++ + .../dbus/qdbusconnection/tst_qdbusconnection.h | 1 + + 4 files changed, 49 insertions(+), 6 deletions(-) + +diff --git a/src/dbus/qdbusintegrator.cpp b/src/dbus/qdbusintegrator.cpp +index cd44861..320419f 100644 +--- a/src/dbus/qdbusintegrator.cpp ++++ b/src/dbus/qdbusintegrator.cpp +@@ -519,6 +519,14 @@ bool QDBusConnectionPrivate::handleMessage(const QDBusMessage &amsg) + switch (amsg.type()) { + case QDBusMessage::SignalMessage: + handleSignal(amsg); ++ // Check local disconnected signal from libdbus ++ if (amsg.interface() == QDBusUtil::dbusInterfaceLocal() ++ && amsg.path() == QDBusUtil::dbusPathLocal() ++ && amsg.member() == QDBusUtil::disconnected() ++ && !QDBusMessagePrivate::isLocal(amsg)) { ++ while (!pendingCalls.isEmpty()) ++ processFinishedCall(pendingCalls.first()); ++ } + // if there are any other filters in this DBusConnection, + // let them see the signal too + return false; +@@ -1767,10 +1775,16 @@ void QDBusConnectionPrivate::processFinishedCall(QDBusPendingCallPrivate *call) + + QDBusMessage &msg = call->replyMessage; + if (call->pending) { +- // decode the message +- DBusMessage *reply = q_dbus_pending_call_steal_reply(call->pending); +- msg = QDBusMessagePrivate::fromDBusMessage(reply, connection->capabilities); +- q_dbus_message_unref(reply); ++ // when processFinishedCall is called and pending call is not completed, ++ // it means we received disconnected signal from libdbus ++ if (q_dbus_pending_call_get_completed(call->pending)) { ++ // decode the message ++ DBusMessage *reply = q_dbus_pending_call_steal_reply(call->pending); ++ msg = QDBusMessagePrivate::fromDBusMessage(reply, connection->capabilities); ++ q_dbus_message_unref(reply); ++ } else { ++ msg = QDBusMessage::createError(QDBusError::Disconnected, QDBusUtil::disconnectedErrorMessage()); ++ } + } + qDBusDebug() << connection << "got message reply:" << msg; + +@@ -2070,8 +2084,8 @@ void QDBusConnectionPrivate::sendInternal(QDBusPendingCallPrivate *pcall, void * + pcall->pending = pending; + q_dbus_pending_call_set_notify(pending, qDBusResultReceived, pcall, 0); + +- // DBus won't notify us when a peer disconnects so we need to track these ourselves +- if (mode == QDBusConnectionPrivate::PeerMode) ++ // DBus won't notify us when a peer disconnects or server terminates so we need to track these ourselves ++ if (mode == QDBusConnectionPrivate::PeerMode || mode == QDBusConnectionPrivate::ClientMode) + pendingCalls.append(pcall); + + return; +diff --git a/src/dbus/qdbusutil_p.h b/src/dbus/qdbusutil_p.h +index 8f5ae92..ca70ff9 100644 +--- a/src/dbus/qdbusutil_p.h ++++ b/src/dbus/qdbusutil_p.h +@@ -155,6 +155,8 @@ namespace QDBusUtil + { return QStringLiteral(DBUS_SERVICE_DBUS); } + inline QString dbusPath() + { return QStringLiteral(DBUS_PATH_DBUS); } ++ inline QString dbusPathLocal() ++ { return QStringLiteral(DBUS_PATH_LOCAL); } + inline QString dbusInterface() + { + // it's the same string, but just be sure +@@ -165,8 +167,12 @@ namespace QDBusUtil + { return QStringLiteral(DBUS_INTERFACE_PROPERTIES); } + inline QString dbusInterfaceIntrospectable() + { return QStringLiteral(DBUS_INTERFACE_INTROSPECTABLE); } ++ inline QString dbusInterfaceLocal() ++ { return QStringLiteral(DBUS_INTERFACE_LOCAL); } + inline QString nameOwnerChanged() + { return QStringLiteral("NameOwnerChanged"); } ++ inline QString disconnected() ++ { return QStringLiteral("Disconnected"); } + inline QString disconnectedErrorMessage() + { return QStringLiteral("Not connected to D-Bus server"); } + } +diff --git a/tests/auto/dbus/qdbusconnection/tst_qdbusconnection.cpp b/tests/auto/dbus/qdbusconnection/tst_qdbusconnection.cpp +index e91f87d..6c7e6b1 100644 +--- a/tests/auto/dbus/qdbusconnection/tst_qdbusconnection.cpp ++++ b/tests/auto/dbus/qdbusconnection/tst_qdbusconnection.cpp +@@ -1218,6 +1218,28 @@ void tst_QDBusConnection::callVirtualObjectLocal() + QCOMPARE(obj.replyArguments, subPathReply.arguments()); + } + ++void tst_QDBusConnection::pendingCallWhenDisconnected() ++{ ++ QDBusServer *server = new QDBusServer; ++ QDBusConnection con = QDBusConnection::connectToPeer(server->address(), "disconnect"); ++ QTestEventLoop::instance().enterLoop(2); ++ QVERIFY(!QTestEventLoop::instance().timeout()); ++ QVERIFY(con.isConnected()); ++ ++ delete server; ++ ++ // Make sure we call the method before we know it is disconnected. ++ QVERIFY(con.isConnected()); ++ QDBusMessage message = QDBusMessage::createMethodCall("", "/", QString(), "method"); ++ QDBusPendingCall reply = con.asyncCall(message); ++ ++ QTestEventLoop::instance().enterLoop(2); ++ QVERIFY(!con.isConnected()); ++ QVERIFY(reply.isFinished()); ++ QVERIFY(reply.isError()); ++ QVERIFY(reply.error().type() == QDBusError::Disconnected); ++} ++ + QString MyObject::path; + QString MyObjectWithoutInterface::path; + QString MyObjectWithoutInterface::interface; +diff --git a/tests/auto/dbus/qdbusconnection/tst_qdbusconnection.h b/tests/auto/dbus/qdbusconnection/tst_qdbusconnection.h +index a53ba32..720e484 100644 +--- a/tests/auto/dbus/qdbusconnection/tst_qdbusconnection.h ++++ b/tests/auto/dbus/qdbusconnection/tst_qdbusconnection.h +@@ -121,6 +121,7 @@ private slots: + void registerVirtualObject(); + void callVirtualObject(); + void callVirtualObjectLocal(); ++ void pendingCallWhenDisconnected(); + + public: + QString serviceName() const { return "org.qtproject.Qt.Autotests.QDBusConnection"; } +-- +2.5.0 + diff --git a/QTBUG-51676-QtDBus-do-not-synchrnoize-local-message-in-daemon-th.patch b/QTBUG-51676-QtDBus-do-not-synchrnoize-local-message-in-daemon-th.patch new file mode 100644 index 0000000..28bef18 --- /dev/null +++ b/QTBUG-51676-QtDBus-do-not-synchrnoize-local-message-in-daemon-th.patch @@ -0,0 +1,37 @@ +From d011c92b47f95554fe639ad9c0542768d802666b Mon Sep 17 00:00:00 2001 +From: Weng Xuetian +Date: Fri, 4 Mar 2016 10:53:46 -0800 +Subject: [PATCH] QtDBus: do not synchrnoize local message in daemon thread + +When qDBusAddSpyHook is used and there is a local message, the +handleObjectCall on this message will be deferred to daemon thread. It +would cause dead lock if dbus daemon thread wait for this message and +there is no point to wait for the reply since daemon thread is not same +as the message sender thread. + +Task-number: QTBUG-51676 +Change-Id: I1dc112894cde7121e8ce302ae51b438ade1ff612 +--- + src/dbus/qdbusintegrator.cpp | 6 ++++-- + 1 file changed, 4 insertions(+), 2 deletions(-) + +diff --git a/src/dbus/qdbusintegrator.cpp b/src/dbus/qdbusintegrator.cpp +index cd44861..caf2e92 100644 +--- a/src/dbus/qdbusintegrator.cpp ++++ b/src/dbus/qdbusintegrator.cpp +@@ -1484,8 +1484,10 @@ void QDBusConnectionPrivate::handleObjectCall(const QDBusMessage &msg) + return; + } + +- if (!QDBusMessagePrivate::isLocal(msg)) { +- // external incoming message ++ if (!QDBusMessagePrivate::isLocal(msg) || QThread::currentThread() == QDBusConnectionManager::instance()) { ++ // two cases: ++ // 1. external incoming message ++ // 2. local message deferred by spy hook + // post it and forget + postEventToThread(HandleObjectCallPostEventAction, result.obj, + new QDBusActivateObjectEvent(QDBusConnection(this), this, result, +-- +2.5.0 + diff --git a/qt5-qtbase.spec b/qt5-qtbase.spec index 866981f..be3f731 100644 --- a/qt5-qtbase.spec +++ b/qt5-qtbase.spec @@ -48,7 +48,7 @@ Summary: Qt5 - QtBase components Name: qt5-qtbase Version: 5.6.0 -Release: 0.38.%{prerelease}%{?dist} +Release: 0.39.%{prerelease}%{?dist} # See LGPL_EXCEPTIONS.txt, for exception details License: LGPLv2 with exceptions or GPLv3 with exceptions @@ -96,6 +96,16 @@ Patch53: qtbase-opensource-src-5.6.0-alsa-1.1.patch # arm patch Patch54: qtbase-opensource-src-5.6.0-arm.patch +# https://codereview.qt-project.org/#/c/151496/ +Patch55: QTBUG-51648-QtDBus-clean-up-signal-hooks-and-object-tree-in-clos.patch + +# https://codereview.qt-project.org/#/c/151340/ +Patch56: QTBUG-51649-QtDBus-finish-all-pending-call-with-error-if-disconn.patch + +# https://codereview.qt-project.org/#/c/151459/ +Patch57: QTBUG-51676-QtDBus-do-not-synchrnoize-local-message-in-daemon-th.patch + + # Epel patches Patch100: qt5-qtbase-5.6.0-el6-sqrt.patch @@ -357,6 +367,9 @@ RPM macros for building Qt5 packages. %patch52 -p1 -b .moc_WORDSIZE %patch53 -p1 -b .alsa1.1 %patch54 -p1 -b .arm +%patch55 -p1 -b .QTBUG-51648 +%patch56 -p1 -b .QTBUG-51649 +%patch57 -p1 -b .QTBUG-51676 %patch100 -p1 -b .sqrt @@ -958,6 +971,9 @@ fi %changelog +* Thu Mar 10 2016 Rex Dieter 5.6.0-0.39.rc +- candidate fixes for various QtDBus deadlocks (QTBUG-51648,QTBUG-51649,QTBUG-51676) + * Mon Mar 07 2016 Rex Dieter 5.6.0-0.38.rc - backport "crash on start if system bus is not available" (QTBUG-51299)