From 62bf7be90ba5d7868cc18c8a1e81209c91f449c5 Mon Sep 17 00:00:00 2001 From: Ulf Hermann Date: Fri, 5 Feb 2021 13:18:25 +0100 Subject: QSequentialAnimationGroupJob: Protect against self-deletion setCurrentAnimation() can indirectly delete the animation group job itself by invoking the animation controller. Use the RETURN_IF_DELETED mechanism to avoid the resulting dangling pointers. Task-number: QTBUG-90401 Change-Id: Ibd0ad21e8d3af4760604c3ff37dc46101d5f49ad Reviewed-by: Fabian Kosmale (cherry picked from commit 680f28b08f65ad38c8d5498b5738231b2a2779a3) Reviewed-by: Qt Cherry-pick Bot --- src/qml/animations/qanimationjobutil_p.h | 4 +-- .../animations/qsequentialanimationgroupjob.cpp | 14 ++++---- .../tst_qsequentialanimationgroupjob.cpp | 42 +++++++++++++++++++++- 3 files changed, 50 insertions(+), 10 deletions(-) diff --git a/src/qml/animations/qanimationjobutil_p.h b/src/qml/animations/qanimationjobutil_p.h index 83cf3b246f..2b7bda3123 100644 --- a/src/qml/animations/qanimationjobutil_p.h +++ b/src/qml/animations/qanimationjobutil_p.h @@ -70,7 +70,7 @@ struct SelfDeletable { // \param func statements or functions that to be executed under test. // \param action post process if p was deleted under test. #define ACTION_IF_DELETED(p, func, action) \ -{ \ +do { \ static_assert(std::is_samem_selfDeletable), SelfDeletable>::value, "m_selfDeletable must be SelfDeletable");\ bool *prevWasDeleted = (p)->m_selfDeletable.m_wasDeleted; \ bool wasDeleted = false; \ @@ -82,7 +82,7 @@ struct SelfDeletable { {action;} \ } \ (p)->m_selfDeletable.m_wasDeleted = prevWasDeleted; \ -} +} while (false) #define RETURN_IF_DELETED(func) \ ACTION_IF_DELETED(this, func, return) diff --git a/src/qml/animations/qsequentialanimationgroupjob.cpp b/src/qml/animations/qsequentialanimationgroupjob.cpp index dc57444b32..1d19bbf79d 100644 --- a/src/qml/animations/qsequentialanimationgroupjob.cpp +++ b/src/qml/animations/qsequentialanimationgroupjob.cpp @@ -338,7 +338,7 @@ void QSequentialAnimationGroupJob::uncontrolledAnimationFinished(QAbstractAnimat if (m_direction == Forward) { // set the current animation to be the next one if (m_currentAnimation->nextSibling()) - setCurrentAnimation(m_currentAnimation->nextSibling()); + RETURN_IF_DELETED(setCurrentAnimation(m_currentAnimation->nextSibling())); for (QAbstractAnimationJob *a = animation->nextSibling(); a; a = a->nextSibling()) { int dur = a->duration(); @@ -353,7 +353,7 @@ void QSequentialAnimationGroupJob::uncontrolledAnimationFinished(QAbstractAnimat } else { // set the current animation to be the previous one if (m_currentAnimation->previousSibling()) - setCurrentAnimation(m_currentAnimation->previousSibling()); + RETURN_IF_DELETED(setCurrentAnimation(m_currentAnimation->previousSibling())); for (QAbstractAnimationJob *a = animation->previousSibling(); a; a = a->previousSibling()) { int dur = a->duration(); @@ -374,12 +374,12 @@ void QSequentialAnimationGroupJob::uncontrolledAnimationFinished(QAbstractAnimat void QSequentialAnimationGroupJob::animationInserted(QAbstractAnimationJob *anim) { if (m_currentAnimation == nullptr) - setCurrentAnimation(firstChild()); // initialize the current animation + RETURN_IF_DELETED(setCurrentAnimation(firstChild())); // initialize the current animation if (m_currentAnimation == anim->nextSibling() && m_currentAnimation->currentTime() == 0 && m_currentAnimation->currentLoop() == 0) { //in this case we simply insert the animation before the current one has actually started - setCurrentAnimation(anim); + RETURN_IF_DELETED(setCurrentAnimation(anim)); } // TODO @@ -398,11 +398,11 @@ void QSequentialAnimationGroupJob::animationRemoved(QAbstractAnimationJob *anim, bool removingCurrent = anim == m_currentAnimation; if (removingCurrent) { if (next) - setCurrentAnimation(next); //let's try to take the next one + RETURN_IF_DELETED(setCurrentAnimation(next)); //let's try to take the next one else if (prev) - setCurrentAnimation(prev); + RETURN_IF_DELETED(setCurrentAnimation(prev)); else// case all animations were removed - setCurrentAnimation(nullptr); + RETURN_IF_DELETED(setCurrentAnimation(nullptr)); } // duration of the previous animations up to the current animation diff --git a/tests/auto/qml/animation/qsequentialanimationgroupjob/tst_qsequentialanimationgroupjob.cpp b/tests/auto/qml/animation/qsequentialanimationgroupjob/tst_qsequentialanimationgroupjob.cpp index 4718eb33b4..4bbfaffc1a 100644 --- a/tests/auto/qml/animation/qsequentialanimationgroupjob/tst_qsequentialanimationgroupjob.cpp +++ b/tests/auto/qml/animation/qsequentialanimationgroupjob/tst_qsequentialanimationgroupjob.cpp @@ -68,6 +68,7 @@ private slots: void insertAnimation(); void clear(); void pauseResume(); + void deleteFromListener(); }; void tst_QSequentialAnimationGroupJob::initTestCase() @@ -126,15 +127,23 @@ protected: class StateChangeListener: public QAnimationJobChangeListener { public: - virtual void animationStateChanged(QAbstractAnimationJob *, QAbstractAnimationJob::State newState, QAbstractAnimationJob::State) + virtual void animationStateChanged( + QAbstractAnimationJob *job, QAbstractAnimationJob::State newState, + QAbstractAnimationJob::State) { states << newState; + if (beEvil) { + delete job->group(); + groupDeleted = true; + } } void clear() { states.clear(); } int count() const { return states.count(); } QList states; + bool beEvil = false; + bool groupDeleted = false; }; class FinishedListener: public QAnimationJobChangeListener @@ -1650,6 +1659,37 @@ void tst_QSequentialAnimationGroupJob::uncontrolledWithLoops() QTRY_COMPARE(group.state(), QAbstractAnimationJob::Stopped); } +void tst_QSequentialAnimationGroupJob::deleteFromListener() +{ + QSequentialAnimationGroupJob *group = new QSequentialAnimationGroupJob; + + UncontrolledAnimation *uncontrolled = new UncontrolledAnimation(); + TestAnimation *shortLoop = new TestAnimation(100); + UncontrolledAnimation *more = new UncontrolledAnimation(); + + shortLoop->setLoopCount(-1); + + group->appendAnimation(uncontrolled); + group->appendAnimation(shortLoop); + group->appendAnimation(more); + + StateChangeListener listener; + listener.beEvil = true; + shortLoop->addAnimationChangeListener(&listener, QAbstractAnimationJob::StateChange); + group->setLoopCount(2); + + group->start(); + + QCOMPARE(group->currentLoop(), 0); + QCOMPARE(group->state(), QAbstractAnimationJob::Running); + QTRY_COMPARE(uncontrolled->state(), QAbstractAnimationJob::Running); + + QVERIFY(!listener.groupDeleted); + uncontrolled->stop(); + + QTRY_VERIFY(listener.groupDeleted); + // It's dead, Jim. +} QTEST_MAIN(tst_QSequentialAnimationGroupJob) #include "tst_qsequentialanimationgroupjob.moc" -- cgit v1.2.3