From 3b9c811a658d45f6e84a98ac26a5d122b34254fc Mon Sep 17 00:00:00 2001 From: "Bradley T. Hughes" Date: Mon, 9 Aug 2010 13:27:31 +0200 Subject: Fix invalid memory write during recursive timer activation The handler for one timer recurses the event loop, and the handler for another timer removes the first timer, returning from the recursion and the handler for the first timer causes an invalid write (since the timer info for the first timer has been deleted). Fix this by keeping an active reference in QTimerInfo (instead of just a bool inTimerEvent). If this is non-zero, the timer is currently being delivered, so we prevent more delivery. When a timer is removed and it's activateRef is set, we clear it so that the delivery code knows now to write to memory that's already been freed. A side effect of this change is that we no longer need to track the currentTimerInfo "globally" anymore, it can be a normal local variable in the QTimerInfoList::activateTimers() function. Task-number: QT-3553 Reviewed-by: olivier Reviewed-by: joao --- src/corelib/kernel/qeventdispatcher_unix.cpp | 31 ++++++++------------ src/corelib/kernel/qeventdispatcher_unix_p.h | 4 +-- tests/auto/qtimer/tst_qtimer.cpp | 44 ++++++++++++++++++++++++++++ 3 files changed, 59 insertions(+), 20 deletions(-) diff --git a/src/corelib/kernel/qeventdispatcher_unix.cpp b/src/corelib/kernel/qeventdispatcher_unix.cpp index f7d45ac..9dadd82 100644 --- a/src/corelib/kernel/qeventdispatcher_unix.cpp +++ b/src/corelib/kernel/qeventdispatcher_unix.cpp @@ -331,7 +331,7 @@ QTimerInfoList::QTimerInfoList() } #endif - firstTimerInfo = currentTimerInfo = 0; + firstTimerInfo = 0; } timeval QTimerInfoList::updateCurrentTime() @@ -445,7 +445,7 @@ bool QTimerInfoList::timerWait(timeval &tm) // Find first waiting timer not already active QTimerInfo *t = 0; for (QTimerInfoList::const_iterator it = constBegin(); it != constEnd(); ++it) { - if (!(*it)->inTimerEvent) { + if (!(*it)->activateRef) { t = *it; break; } @@ -474,7 +474,7 @@ void QTimerInfoList::registerTimer(int timerId, int interval, QObject *object) t->interval.tv_usec = (interval % 1000) * 1000; t->timeout = updateCurrentTime() + t->interval; t->obj = object; - t->inTimerEvent = false; + t->activateRef = 0; timerInsert(t); } @@ -489,8 +489,8 @@ bool QTimerInfoList::unregisterTimer(int timerId) removeAt(i); if (t == firstTimerInfo) firstTimerInfo = 0; - if (t == currentTimerInfo) - currentTimerInfo = 0; + if (t->activateRef) + *(t->activateRef) = 0; // release the timer id if (!QObjectPrivate::get(t->obj)->inThreadChangeEvent) @@ -515,8 +515,8 @@ bool QTimerInfoList::unregisterTimers(QObject *object) removeAt(i); if (t == firstTimerInfo) firstTimerInfo = 0; - if (t == currentTimerInfo) - currentTimerInfo = 0; + if (t->activateRef) + *(t->activateRef) = 0; // release the timer id if (!QObjectPrivate::get(t->obj)->inThreadChangeEvent) @@ -552,10 +552,7 @@ int QTimerInfoList::activateTimers() bool firstTime = true; timeval currentTime; int n_act = 0, maxCount = count(); - - QTimerInfo *saveFirstTimerInfo = firstTimerInfo; - QTimerInfo *saveCurrentTimerInfo = currentTimerInfo; - firstTimerInfo = currentTimerInfo = 0; + firstTimerInfo = 0; while (maxCount--) { currentTime = updateCurrentTime(); @@ -567,7 +564,7 @@ int QTimerInfoList::activateTimers() if (isEmpty()) break; - currentTimerInfo = first(); + QTimerInfo *currentTimerInfo = first(); if (currentTime < currentTimerInfo->timeout) break; // no timer has expired @@ -594,21 +591,19 @@ int QTimerInfoList::activateTimers() if (currentTimerInfo->interval.tv_usec > 0 || currentTimerInfo->interval.tv_sec > 0) n_act++; - if (!currentTimerInfo->inTimerEvent) { + if (!currentTimerInfo->activateRef) { // send event, but don't allow it to recurse - currentTimerInfo->inTimerEvent = true; + currentTimerInfo->activateRef = ¤tTimerInfo; QTimerEvent e(currentTimerInfo->id); QCoreApplication::sendEvent(currentTimerInfo->obj, &e); if (currentTimerInfo) - currentTimerInfo->inTimerEvent = false; + currentTimerInfo->activateRef = 0; } } - firstTimerInfo = saveFirstTimerInfo; - currentTimerInfo = saveCurrentTimerInfo; - + firstTimerInfo = 0; return n_act; } diff --git a/src/corelib/kernel/qeventdispatcher_unix_p.h b/src/corelib/kernel/qeventdispatcher_unix_p.h index cbe58de..060a163 100644 --- a/src/corelib/kernel/qeventdispatcher_unix_p.h +++ b/src/corelib/kernel/qeventdispatcher_unix_p.h @@ -77,7 +77,7 @@ struct QTimerInfo { timeval interval; // - timer interval timeval timeout; // - when to sent event QObject *obj; // - object to receive event - bool inTimerEvent; + QTimerInfo **activateRef; // - ref from activateTimers }; class QTimerInfoList : public QList @@ -92,7 +92,7 @@ class QTimerInfoList : public QList #endif // state variables used by activateTimers() - QTimerInfo *firstTimerInfo, *currentTimerInfo; + QTimerInfo *firstTimerInfo; public: QTimerInfoList(); diff --git a/tests/auto/qtimer/tst_qtimer.cpp b/tests/auto/qtimer/tst_qtimer.cpp index a0408ef..8d213ed 100644 --- a/tests/auto/qtimer/tst_qtimer.cpp +++ b/tests/auto/qtimer/tst_qtimer.cpp @@ -86,6 +86,7 @@ private slots: void timerIdPersistsAfterThreadExit(); void cancelLongTimer(); void singleShotStaticFunctionZeroTimeout(); + void recurseOnTimeoutAndStopTimer(); }; class TimerHelper : public QObject @@ -623,5 +624,48 @@ void tst_QTimer::singleShotStaticFunctionZeroTimeout() QCOMPARE(helper.count, 1); } +class RecursOnTimeoutAndStopTimerTimer : public QObject +{ + Q_OBJECT + +public: + QTimer *one; + QTimer *two; + +public slots: + void onetrigger() + { + QCoreApplication::processEvents(); + } + + void twotrigger() + { + one->stop(); + } +}; + +void tst_QTimer::recurseOnTimeoutAndStopTimer() +{ + QEventLoop eventLoop; + QTimer::singleShot(1000, &eventLoop, SLOT(quit())); + + RecursOnTimeoutAndStopTimerTimer t; + t.one = new QTimer(&t); + t.two = new QTimer(&t); + + QObject::connect(t.one, SIGNAL(timeout()), &t, SLOT(onetrigger())); + QObject::connect(t.two, SIGNAL(timeout()), &t, SLOT(twotrigger())); + + t.two->setSingleShot(true); + + t.one->start(); + t.two->start(); + + (void) eventLoop.exec(); + + QVERIFY(!t.one->isActive()); + QVERIFY(!t.two->isActive()); +} + QTEST_MAIN(tst_QTimer) #include "tst_qtimer.moc" -- cgit v0.12