From d438be92dd7068fef94ce98e1ec039fe0ef4f3b3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bernhard=20=C3=9Cbelacker?= Date: Sun, 12 Feb 2017 14:10:43 +0100 Subject: Avoid access to declarativeData when isDeletingChildren is set QObject's members declarativeData and currentChildBeingDeleted share the same memory because they are inside a union. This leads to a problem when destructing mixed Widgets and QML objects. Then in QObjectPrivate::deleteChildren the member currentChildBeingDeleted is set. But unfortunatley QObjectWrapper::destroyObject retrieves the same pointer via declarativeData. This patch should avoid this by disallowing retrieval of declarativeData when isDeletingChildren is set (or at least adds a Q_ASSERT). Task-number: QTBUG-57714 Change-Id: I9ee02f79be3e8226c30076c24859b49b8dcfaecf Reviewed-by: Simon Hausmann Reviewed-by: Eirik Aavitsland --- src/qml/jsruntime/qv4qobjectwrapper_p.h | 11 ++++------- src/qml/qml/qqmldata_p.h | 8 +++++--- src/qml/qml/qqmlengine.cpp | 30 ++++++++++-------------------- src/qml/qml/qqmlobjectcreator.cpp | 4 +++- src/qml/qml/qqmlproperty.cpp | 2 +- src/qml/types/qqmldelegatemodel.cpp | 11 ++++------- 6 files changed, 27 insertions(+), 39 deletions(-) diff --git a/src/qml/jsruntime/qv4qobjectwrapper_p.h b/src/qml/jsruntime/qv4qobjectwrapper_p.h index c7c4f4dd77..3764943499 100644 --- a/src/qml/jsruntime/qv4qobjectwrapper_p.h +++ b/src/qml/jsruntime/qv4qobjectwrapper_p.h @@ -209,13 +209,10 @@ inline ReturnedValue QObjectWrapper::wrap(ExecutionEngine *engine, QObject *obje if (Q_UNLIKELY(QQmlData::wasDeleted(object))) return QV4::Encode::null(); - QObjectPrivate *priv = QObjectPrivate::get(const_cast(object)); - if (Q_LIKELY(priv->declarativeData)) { - auto ddata = static_cast(priv->declarativeData); - if (Q_LIKELY(ddata->jsEngineId == engine->m_engineId && !ddata->jsWrapper.isUndefined())) { - // We own the JS object - return ddata->jsWrapper.value(); - } + auto ddata = QQmlData::get(object); + if (Q_LIKELY(ddata && ddata->jsEngineId == engine->m_engineId && !ddata->jsWrapper.isUndefined())) { + // We own the JS object + return ddata->jsWrapper.value(); } return wrap_slowPath(engine, object); diff --git a/src/qml/qml/qqmldata_p.h b/src/qml/qml/qqmldata_p.h index e271598c2d..2083326cd5 100644 --- a/src/qml/qml/qqmldata_p.h +++ b/src/qml/qml/qqmldata_p.h @@ -201,7 +201,9 @@ public: static QQmlData *get(const QObject *object, bool create = false) { QObjectPrivate *priv = QObjectPrivate::get(const_cast(object)); - if (priv->wasDeleted) { + // If QObjectData::isDeletingChildren is set then access to QObjectPrivate::declarativeData has + // to be avoided because QObjectPrivate::currentChildBeingDeleted is in use. + if (priv->isDeletingChildren || priv->wasDeleted) { Q_ASSERT(!create); return 0; } else if (priv->declarativeData) { @@ -269,8 +271,8 @@ bool QQmlData::wasDeleted(QObject *object) if (!priv || priv->wasDeleted) return true; - return priv->declarativeData && - static_cast(priv->declarativeData)->isQueuedForDeletion; + QQmlData *ddata = QQmlData::get(object); + return ddata && ddata->isQueuedForDeletion; } QQmlNotifierEndpoint *QQmlData::notify(int index) diff --git a/src/qml/qml/qqmlengine.cpp b/src/qml/qml/qqmlengine.cpp index f1c592b632..1eb892a0de 100644 --- a/src/qml/qml/qqmlengine.cpp +++ b/src/qml/qml/qqmlengine.cpp @@ -692,9 +692,7 @@ QQmlEnginePrivate::~QQmlEnginePrivate() void QQmlPrivate::qdeclarativeelement_destructor(QObject *o) { - QObjectPrivate *p = QObjectPrivate::get(o); - if (p->declarativeData) { - QQmlData *d = static_cast(p->declarativeData); + if (QQmlData *d = QQmlData::get(o)) { if (d->ownContext && d->context) { d->context->destroy(); d->context = 0; @@ -864,13 +862,10 @@ void QQmlData::markAsDeleted(QObject *o) void QQmlData::setQueuedForDeletion(QObject *object) { if (object) { - if (QObjectPrivate *priv = QObjectPrivate::get(object)) { - if (!priv->wasDeleted && priv->declarativeData) { - QQmlData *ddata = QQmlData::get(object, false); - if (ddata->ownContext && ddata->context) - ddata->context->emitDestruction(); - ddata->isQueuedForDeletion = true; - } + if (QQmlData *ddata = QQmlData::get(object)) { + if (ddata->ownContext && ddata->context) + ddata->context->emitDestruction(); + ddata->isQueuedForDeletion = true; } } } @@ -1319,17 +1314,11 @@ QQmlContext *QQmlEngine::contextForObject(const QObject *object) if(!object) return 0; - QObjectPrivate *priv = QObjectPrivate::get(const_cast(object)); - - QQmlData *data = - static_cast(priv->declarativeData); - - if (!data) - return 0; - else if (data->outerContext) + QQmlData *data = QQmlData::get(object); + if (data && data->outerContext) return data->outerContext->asQQmlContext(); - else - return 0; + + return 0; } /*! @@ -1864,6 +1853,7 @@ void QQmlData::setPendingBindingBit(QObject *obj, int coreIndex) QQmlData *QQmlData::createQQmlData(QObjectPrivate *priv) { Q_ASSERT(priv); + Q_ASSERT(!priv->isDeletingChildren); priv->declarativeData = new QQmlData; return static_cast(priv->declarativeData); } diff --git a/src/qml/qml/qqmlobjectcreator.cpp b/src/qml/qml/qqmlobjectcreator.cpp index 09936f6e7a..3ed3ce5460 100644 --- a/src/qml/qml/qqmlobjectcreator.cpp +++ b/src/qml/qml/qqmlobjectcreator.cpp @@ -1075,7 +1075,9 @@ QObject *QQmlObjectCreator::createInstance(int index, QObject *parent, bool isCo { QQmlData *ddata = new (ddataMemory) QQmlData; ddata->ownMemory = false; - QObjectPrivate::get(instance)->declarativeData = ddata; + QObjectPrivate* p = QObjectPrivate::get(instance); + Q_ASSERT(!p->isDeletingChildren); + p->declarativeData = ddata; } const int parserStatusCast = type->parserStatusCast(); diff --git a/src/qml/qml/qqmlproperty.cpp b/src/qml/qml/qqmlproperty.cpp index 7df8336f51..cec04d25a1 100644 --- a/src/qml/qml/qqmlproperty.cpp +++ b/src/qml/qml/qqmlproperty.cpp @@ -1628,7 +1628,7 @@ QMetaMethod QQmlPropertyPrivate::findSignalByName(const QMetaObject *mo, const Q */ static inline void flush_vme_signal(const QObject *object, int index, bool indexInSignalRange) { - QQmlData *data = static_cast(QObjectPrivate::get(const_cast(object))->declarativeData); + QQmlData *data = QQmlData::get(object); if (data && data->propertyCache) { QQmlPropertyData *property = indexInSignalRange ? data->propertyCache->signal(index) : data->propertyCache->method(index); diff --git a/src/qml/types/qqmldelegatemodel.cpp b/src/qml/types/qqmldelegatemodel.cpp index d9a8b1d179..62a9c40c07 100644 --- a/src/qml/types/qqmldelegatemodel.cpp +++ b/src/qml/types/qqmldelegatemodel.cpp @@ -1959,9 +1959,8 @@ void QQmlDelegateModelItem::destroyObject() Q_ASSERT(object); Q_ASSERT(contextData); - QObjectPrivate *p = QObjectPrivate::get(object); - Q_ASSERT(p->declarativeData); - QQmlData *data = static_cast(p->declarativeData); + QQmlData *data = QQmlData::get(object); + Q_ASSERT(data); if (data->ownContext && data->context) data->context->clearContext(); object->deleteLater(); @@ -1978,10 +1977,8 @@ void QQmlDelegateModelItem::destroyObject() QQmlDelegateModelItem *QQmlDelegateModelItem::dataForObject(QObject *object) { - QObjectPrivate *p = QObjectPrivate::get(object); - QQmlContextData *context = p->declarativeData - ? static_cast(p->declarativeData)->context - : 0; + QQmlData *d = QQmlData::get(object); + QQmlContextData *context = d ? d->context : 0; for (context = context ? context->parent : 0; context; context = context->parent) { if (QQmlDelegateModelItem *cacheItem = qobject_cast( context->contextObject)) { -- cgit v1.2.3