Fix a race condition with QtDBus blocking for replies.

If an auxiliary thread tried to block on waiting for a reply, and at
the same time the main thread handled the reply, there's room for a
race condition. So ensure only one thread is stopped at
dbus_pending_call_block().

The other thread(s) will be waiting on the QWaitCondition.

It's not a race condition for the main thread to process (and finish
processing) the reply while the auxiliary thread hasn't even started
to wait. The code will ensure that the reply is properly seen.

Task-Id: https://projects.maemo.org/bugzilla/show_bug.cgi?id=155306
Reviewed-By: Trust Me

4 files changed, 83 insertions, 34 deletions

diff --git a/src/dbus/qdbusintegrator.cpp b/src/dbus/qdbusintegrator.cpp
index 6354770..0f49dcc 100644
--- a/src/dbus/qdbusintegrator.cpp
+++ b/src/dbus/qdbusintegrator.cpp
@@ -1687,15 +1687,31 @@ static void qDBusResultReceived(DBusPendingCall *pending, void *user_data)
 void QDBusConnectionPrivate::waitForFinished(QDBusPendingCallPrivate *pcall)
 {
     Q_ASSERT(pcall->pending);
-    QDBusDispatchLocker locker(PendingCallBlockAction, this);
-    q_dbus_pending_call_block(pcall->pending);
-    // QDBusConnectionPrivate::processFinishedCall() is called automatically
+    Q_ASSERT(!pcall->autoDelete);
+    //Q_ASSERT(pcall->mutex.isLocked()); // there's no such function
+
+    if (pcall->waitingForFinished) {
+        // another thread is already waiting
+        pcall->waitForFinishedCondition.wait(&pcall->mutex);
+    } else {
+        pcall->waitingForFinished = true;
+        pcall->mutex.unlock();
+
+        {
+            QDBusDispatchLocker locker(PendingCallBlockAction, this);
+            q_dbus_pending_call_block(pcall->pending);
+            // QDBusConnectionPrivate::processFinishedCall() is called automatically
+        }
+        pcall->mutex.lock();
+    }
 }
 
 void QDBusConnectionPrivate::processFinishedCall(QDBusPendingCallPrivate *call)
 {
     QDBusConnectionPrivate *connection = const_cast<QDBusConnectionPrivate *>(call->connection);
 
+    QMutexLocker locker(&call->mutex);
+
     QDBusMessage &msg = call->replyMessage;
     if (call->pending) {
         // decode the message
@@ -1725,6 +1741,12 @@ void QDBusConnectionPrivate::processFinishedCall(QDBusPendingCallPrivate *call)
             qDBusDebug() << "Deliver failed!";
     }
 
+    if (call->pending)
+        q_dbus_pending_call_unref(call->pending);
+    call->pending = 0;
+
+    locker.unlock();
+
     // Are there any watchers?
     if (call->watcherHelper)
         call->watcherHelper->emitSignals(msg, call->sentMessage);
@@ -1732,12 +1754,10 @@ void QDBusConnectionPrivate::processFinishedCall(QDBusPendingCallPrivate *call)
     if (msg.type() == QDBusMessage::ErrorMessage)
         emit connection->callWithCallbackFailed(QDBusError(msg), call->sentMessage);
 
-    if (call->pending)
-        q_dbus_pending_call_unref(call->pending);
-    call->pending = 0;
-
-    if (call->autoDelete)
+    if (call->autoDelete) {
+        Q_ASSERT(!call->waitingForFinished); // can't wait on a call with autoDelete!
         delete call;
+    }
 }
 
 int QDBusConnectionPrivate::send(const QDBusMessage& message)
@@ -1879,17 +1899,14 @@ QDBusPendingCallPrivate *QDBusConnectionPrivate::sendWithReplyAsync(const QDBusM
 {
     if (isServiceRegisteredByThread(message.service())) {
         // special case for local calls
-        QDBusPendingCallPrivate *pcall = new QDBusPendingCallPrivate;
-        pcall->sentMessage = message;
+        QDBusPendingCallPrivate *pcall = new QDBusPendingCallPrivate(message, this);
         pcall->replyMessage = sendWithReplyLocal(message);
-        pcall->connection = this;
 
         return pcall;
     }
 
     checkThread();
-    QDBusPendingCallPrivate *pcall = new QDBusPendingCallPrivate;
-    pcall->sentMessage = message;
+    QDBusPendingCallPrivate *pcall = new QDBusPendingCallPrivate(message, this);
     pcall->ref = 0;
 
     QDBusError error;
@@ -1913,7 +1930,6 @@ QDBusPendingCallPrivate *QDBusConnectionPrivate::sendWithReplyAsync(const QDBusM
             q_dbus_message_unref(msg);
 
             pcall->pending = pending;
-            pcall->connection = this;
             q_dbus_pending_call_set_notify(pending, qDBusResultReceived, pcall, 0);
 
             return pcall;
diff --git a/src/dbus/qdbuspendingcall.cpp b/src/dbus/qdbuspendingcall.cpp
index 3db5d9f..86b585d 100644
--- a/src/dbus/qdbuspendingcall.cpp
+++ b/src/dbus/qdbuspendingcall.cpp
@@ -208,6 +208,8 @@ void QDBusPendingCallPrivate::setMetaTypes(int count, const int *types)
 
 void QDBusPendingCallPrivate::checkReceivedSignature()
 {
+    // MUST BE CALLED WITH A LOCKED MUTEX!
+
     if (replyMessage.type() == QDBusMessage::InvalidMessage)
         return;                 // not yet finished - no message to
                                 // validate against
@@ -230,6 +232,8 @@ void QDBusPendingCallPrivate::checkReceivedSignature()
 
 void QDBusPendingCallPrivate::waitForFinished()
 {
+    QMutexLocker locker(&mutex);
+
     if (replyMessage.type() != QDBusMessage::InvalidMessage)
         return;                 // already finished
 
@@ -310,7 +314,11 @@ QDBusPendingCall &QDBusPendingCall::operator=(const QDBusPendingCall &other)
 
 bool QDBusPendingCall::isFinished() const
 {
-    return !d || (d->replyMessage.type() != QDBusMessage::InvalidMessage);
+    if (!d)
+        return true; // considered finished
+
+    QMutexLocker locker(&d->mutex);
+    return d->replyMessage.type() != QDBusMessage::InvalidMessage;
 }
 
 void QDBusPendingCall::waitForFinished()
@@ -329,7 +337,10 @@ void QDBusPendingCall::waitForFinished()
 */
 bool QDBusPendingCall::isValid() const
 {
-    return d ? d->replyMessage.type() == QDBusMessage::ReplyMessage : false;
+    if (!d)
+        return false;
+    QMutexLocker locker(&d->mutex);
+    return d->replyMessage.type() == QDBusMessage::ReplyMessage;
 }
 
 /*!
@@ -343,7 +354,10 @@ bool QDBusPendingCall::isValid() const
 */
 bool QDBusPendingCall::isError() const
 {
-    return d ? d->replyMessage.type() == QDBusMessage::ErrorMessage : true;
+    if (!d)
+        return true; // considered finished and an error
+    QMutexLocker locker(&d->mutex);
+    return d->replyMessage.type() == QDBusMessage::ErrorMessage;
 }
 
 /*!
@@ -356,8 +370,10 @@ bool QDBusPendingCall::isError() const
 */
 QDBusError QDBusPendingCall::error() const
 {
-    if (d)
+    if (d) {
+        QMutexLocker locker(&d->mutex);
         return d->replyMessage;
+    }
 
     // not connected, return an error
     QDBusError err = QDBusError(QDBusError::Disconnected,
@@ -378,7 +394,10 @@ QDBusError QDBusPendingCall::error() const
 */
 QDBusMessage QDBusPendingCall::reply() const
 {
-    return d ? d->replyMessage : QDBusMessage::createError(error());
+    if (!d)
+        return QDBusMessage::createError(error());
+    QMutexLocker locker(&d->mutex);
+    return d->replyMessage;
 }
 
 #if 0
@@ -439,9 +458,8 @@ QDBusPendingCall QDBusPendingCall::fromCompletedCall(const QDBusMessage &msg)
     QDBusPendingCallPrivate *d = 0;
     if (msg.type() == QDBusMessage::ErrorMessage ||
         msg.type() == QDBusMessage::ReplyMessage) {
-        d = new QDBusPendingCallPrivate;
+        d = new QDBusPendingCallPrivate(QDBusMessage(), 0);
         d->replyMessage = msg;
-        d->connection = 0;
     }
 
     return QDBusPendingCall(d);
@@ -471,9 +489,10 @@ QDBusPendingCallWatcher::QDBusPendingCallWatcher(const QDBusPendingCall &call, Q
     : QObject(*new QDBusPendingCallWatcherPrivate, parent), QDBusPendingCall(call)
 {
     if (d) {                    // QDBusPendingCall::d
+        QMutexLocker locker(&d->mutex);
         if (!d->watcherHelper) {
             d->watcherHelper = new QDBusPendingCallWatcherHelper;
-            if (isFinished()) {
+            if (d->replyMessage.type() != QDBusMessage::InvalidMessage) {
                 // cause a signal emission anyways
                 QMetaObject::invokeMethod(d->watcherHelper, "finished", Qt::QueuedConnection);
             }
diff --git a/src/dbus/qdbuspendingcall_p.h b/src/dbus/qdbuspendingcall_p.h
index 641c397..c3aed53 100644
--- a/src/dbus/qdbuspendingcall_p.h
+++ b/src/dbus/qdbuspendingcall_p.h
@@ -57,6 +57,8 @@
 #include <qshareddata.h>
 #include <qpointer.h>
 #include <qlist.h>
+#include <qmutex.h>
+#include <qwaitcondition.h>
 
 #include "qdbusmessage.h"
 #include "qdbus_symbols_p.h"
@@ -71,24 +73,35 @@ class QDBusConnectionPrivate;
 class QDBusPendingCallPrivate: public QSharedData
 {
 public:
-    QDBusMessage sentMessage;
-    QDBusMessage replyMessage;
-//    QDBusMessage pendingReplyMessage; // used in the local loop
-    QDBusPendingCallWatcherHelper *watcherHelper;
-    DBusPendingCall *pending;
-    QDBusConnectionPrivate *connection;
+    // {
+    //     set only during construction:
+    const QDBusMessage sentMessage;
+    QDBusConnectionPrivate * const connection;
 
-    QString expectedReplySignature;
-    int expectedReplyCount;
-
-    // for the callback
+    // for the callback mechanism (see setReplyCallback and QDBusConnectionPrivate::sendWithReplyAsync)
     QPointer<QObject> receiver;
     QList<int> metaTypes;
     int methodIdx;
 
     bool autoDelete;
+    // }
+
+    mutable QMutex mutex;
+    QWaitCondition waitForFinishedCondition;
+
+    // {
+    //    protected by the mutex above:
+    QDBusPendingCallWatcherHelper *watcherHelper;
+    QDBusMessage replyMessage;
+    DBusPendingCall *pending;
+    volatile bool waitingForFinished;
+
+    QString expectedReplySignature;
+    int expectedReplyCount;
+    // }
 
-    QDBusPendingCallPrivate() : watcherHelper(0), pending(0), autoDelete(false)
+    QDBusPendingCallPrivate(const QDBusMessage &sent, QDBusConnectionPrivate *connection)
+        : sentMessage(sent), connection(connection), autoDelete(false), watcherHelper(0), pending(0), waitingForFinished(false)
     { }
     ~QDBusPendingCallPrivate();
     bool setReplyCallback(QObject *target, const char *member);
diff --git a/src/dbus/qdbuspendingreply.cpp b/src/dbus/qdbuspendingreply.cpp
index b492660..1df47f4 100644
--- a/src/dbus/qdbuspendingreply.cpp
+++ b/src/dbus/qdbuspendingreply.cpp
@@ -252,7 +252,7 @@ void QDBusPendingReplyData::assign(const QDBusPendingCall &other)
 
 void QDBusPendingReplyData::assign(const QDBusMessage &message)
 {
-    d = new QDBusPendingCallPrivate; // drops the reference to the old one
+    d = new QDBusPendingCallPrivate(QDBusMessage(), 0); // drops the reference to the old one
     d->replyMessage = message;
 }
 
@@ -271,6 +271,7 @@ QVariant QDBusPendingReplyData::argumentAt(int index) const
 void QDBusPendingReplyData::setMetaTypes(int count, const int *types)
 {
     Q_ASSERT(d);
+    QMutexLocker locker(&d->mutex);
     d->setMetaTypes(count, types);
     d->checkReceivedSignature();
 }