From fd256203708e79d64bad856e39bb5a43357bfd06 Mon Sep 17 00:00:00 2001 From: Thiago Macieira Date: Tue, 18 May 2010 15:58:24 +0200 Subject: 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 --- src/dbus/qdbusintegrator.cpp | 44 ++++++++++++++++++++++++++++-------------- src/dbus/qdbuspendingcall.cpp | 35 +++++++++++++++++++++++++-------- src/dbus/qdbuspendingcall_p.h | 35 ++++++++++++++++++++++----------- src/dbus/qdbuspendingreply.cpp | 3 ++- 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(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 #include #include +#include +#include #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 receiver; QList 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(); } -- cgit v0.12