From b67cc148693de06370633cddf82a31664004e65c Mon Sep 17 00:00:00 2001 From: Mitch Curtis Date: Wed, 27 May 2020 10:22:37 +0200 Subject: StackView: fix heap-use-after-free when pushing after clear This patch extends the work done in aaec25a7 to cover all operations. Note also that b94889f4 does a similar thing to this patch and aaec25a7, in that it explicitly ignores operations that are done during the removal of elements. Fixes: QTBUG-84381 Pick-to: 5.15 Change-Id: Id8bbbded39d8e58bcf0e8eedeb2dde794952333f Reviewed-by: Richard Moe Gustavsen --- src/quicktemplates2/qquickstackview.cpp | 37 +++++++++++++++++++---------- src/quicktemplates2/qquickstackview_p.cpp | 6 +++++ src/quicktemplates2/qquickstackview_p_p.h | 3 ++- tests/auto/controls/data/tst_stackview.qml | 38 +++++++++++++++++++++++++----- 4 files changed, 64 insertions(+), 20 deletions(-) diff --git a/src/quicktemplates2/qquickstackview.cpp b/src/quicktemplates2/qquickstackview.cpp index 6455731d..e5e2c39b 100644 --- a/src/quicktemplates2/qquickstackview.cpp +++ b/src/quicktemplates2/qquickstackview.cpp @@ -558,7 +558,14 @@ QQuickItem *QQuickStackView::find(const QJSValue &callback, LoadBehavior behavio void QQuickStackView::push(QQmlV4Function *args) { Q_D(QQuickStackView); - QScopedValueRollback rollback(d->operation, QStringLiteral("push")); + const QString operationName = QStringLiteral("push"); + if (d->modifyingElements) { + d->warnOfInterruption(operationName); + return; + } + + QScopedValueRollback modifyingElements(d->modifyingElements, true); + QScopedValueRollback operationNameRollback(d->operation, operationName); if (args->length() <= 0) { d->warn(QStringLiteral("missing arguments")); args->setReturnValue(QV4::Encode::null()); @@ -651,14 +658,15 @@ void QQuickStackView::push(QQmlV4Function *args) void QQuickStackView::pop(QQmlV4Function *args) { Q_D(QQuickStackView); - if (d->removingElements) { - d->warn(QStringLiteral("cannot pop while already in the process of removing elements")); + const QString operationName = QStringLiteral("pop"); + if (d->modifyingElements) { + d->warnOfInterruption(operationName); args->setReturnValue(QV4::Encode::null()); return; } - QScopedValueRollback removingElements(d->removingElements, true); - QScopedValueRollback rollback(d->operation, QStringLiteral("pop")); + QScopedValueRollback modifyingElements(d->modifyingElements, true); + QScopedValueRollback operationNameRollback(d->operation, operationName); int argc = args->length(); if (d->elements.count() <= 1 || argc > 2) { if (argc > 2) @@ -813,14 +821,15 @@ void QQuickStackView::pop(QQmlV4Function *args) void QQuickStackView::replace(QQmlV4Function *args) { Q_D(QQuickStackView); - if (d->removingElements) { - d->warn(QStringLiteral("cannot replace while already in the process of removing elements")); + const QString operationName = QStringLiteral("replace"); + if (d->modifyingElements) { + d->warnOfInterruption(operationName); args->setReturnValue(QV4::Encode::null()); return; } - QScopedValueRollback removingElements(d->removingElements, true); - QScopedValueRollback rollback(d->operation, QStringLiteral("replace")); + QScopedValueRollback modifyingElements(d->modifyingElements, true); + QScopedValueRollback operationNameRollback(d->operation, operationName); if (args->length() <= 0) { d->warn(QStringLiteral("missing arguments")); args->setReturnValue(QV4::Encode::null()); @@ -916,12 +925,14 @@ void QQuickStackView::clear(Operation operation) if (d->elements.isEmpty()) return; - if (d->removingElements) { - d->warn(QStringLiteral("cannot clear while already in the process of removing elements")); + const QString operationName = QStringLiteral("clear"); + if (d->modifyingElements) { + d->warnOfInterruption(operationName); return; } - QScopedValueRollback removingElements(d->removingElements, true); + QScopedValueRollback modifyingElements(d->modifyingElements, true); + QScopedValueRollback operationNameRollback(d->operation, operationName); if (operation != Immediate) { QQuickStackElement *exit = d->elements.pop(); exit->removal = true; @@ -1128,7 +1139,7 @@ void QQuickStackView::componentComplete() QQuickControl::componentComplete(); Q_D(QQuickStackView); - QScopedValueRollback rollback(d->operation, QStringLiteral("initialItem")); + QScopedValueRollback operationNameRollback(d->operation, QStringLiteral("initialItem")); QQuickStackElement *element = nullptr; QString error; int oldDepth = d->elements.count(); diff --git a/src/quicktemplates2/qquickstackview_p.cpp b/src/quicktemplates2/qquickstackview_p.cpp index 1655ff36..ce81a929 100644 --- a/src/quicktemplates2/qquickstackview_p.cpp +++ b/src/quicktemplates2/qquickstackview_p.cpp @@ -56,6 +56,12 @@ void QQuickStackViewPrivate::warn(const QString &error) qmlWarning(q) << operation << ": " << error; } +void QQuickStackViewPrivate::warnOfInterruption(const QString &attemptedOperation) +{ + Q_Q(QQuickStackView); + qmlWarning(q) << "cannot " << attemptedOperation << " while already in the process of completing a " << operation; +} + void QQuickStackViewPrivate::setCurrentItem(QQuickStackElement *element) { Q_Q(QQuickStackView); diff --git a/src/quicktemplates2/qquickstackview_p_p.h b/src/quicktemplates2/qquickstackview_p_p.h index b76a033c..0c64019d 100644 --- a/src/quicktemplates2/qquickstackview_p_p.h +++ b/src/quicktemplates2/qquickstackview_p_p.h @@ -73,6 +73,7 @@ public: } void warn(const QString &error); + void warnOfInterruption(const QString &attemptedOperation); void setCurrentItem(QQuickStackElement *element); @@ -94,7 +95,7 @@ public: void depthChange(int newDepth, int oldDepth); bool busy = false; - bool removingElements = false; + bool modifyingElements = false; QString operation; QJSValue initialItem; QQuickItem *currentItem = nullptr; diff --git a/tests/auto/controls/data/tst_stackview.qml b/tests/auto/controls/data/tst_stackview.qml index c15ce8ea..767ec818 100644 --- a/tests/auto/controls/data/tst_stackview.qml +++ b/tests/auto/controls/data/tst_stackview.qml @@ -804,7 +804,10 @@ TestCase { var item = control.push(component, StackView.Immediate) verify(item) - item.StackView.onRemoved.connect(function() { control.push(component, StackView.Immediate) } ) + item.StackView.onRemoved.connect(function() { + ignoreWarning(/.*QML StackView: cannot push while already in the process of completing a pop/) + control.push(component, StackView.Immediate) + }) // don't crash (QTBUG-62153) control.pop(StackView.Immediate) @@ -1287,7 +1290,7 @@ TestCase { control.push(container.clearUponDestructionComponent, StackView.Immediate) // Shouldn't crash. - ignoreWarning(new RegExp(".*cannot clear while already in the process of removing elements")) + ignoreWarning(/.*cannot clear while already in the process of completing a clear/) control.clear(StackView.Immediate) } @@ -1301,7 +1304,7 @@ TestCase { // Pop all items except the first, removing the second item we pushed in the process. // Shouldn't crash. - ignoreWarning(new RegExp(".*cannot clear while already in the process of removing elements")) + ignoreWarning(/.*cannot clear while already in the process of completing a pop/) control.pop(null, StackView.Immediate) } @@ -1316,7 +1319,7 @@ TestCase { control.push(container.clearUponDestructionComponent, StackView.Immediate) // Pop the top item, then pop down to the first item in response. - ignoreWarning(new RegExp(".*cannot pop while already in the process of removing elements")) + ignoreWarning(/.*cannot pop while already in the process of completing a pop/) control.pop(StackView.Immediate) } @@ -1329,7 +1332,7 @@ TestCase { control.push(container.clearUponDestructionComponent, StackView.Immediate) // Replace the top item, then clear in response. - ignoreWarning(new RegExp(".*cannot clear while already in the process of removing elements")) + ignoreWarning(/.*cannot clear while already in the process of completing a replace/) control.replace(component, StackView.Immediate) } @@ -1342,7 +1345,7 @@ TestCase { control.push(container.clearUponDestructionComponent, StackView.Immediate) // Replace the top item, then clear in response. - ignoreWarning(new RegExp(".*cannot replace while already in the process of removing elements")) + ignoreWarning(/.*cannot replace while already in the process of completing a clear/) control.clear(StackView.Immediate) } @@ -1404,4 +1407,27 @@ TestCase { tryCompare(control, "busy", false) compare(redRect.visible, true) } + + // QTBUG-84381 + function test_clearAndPushAfterDepthChange() { + var control = createTemporaryObject(stackView, testCase, { + popEnter: null, popExit: null, pushEnter: null, + pushExit: null, replaceEnter: null, replaceExit: null + }) + verify(control) + + control.depthChanged.connect(function() { + if (control.depth === 2) { + // Shouldn't assert. + ignoreWarning(/.*QML StackView: cannot clear while already in the process of completing a push/) + control.clear() + // Shouldn't crash. + ignoreWarning(/.*QML StackView: cannot push while already in the process of completing a push/) + control.push(component) + } + }) + + control.push(component) + control.push(component) + } } -- cgit v1.2.3