File 0003-QDBusConnection-Merge-the-dispatch-and-the... of Package libqt5-qtbase

Overview Repositories Revisions Requests Users Attributes Meta

File 0003-QDBusConnection-Merge-the-dispatch-and-the-watch-and.patch of Package libqt5-qtbase (Revision ead70e1d8c427cc3f6a47e04dc502396)

Currently displaying revision ead70e1d8c427cc3f6a47e04dc502396 , Show latest

From a006c934177263675302cd5f4ae0c3b1e35215e3 Mon Sep 17 00:00:00 2001
From: Thiago Macieira <thiago.macieira@intel.com>
Date: Tue, 28 Oct 2014 19:26:17 -0700
Subject: [PATCH 3/4] QDBusConnection: Merge the dispatch and the
 watch-and-timeout locks
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

We don't need two anymore because they now protect the same thing: the
state of the DBusConnection. The difference existed when it was possible
for two threads to access the DBusConnection at the same time: one doing
dispatching and one doing something else. Unfortunately, even though
DBusConnection supports this, QtDBus doesn't.

From d47c05b1889bb4f06203bbc65f4660b8d0128954 (2008-10-08):
   Details:  if we're removing a timer or a watcher from our list,
   there's a race condition: one thread (not the QDBusConnection thread)
   could be asking for the removal (which causes an event to be sent),
   then deletes the pointer. In the meantime, QDBusConnection will
   process the timers and socket notifiers and could end up calling
   lidbus-1 with deleted pointers.

That commit fixed the race condition but introduced a deadlock.

Task-number: QTBUG-42189
Change-Id: I034038f763cbad3a67398909defd31a23c27c965
Reviewed-by: Jędrzej Nowacki <jedrzej.nowacki@digia.com>
Reviewed-by: Albert Astals Cid <albert.astals@canonical.com>
Reviewed-by: Frederik Gladhorn <frederik.gladhorn@theqtcompany.com>
(cherry picked from commit eb99c28861f5e841f306cfe8689627fe0e9bf2e8)
---
 src/dbus/qdbusconnection_p.h  | 16 +++++-----------
 src/dbus/qdbusintegrator.cpp  | 22 +++++++++++-----------
 src/dbus/qdbusthreaddebug_p.h |  7 -------
 3 files changed, 16 insertions(+), 29 deletions(-)

diff --git a/src/dbus/qdbusconnection_p.h b/src/dbus/qdbusconnection_p.h
index fd4ced078d667d966920728252d5ee55dda76ec1..a75dabeeee5c2b920fc4708f2eeefdaeb97ba32c 100644
--- a/src/dbus/qdbusconnection_p.h
+++ b/src/dbus/qdbusconnection_p.h
@@ -290,24 +290,18 @@ public:
     QStringList serverConnectionNames;
 
     ConnectionMode mode;
+    QDBusConnectionInterface *busService;
 
-    // members accessed in unlocked mode (except for deletion)
-    // connection and server provide their own locking mechanisms
-    // busService doesn't have state to be changed
+    // the dispatch lock protects everything related to the DBusConnection or DBusServer
+    // including the timeouts and watches
+    QMutex dispatchLock;
     DBusConnection *connection;
     DBusServer *server;
-    QDBusConnectionInterface *busService;
-
-    // watchers and timeouts are accessed from any thread
-    // but the corresponding timer and QSocketNotifier must be handled
-    // only in the object's thread
-    QMutex watchAndTimeoutLock;
     WatcherHash watchers;
     TimeoutHash timeouts;
     PendingTimeoutList timeoutsPendingAdd;
 
-    // members accessed through a lock
-    QMutex dispatchLock;
+    // the master lock protects our own internal state
     QReadWriteLock lock;
     QDBusError lastError;
 
diff --git a/src/dbus/qdbusintegrator.cpp b/src/dbus/qdbusintegrator.cpp
index 03f9eef96ed160b8b6df888661d5c1bd5840047b..b396c23a1d0c1958971ce76c8b93ab43c9a18c68 100644
--- a/src/dbus/qdbusintegrator.cpp
+++ b/src/dbus/qdbusintegrator.cpp
@@ -161,7 +161,7 @@ static dbus_bool_t qDBusAddTimeout(DBusTimeout *timeout, void *data)
     if (!q_dbus_timeout_get_enabled(timeout))
         return true;
 
-    QDBusWatchAndTimeoutLocker locker(AddTimeoutAction, d);
+    QDBusDispatchLocker locker(AddTimeoutAction, d);
     if (QCoreApplication::instance() && QThread::currentThread() == d->thread()) {
         // correct thread
         return qDBusRealAddTimeout(d, timeout, q_dbus_timeout_get_interval(timeout));
@@ -196,7 +196,7 @@ static void qDBusRemoveTimeout(DBusTimeout *timeout, void *data)
 
     QDBusConnectionPrivate *d = static_cast<QDBusConnectionPrivate *>(data);
 
-    QDBusWatchAndTimeoutLocker locker(RemoveTimeoutAction, d);
+    QDBusDispatchLocker locker(RemoveTimeoutAction, d);
 
     // is it pending addition?
     QDBusConnectionPrivate::PendingTimeoutList::iterator pit = d->timeoutsPendingAdd.begin();
@@ -269,7 +269,7 @@ static bool qDBusRealAddWatch(QDBusConnectionPrivate *d, DBusWatch *watch, int f
 {
     QDBusConnectionPrivate::Watcher watcher;
 
-    QDBusWatchAndTimeoutLocker locker(AddWatchAction, d);
+    QDBusDispatchLocker locker(AddWatchAction, d);
     if (flags & DBUS_WATCH_READABLE) {
         //qDebug("addReadWatch %d", fd);
         watcher.watch = watch;
@@ -303,7 +303,7 @@ static void qDBusRemoveWatch(DBusWatch *watch, void *data)
     QDBusConnectionPrivate *d = static_cast<QDBusConnectionPrivate *>(data);
     int fd = q_dbus_watch_get_unix_fd(watch);
 
-    QDBusWatchAndTimeoutLocker locker(RemoveWatchAction, d);
+    QDBusDispatchLocker locker(RemoveWatchAction, d);
     QDBusConnectionPrivate::WatcherHash::iterator i = d->watchers.find(fd);
     while (i != d->watchers.end() && i.key() == fd) {
         if (i.value().watch == watch) {
@@ -347,7 +347,7 @@ static void qDBusToggleWatch(DBusWatch *watch, void *data)
 
 static void qDBusRealToggleWatch(QDBusConnectionPrivate *d, DBusWatch *watch, int fd)
 {
-    QDBusWatchAndTimeoutLocker locker(ToggleWatchAction, d);
+    QDBusDispatchLocker locker(ToggleWatchAction, d);
 
     QDBusConnectionPrivate::WatcherHash::iterator i = d->watchers.find(fd);
     while (i != d->watchers.end() && i.key() == fd) {
@@ -1022,8 +1022,8 @@ void QDBusConnectionPrivate::deliverCall(QObject *object, int /*flags*/, const Q
 extern bool qDBusInitThreads();
 
 QDBusConnectionPrivate::QDBusConnectionPrivate(QObject *p)
-    : QObject(p), ref(1), capabilities(0), mode(InvalidMode), connection(0), server(0), busService(0),
-      watchAndTimeoutLock(QMutex::Recursive), dispatchLock(QMutex::Recursive),
+    : QObject(p), ref(1), capabilities(0), mode(InvalidMode), busService(0),
+      dispatchLock(QMutex::Recursive), connection(0), server(0),
       rootNode(QString(QLatin1Char('/'))),
       anonymousAuthenticationAllowed(false)
 {
@@ -1133,7 +1133,7 @@ bool QDBusConnectionPrivate::handleError(const QDBusErrorInternal &error)
 void QDBusConnectionPrivate::timerEvent(QTimerEvent *e)
 {
     {
-        QDBusWatchAndTimeoutLocker locker(TimerEventAction, this);
+        QDBusDispatchLocker locker(TimerEventAction, this);
         DBusTimeout *timeout = timeouts.value(e->timerId(), 0);
         if (timeout)
             q_dbus_timeout_handle(timeout);
@@ -1152,7 +1152,7 @@ void QDBusConnectionPrivate::customEvent(QEvent *e)
     switch (ev->subtype)
     {
     case QDBusConnectionCallbackEvent::AddTimeout: {
-        QDBusWatchAndTimeoutLocker locker(RealAddTimeoutAction, this);
+        QDBusDispatchLocker locker(RealAddTimeoutAction, this);
         while (!timeoutsPendingAdd.isEmpty()) {
             QPair<DBusTimeout *, int> entry = timeoutsPendingAdd.takeFirst();
             qDBusRealAddTimeout(this, entry.first, entry.second);
@@ -1188,7 +1188,7 @@ void QDBusConnectionPrivate::socketRead(int fd)
     QVarLengthArray<DBusWatch *, 2> pendingWatches;
 
     {
-        QDBusWatchAndTimeoutLocker locker(SocketReadAction, this);
+        QDBusDispatchLocker locker(SocketReadAction, this);
         WatcherHash::ConstIterator it = watchers.constFind(fd);
         while (it != watchers.constEnd() && it.key() == fd) {
             if (it->watch && it->read && it->read->isEnabled())
@@ -1208,7 +1208,7 @@ void QDBusConnectionPrivate::socketWrite(int fd)
     QVarLengthArray<DBusWatch *, 2> pendingWatches;
 
     {
-        QDBusWatchAndTimeoutLocker locker(SocketWriteAction, this);
+        QDBusDispatchLocker locker(SocketWriteAction, this);
         WatcherHash::ConstIterator it = watchers.constFind(fd);
         while (it != watchers.constEnd() && it.key() == fd) {
             if (it->watch && it->write && it->write->isEnabled())
diff --git a/src/dbus/qdbusthreaddebug_p.h b/src/dbus/qdbusthreaddebug_p.h
index dcde99169cc15424c7cfa0acf3fd18272ef12aca..726ab051d0739ece9d1ea623e04b39f9368464ec 100644
--- a/src/dbus/qdbusthreaddebug_p.h
+++ b/src/dbus/qdbusthreaddebug_p.h
@@ -207,13 +207,6 @@ struct QDBusDispatchLocker: QDBusMutexLocker
     { }
 };
 
-struct QDBusWatchAndTimeoutLocker: QDBusMutexLocker
-{
-    inline QDBusWatchAndTimeoutLocker(ThreadAction a, QDBusConnectionPrivate *s)
-        : QDBusMutexLocker(a, s, &s->watchAndTimeoutLock)
-    { }
-};
-
 #if QDBUS_THREAD_DEBUG
 # define SEM_ACQUIRE(action, sem)                                       \
     do {                                                                \
-- 
2.1.2

Places

File 0003-QDBusConnection-Merge-the-dispatch-and-the-watch-and.patch of Package libqt5-qtbase (Revision ead70e1d8c427cc3f6a47e04dc502396)

Places