From 984bc7cc3e53e1a9003209858f89bd7fcbd5f81c Mon Sep 17 00:00:00 2001 From: Fabian Kosmale Date: Fri, 29 Jan 2021 13:29:10 +0100 Subject: Address thread safety issues in QProperty classes While we do not support cross-thread bindings, reading of properties from a different thread when no bindings are involved must continue to work. However the check whether bindings are involved used the QBindingStatus in the QObjectPrivate. That one contains the wrong value when the QObject is accessed from a different thread than the one it has affinity with. This patch reads from the thread_local directly instead to sidetstep the issue. Change-Id: I8ce2092f35e210566934e2439beb5d48fd8cf226 Reviewed-by: Fabian Kosmale Reviewed-by: Lars Knoll Reviewed-by: Qt CI Bot --- src/corelib/kernel/qproperty.cpp | 14 ++++++++++++-- src/corelib/kernel/qproperty.h | 2 +- src/corelib/kernel/qproperty_p.h | 10 +++++++--- 3 files changed, 20 insertions(+), 6 deletions(-) diff --git a/src/corelib/kernel/qproperty.cpp b/src/corelib/kernel/qproperty.cpp index ad25492283..1c642562a8 100644 --- a/src/corelib/kernel/qproperty.cpp +++ b/src/corelib/kernel/qproperty.cpp @@ -1830,11 +1830,14 @@ void QBindingStorage::maybeUpdateBindingAndRegister_helper(const QUntypedPropert void QBindingStorage::registerDependency_helper(const QUntypedPropertyData *data) const { Q_ASSERT(bindingStatus); + // Use ::bindingStatus to get the binding from TLS. This is required, so that reads from + // another thread do not register as dependencies + auto *currentBinding = QT_PREPEND_NAMESPACE(bindingStatus).currentlyEvaluatingBinding; QUntypedPropertyData *dd = const_cast(data); - auto storage = QBindingStoragePrivate(d).get(dd, /*create=*/ bindingStatus->currentlyEvaluatingBinding != nullptr); + auto storage = QBindingStoragePrivate(d).get(dd, /*create=*/ currentBinding != nullptr); if (!storage) return; - storage->registerWithCurrentlyEvaluatingBinding(bindingStatus->currentlyEvaluatingBinding); + storage->registerWithCurrentlyEvaluatingBinding(currentBinding); } @@ -1874,6 +1877,13 @@ bool isAnyBindingEvaluating() { return bindingStatus.currentlyEvaluatingBinding != nullptr; } + +bool isPropertyInBindingWrapper(const QUntypedPropertyData *property) +{ + return bindingStatus.currentCompatProperty && + bindingStatus.currentCompatProperty->property == property; +} + } // namespace QtPrivate end QT_END_NAMESPACE diff --git a/src/corelib/kernel/qproperty.h b/src/corelib/kernel/qproperty.h index 40b7a9452d..1c557fbace 100644 --- a/src/corelib/kernel/qproperty.h +++ b/src/corelib/kernel/qproperty.h @@ -845,7 +845,7 @@ public: void registerDependency(const QUntypedPropertyData *data) const { - if (!d && !bindingStatus->currentlyEvaluatingBinding) + if (!bindingStatus->currentlyEvaluatingBinding) return; registerDependency_helper(data); } diff --git a/src/corelib/kernel/qproperty_p.h b/src/corelib/kernel/qproperty_p.h index 00978e8ea6..7ac02cb43d 100644 --- a/src/corelib/kernel/qproperty_p.h +++ b/src/corelib/kernel/qproperty_p.h @@ -368,6 +368,10 @@ inline QPropertyObserverPointer QPropertyBindingDataPointer::firstObserver() con return { reinterpret_cast(ptr->d_ptr) }; } +namespace QtPrivate { + Q_CORE_EXPORT bool isPropertyInBindingWrapper(const QUntypedPropertyData *property); +} + template class QObjectCompatProperty : public QPropertyData { @@ -397,10 +401,10 @@ class QObjectCompatProperty : public QPropertyData (thisData->owner()->*Setter)(copy.valueBypassingBindings()); return true; } - inline bool inBindingWrapper(const QBindingStorage *storage) const + bool inBindingWrapper(const QBindingStorage *storage) const { - return storage->bindingStatus->currentCompatProperty && - storage->bindingStatus->currentCompatProperty->property == this; + return storage->bindingStatus->currentCompatProperty + && QtPrivate::isPropertyInBindingWrapper(this); } public: -- cgit v1.2.3