From 64e3bd481e5d54d555959ceecbd5c4576c571241 Mon Sep 17 00:00:00 2001 From: Peter Seiderer Date: Mon, 17 Jun 2013 20:17:08 +0200 Subject: Fix unprotected access to QDBusPendingCallPrivate::pending. In QDBusConnectionPrivate::waitForFinished() pcall->pending was used after the protection by pcall->mutex was released. A simultaneous call to QDBusConnectionPrivate::processFinishedCall() was able to reset pcall->pending to null before it was used for the q_dbus_pending_call_block(pcall->pending) call. Fixed by releasing (and setting to 0) of pcall->pending in processFinishedCall() only in case no one is waiting yet, otherwise release pcall->pending by the first thread waiting in waitForFinished(). There is still a race condition about deleting QDBusPendingCallPrivate (too early) which will be fixed in the next two commits. Task-number: QTBUG-27809 Change-Id: I040173810ad90653fe1bd1915f22d8dd70d47d8c Reviewed-by: Thiago Macieira --- src/dbus/qdbusintegrator.cpp | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) (limited to 'src/dbus') diff --git a/src/dbus/qdbusintegrator.cpp b/src/dbus/qdbusintegrator.cpp index d5c359aea1..ed019cb662 100644 --- a/src/dbus/qdbusintegrator.cpp +++ b/src/dbus/qdbusintegrator.cpp @@ -1844,6 +1844,12 @@ void QDBusConnectionPrivate::waitForFinished(QDBusPendingCallPrivate *pcall) // QDBusConnectionPrivate::processFinishedCall() is called automatically } pcall->mutex.lock(); + + if (pcall->pending) { + q_dbus_pending_call_unref(pcall->pending); + pcall->pending = 0; + } + pcall->waitForFinishedCondition.wakeAll(); } } @@ -1890,9 +1896,10 @@ void QDBusConnectionPrivate::processFinishedCall(QDBusPendingCallPrivate *call) qDBusDebug() << "Deliver failed!"; } - if (call->pending) + if (call->pending && !call->waitingForFinished) { q_dbus_pending_call_unref(call->pending); - call->pending = 0; + call->pending = 0; + } locker.unlock(); -- cgit v1.2.3 From 72ecf5a7ecb688a7e19cbc2f70e358a94d02edf7 Mon Sep 17 00:00:00 2001 From: Peter Seiderer Date: Mon, 17 Jun 2013 20:56:08 +0200 Subject: Remove QDBusPendingCallPrivate::autoDelete logic. First step to fix race condition about deleting QDBusPendingCallPrivate. In a multithreaded application on a slow/single core cpu the following race (and segmentation fault) can occur: First thread A is running: A: QDBusPendingReply<> reply = pi->asyncCallWithArgumentList(method, argumentList); Then when the dbus answer arrives thread B will call: B: QDBusConnectionPrivate::processFinishedCall() B: ... B: locker.unlock() and runs until here, go on with thread A: A: reply.waitForFinished(); A: QDBusPendingCallPrivate::waitForFinished() A: { A: QMutexLocker locker(&mutex); A: if (replyMessage.type() != QDBusMessage::InvalidMessage) A: return; which returns immediately (mutex acquired, replyMessage alread set), now reply goes out of scope (destructor called) and QDBusPendingCall::d's destructor of type QExplicitlySharedDataPointer deletes the reference counted object QDBusPendingCallPrivate. Now thread B continues, still in processFinishedCall() B: if (call->watcherHelper) B: call->watcherHelper->emitSignals(msg, call->sentMessage); B: B: if (msg.type() == QDBusMessage::ErrorMessage) B: emit connection->callWithCallbackFailed(QDBusError(msg), B: call->sentMessage); accessing alread deleted object QDBusPendingCallPrivate via call->... Fixed QDBusPendingCallPrivate deletion by proper reference counting will be done in the next commit. Task-number: QTBUG-27809 Change-Id: I15b3f0242471b62eaafadc763fb6a33339ff2fe1 Reviewed-by: Thiago Macieira --- src/dbus/qdbusintegrator.cpp | 14 -------------- src/dbus/qdbuspendingcall_p.h | 3 +-- 2 files changed, 1 insertion(+), 16 deletions(-) (limited to 'src/dbus') diff --git a/src/dbus/qdbusintegrator.cpp b/src/dbus/qdbusintegrator.cpp index ed019cb662..4b9bfc3fc8 100644 --- a/src/dbus/qdbusintegrator.cpp +++ b/src/dbus/qdbusintegrator.cpp @@ -1828,7 +1828,6 @@ static void qDBusResultReceived(DBusPendingCall *pending, void *user_data) void QDBusConnectionPrivate::waitForFinished(QDBusPendingCallPrivate *pcall) { Q_ASSERT(pcall->pending); - Q_ASSERT(!pcall->autoDelete); //Q_ASSERT(pcall->mutex.isLocked()); // there's no such function if (pcall->waitingForFinished) { @@ -1854,13 +1853,6 @@ void QDBusConnectionPrivate::waitForFinished(QDBusPendingCallPrivate *pcall) } } -// this function is called only in a Q_ASSERT -static inline Q_DECL_UNUSED bool waitingForFinishedIsSet(QDBusPendingCallPrivate *call) -{ - const QMutexLocker locker(&call->mutex); - return call->waitingForFinished; -} - void QDBusConnectionPrivate::processFinishedCall(QDBusPendingCallPrivate *call) { QDBusConnectionPrivate *connection = const_cast(call->connection); @@ -1909,11 +1901,6 @@ void QDBusConnectionPrivate::processFinishedCall(QDBusPendingCallPrivate *call) if (msg.type() == QDBusMessage::ErrorMessage) emit connection->callWithCallbackFailed(QDBusError(msg), call->sentMessage); - - if (call->autoDelete) { - Q_ASSERT(!waitingForFinishedIsSet(call)); // can't wait on a call with autoDelete! - delete call; - } } int QDBusConnectionPrivate::send(const QDBusMessage& message) @@ -2132,7 +2119,6 @@ int QDBusConnectionPrivate::sendWithReplyAsync(const QDBusMessage &message, QObj return 1; } - pcall->autoDelete = true; pcall->ref.ref(); pcall->setReplyCallback(receiver, returnMethod); diff --git a/src/dbus/qdbuspendingcall_p.h b/src/dbus/qdbuspendingcall_p.h index 36d0b9023a..d0b28b3c0e 100644 --- a/src/dbus/qdbuspendingcall_p.h +++ b/src/dbus/qdbuspendingcall_p.h @@ -85,7 +85,6 @@ public: QVector metaTypes; int methodIdx; - bool autoDelete; // } mutable QMutex mutex; @@ -102,7 +101,7 @@ public: // } QDBusPendingCallPrivate(const QDBusMessage &sent, QDBusConnectionPrivate *connection) - : sentMessage(sent), connection(connection), autoDelete(false), watcherHelper(0), pending(0), waitingForFinished(false) + : sentMessage(sent), connection(connection), watcherHelper(0), pending(0), waitingForFinished(false) { } ~QDBusPendingCallPrivate(); bool setReplyCallback(QObject *target, const char *member); -- cgit v1.2.3 From 6c21f42657b494e24112c90d8b9fff719f1f8791 Mon Sep 17 00:00:00 2001 From: Peter Seiderer Date: Mon, 17 Jun 2013 22:44:30 +0200 Subject: Change QDBusPendingCallPrivate to full reference counting for deletion. Fixes race between QDBusConnectionPrivate::processFinishedCall() releasing the mutex before emitting signals (using various members of QDBusPendingCallPrivate) and deletion of the QDBusPendingCallPrivate object through QDBusPendingCall::d's destructor (a member of type QExplicitlySharedDataPointer) leeds to segmentation fault with CrashTest example on slow/single core arm cpu). Task-number: QTBUG-27809 Change-Id: I3590d74d1cfa5816ede764b50b83a7008ec780ff Reviewed-by: Thiago Macieira --- src/dbus/qdbusconnection.cpp | 3 +- src/dbus/qdbusconnection_p.h | 5 +-- src/dbus/qdbusintegrator.cpp | 87 ++++++++++++++++++++++++------------------- src/dbus/qdbuspendingcall.cpp | 6 +++ 4 files changed, 58 insertions(+), 43 deletions(-) (limited to 'src/dbus') diff --git a/src/dbus/qdbusconnection.cpp b/src/dbus/qdbusconnection.cpp index 74a3a752a5..a29ba4cb0f 100644 --- a/src/dbus/qdbusconnection.cpp +++ b/src/dbus/qdbusconnection.cpp @@ -54,6 +54,7 @@ #include "qdbusinterface_p.h" #include "qdbusutil_p.h" #include "qdbusconnectionmanager_p.h" +#include "qdbuspendingcall_p.h" #include "qdbusthreaddebug_p.h" @@ -611,7 +612,7 @@ QDBusPendingCall QDBusConnection::asyncCall(const QDBusMessage &message, int tim return QDBusPendingCall(0); // null pointer -> disconnected } - QDBusPendingCallPrivate *priv = d->sendWithReplyAsync(message, timeout); + QDBusPendingCallPrivate *priv = d->sendWithReplyAsync(message, 0, 0, 0, timeout); return QDBusPendingCall(priv); } diff --git a/src/dbus/qdbusconnection_p.h b/src/dbus/qdbusconnection_p.h index 73c8dcf411..c702de141a 100644 --- a/src/dbus/qdbusconnection_p.h +++ b/src/dbus/qdbusconnection_p.h @@ -199,9 +199,8 @@ public: int send(const QDBusMessage &message); QDBusMessage sendWithReply(const QDBusMessage &message, int mode, int timeout = -1); QDBusMessage sendWithReplyLocal(const QDBusMessage &message); - QDBusPendingCallPrivate *sendWithReplyAsync(const QDBusMessage &message, int timeout = -1); - int sendWithReplyAsync(const QDBusMessage &message, QObject *receiver, - const char *returnMethod, const char *errorMethod, int timeout = -1); + QDBusPendingCallPrivate *sendWithReplyAsync(const QDBusMessage &message, QObject *receiver, + const char *returnMethod, const char *errorMethod,int timeout = -1); bool connectSignal(const QString &service, const QString &path, const QString& interface, const QString &name, const QStringList &argumentMatch, const QString &signature, QObject *receiver, const char *slot); diff --git a/src/dbus/qdbusintegrator.cpp b/src/dbus/qdbusintegrator.cpp index 4b9bfc3fc8..3f25f02bee 100644 --- a/src/dbus/qdbusintegrator.cpp +++ b/src/dbus/qdbusintegrator.cpp @@ -1901,6 +1901,9 @@ void QDBusConnectionPrivate::processFinishedCall(QDBusPendingCallPrivate *call) if (msg.type() == QDBusMessage::ErrorMessage) emit connection->callWithCallbackFailed(QDBusError(msg), call->sentMessage); + + if (!call->ref.deref()) + delete call; } int QDBusConnectionPrivate::send(const QDBusMessage& message) @@ -1983,7 +1986,7 @@ QDBusMessage QDBusConnectionPrivate::sendWithReply(const QDBusMessage &message, return amsg; } else { // use the event loop - QDBusPendingCallPrivate *pcall = sendWithReplyAsync(message, timeout); + QDBusPendingCallPrivate *pcall = sendWithReplyAsync(message, 0, 0, 0, timeout); Q_ASSERT(pcall); if (pcall->replyMessage.type() == QDBusMessage::InvalidMessage) { @@ -1999,6 +2002,10 @@ QDBusMessage QDBusConnectionPrivate::sendWithReply(const QDBusMessage &message, QDBusMessage reply = pcall->replyMessage; lastError = QDBusError(reply); // set or clear error + bool r = pcall->ref.deref(); + Q_ASSERT(!r); + Q_UNUSED(r); + delete pcall; return reply; } @@ -2038,19 +2045,55 @@ QDBusMessage QDBusConnectionPrivate::sendWithReplyLocal(const QDBusMessage &mess } QDBusPendingCallPrivate *QDBusConnectionPrivate::sendWithReplyAsync(const QDBusMessage &message, - int timeout) + QObject *receiver, const char *returnMethod, + const char *errorMethod, int timeout) { if (isServiceRegisteredByThread(message.service())) { // special case for local calls QDBusPendingCallPrivate *pcall = new QDBusPendingCallPrivate(message, this); pcall->replyMessage = sendWithReplyLocal(message); + if (receiver && returnMethod) + pcall->setReplyCallback(receiver, returnMethod); + + if (errorMethod) { + pcall->watcherHelper = new QDBusPendingCallWatcherHelper; + connect(pcall->watcherHelper, SIGNAL(error(QDBusError,QDBusMessage)), receiver, errorMethod, + Qt::QueuedConnection); + pcall->watcherHelper->moveToThread(thread()); + } + if ((receiver && returnMethod) || errorMethod) { + // no one waiting, will delete pcall in processFinishedCall() + pcall->ref.store(1); + } else { + // set double ref to prevent race between processFinishedCall() and ref counting + // by QDBusPendingCall::QExplicitlySharedDataPointer + pcall->ref.store(2); + } + processFinishedCall(pcall); return pcall; } checkThread(); QDBusPendingCallPrivate *pcall = new QDBusPendingCallPrivate(message, this); - pcall->ref.store(0); + if (receiver && returnMethod) + pcall->setReplyCallback(receiver, returnMethod); + + if (errorMethod) { + pcall->watcherHelper = new QDBusPendingCallWatcherHelper; + connect(pcall->watcherHelper, SIGNAL(error(QDBusError,QDBusMessage)), receiver, errorMethod, + Qt::QueuedConnection); + pcall->watcherHelper->moveToThread(thread()); + } + + if ((receiver && returnMethod) || errorMethod) { + // no one waiting, will delete pcall in processFinishedCall() + pcall->ref.store(1); + } else { + // set double ref to prevent race between processFinishedCall() and ref counting + // by QDBusPendingCall::QExplicitlySharedDataPointer + pcall->ref.store(2); + } QDBusError error; DBusMessage *msg = QDBusMessagePrivate::toDBusMessage(message, capabilities, &error); @@ -2061,6 +2104,7 @@ QDBusPendingCallPrivate *QDBusConnectionPrivate::sendWithReplyAsync(const QDBusM qPrintable(error.message())); pcall->replyMessage = QDBusMessage::createError(error); lastError = error; + processFinishedCall(pcall); return pcall; } @@ -2086,45 +2130,10 @@ QDBusPendingCallPrivate *QDBusConnectionPrivate::sendWithReplyAsync(const QDBusM q_dbus_message_unref(msg); pcall->replyMessage = QDBusMessage::createError(error); + processFinishedCall(pcall); return pcall; } -int QDBusConnectionPrivate::sendWithReplyAsync(const QDBusMessage &message, QObject *receiver, - const char *returnMethod, const char *errorMethod, - int timeout) -{ - QDBusPendingCallPrivate *pcall = sendWithReplyAsync(message, timeout); - Q_ASSERT(pcall); - - // has it already finished with success (dispatched locally)? - if (pcall->replyMessage.type() == QDBusMessage::ReplyMessage) { - pcall->setReplyCallback(receiver, returnMethod); - processFinishedCall(pcall); - delete pcall; - return 1; - } - - // either it hasn't finished or it has finished with error - if (errorMethod) { - pcall->watcherHelper = new QDBusPendingCallWatcherHelper; - connect(pcall->watcherHelper, SIGNAL(error(QDBusError,QDBusMessage)), receiver, errorMethod, - Qt::QueuedConnection); - pcall->watcherHelper->moveToThread(thread()); - } - - // has it already finished and is an error reply message? - if (pcall->replyMessage.type() == QDBusMessage::ErrorMessage) { - processFinishedCall(pcall); - delete pcall; - return 1; - } - - pcall->ref.ref(); - pcall->setReplyCallback(receiver, returnMethod); - - return 1; -} - bool QDBusConnectionPrivate::connectSignal(const QString &service, const QString &path, const QString &interface, const QString &name, const QStringList &argumentMatch, const QString &signature, diff --git a/src/dbus/qdbuspendingcall.cpp b/src/dbus/qdbuspendingcall.cpp index 0b4ff3a397..49f9fc0cd8 100644 --- a/src/dbus/qdbuspendingcall.cpp +++ b/src/dbus/qdbuspendingcall.cpp @@ -256,6 +256,11 @@ QDBusPendingCall::QDBusPendingCall(const QDBusPendingCall &other) QDBusPendingCall::QDBusPendingCall(QDBusPendingCallPrivate *dd) : d(dd) { + if (dd) { + bool r = dd->ref.deref(); + Q_ASSERT(r); + Q_UNUSED(r); + } } /*! @@ -469,6 +474,7 @@ QDBusPendingCall QDBusPendingCall::fromCompletedCall(const QDBusMessage &msg) msg.type() == QDBusMessage::ReplyMessage) { d = new QDBusPendingCallPrivate(QDBusMessage(), 0); d->replyMessage = msg; + d->ref.store(1); } return QDBusPendingCall(d); -- cgit v1.2.3 From 5d192deed9ef7289e44cd936576d98b3afa2601a Mon Sep 17 00:00:00 2001 From: Albert Astals Cid Date: Fri, 26 Jul 2013 17:11:19 +0200 Subject: Fix crash on qdbus when return type is a raw dbus type Assigning a -1 to type is going to make things crash since it basically means unresolved and when you try to access the string data go to a index that doesn't exist So what I do is save the return type in rawReturnType if it is a raw one and reassign the type to IsUnresolvedType | strings.enter(mm.rawReturnType) instead of -1 when "saving" the metaobject Task-number: QTBUG-32671 Change-Id: I67898dea8a1926eee80c16417e877ef4e22aa06b Reviewed-by: Thiago Macieira --- src/dbus/qdbusmetaobject.cpp | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) (limited to 'src/dbus') diff --git a/src/dbus/qdbusmetaobject.cpp b/src/dbus/qdbusmetaobject.cpp index 799c66f6f9..127cf6658c 100644 --- a/src/dbus/qdbusmetaobject.cpp +++ b/src/dbus/qdbusmetaobject.cpp @@ -75,6 +75,7 @@ private: QByteArray name; QVarLengthArray inputTypes; QVarLengthArray outputTypes; + QByteArray rawReturnType; int flags; }; @@ -276,6 +277,9 @@ void QDBusMetaObjectGenerator::parseMethods() mm.outputTypes.append(type.id); + if (i == 0 && type.id == -1) { + mm.rawReturnType = type.name; + } if (i != 0) { // non-const ref parameter mm.parameterNames.append(arg.name.toLatin1()); @@ -471,10 +475,14 @@ void QDBusMetaObjectGenerator::write(QDBusMetaObject *obj) int type; QByteArray typeName; if (i < 0) { // Return type - if (!mm.outputTypes.isEmpty()) + if (!mm.outputTypes.isEmpty()) { type = mm.outputTypes.first(); - else + if (type == -1) { + type = IsUnresolvedType | strings.enter(mm.rawReturnType); + } + } else { type = QMetaType::Void; + } } else if (i < mm.inputTypes.size()) { type = mm.inputTypes.at(i); } else { -- cgit v1.2.3 From c3f485c5250a503832e767e1fe5e40595126f6c5 Mon Sep 17 00:00:00 2001 From: Albert Astals Cid Date: Thu, 25 Jul 2013 17:06:16 +0200 Subject: Expose invokables that are not slots in the xml Without it the invocations were working but were not listed on introspection Change-Id: Ie62f7dc3577f52b6888ddebf0392fdf51f2845d5 Reviewed-by: Thiago Macieira --- src/dbus/qdbusxmlgenerator.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) (limited to 'src/dbus') diff --git a/src/dbus/qdbusxmlgenerator.cpp b/src/dbus/qdbusxmlgenerator.cpp index 8c822162e4..bf5e24cd5f 100644 --- a/src/dbus/qdbusxmlgenerator.cpp +++ b/src/dbus/qdbusxmlgenerator.cpp @@ -209,12 +209,13 @@ static QString generateInterfaceXml(const QMetaObject *mo, int flags, int method } int wantedMask; + const bool isSlot = mm.methodType() == QMetaMethod::Slot; if (isScriptable) wantedMask = isSignal ? QDBusConnection::ExportScriptableSignals - : QDBusConnection::ExportScriptableSlots; + : isSlot ? QDBusConnection::ExportScriptableSlots : QDBusConnection::ExportScriptableInvokables; else wantedMask = isSignal ? QDBusConnection::ExportNonScriptableSignals - : QDBusConnection::ExportNonScriptableSlots; + : isSlot ? QDBusConnection::ExportNonScriptableSlots : QDBusConnection::ExportNonScriptableInvokables; if ((flags & wantedMask) != wantedMask) continue; -- cgit v1.2.3 From 23214c815ec267e999015a971768dbf8f6ea0957 Mon Sep 17 00:00:00 2001 From: Albert Astals Cid Date: Wed, 7 Aug 2013 20:19:09 +0200 Subject: Revert c3f485c5250a503832e767e1fe5e40595126f6c5 It has been discovered it changes the behavior of qdbuscpp2xml resulting in builds of some apps breaking. Even if the behavior is more correct, such behavior change in a stable branch is not acceptable Change-Id: I1d79104ebf11c3f48c84f109be2926af96cddae7 Reviewed-by: Thiago Macieira --- src/dbus/qdbusxmlgenerator.cpp | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) (limited to 'src/dbus') diff --git a/src/dbus/qdbusxmlgenerator.cpp b/src/dbus/qdbusxmlgenerator.cpp index bf5e24cd5f..8c822162e4 100644 --- a/src/dbus/qdbusxmlgenerator.cpp +++ b/src/dbus/qdbusxmlgenerator.cpp @@ -209,13 +209,12 @@ static QString generateInterfaceXml(const QMetaObject *mo, int flags, int method } int wantedMask; - const bool isSlot = mm.methodType() == QMetaMethod::Slot; if (isScriptable) wantedMask = isSignal ? QDBusConnection::ExportScriptableSignals - : isSlot ? QDBusConnection::ExportScriptableSlots : QDBusConnection::ExportScriptableInvokables; + : QDBusConnection::ExportScriptableSlots; else wantedMask = isSignal ? QDBusConnection::ExportNonScriptableSignals - : isSlot ? QDBusConnection::ExportNonScriptableSlots : QDBusConnection::ExportNonScriptableInvokables; + : QDBusConnection::ExportNonScriptableSlots; if ((flags & wantedMask) != wantedMask) continue; -- cgit v1.2.3