From 7b7ad02681f2c28fb18d053618f6804e989b2f56 Mon Sep 17 00:00:00 2001 From: Jonathan Liu Date: Thu, 18 Sep 2014 19:54:46 +1000 Subject: Improve checking for event/socket notifiers and timers Starting/stopping timers from another thread may result in errors that may not appear until hours, days or weeks after if a release build of Qt is used with the GLib/UNIX event dispatchers. Such errors may manifest as warnings such as "QObject::killTimer(): Error: timer id 7 is not valid for object 0x2a51b488 (), timer has not been killed" and application crashes (e.g. crashes in malloc, realloc and malloc_consolidate). Initial-patch-by: Eike Ziller Task-number: QTBUG-40636 Change-Id: I2de50d50eb1fc7467fcebb9c73b74d2f85137933 Reviewed-by: Jocelyn Turcotte Reviewed-by: Thiago Macieira --- src/corelib/kernel/qbasictimer.cpp | 38 ++++++++++++++++++++------- src/corelib/kernel/qeventdispatcher_glib.cpp | 6 ++--- src/corelib/kernel/qeventdispatcher_unix.cpp | 6 ++--- src/corelib/kernel/qeventdispatcher_win.cpp | 12 ++++++--- src/corelib/kernel/qeventdispatcher_winrt.cpp | 12 ++++++--- src/corelib/kernel/qobject.cpp | 31 +++++++++++++++------- src/corelib/kernel/qsocketnotifier.cpp | 4 +++ src/corelib/kernel/qtimer.cpp | 4 +++ src/corelib/kernel/qwineventnotifier.cpp | 10 ++++--- 9 files changed, 89 insertions(+), 34 deletions(-) (limited to 'src/corelib') diff --git a/src/corelib/kernel/qbasictimer.cpp b/src/corelib/kernel/qbasictimer.cpp index f3366489d5..377c45219c 100644 --- a/src/corelib/kernel/qbasictimer.cpp +++ b/src/corelib/kernel/qbasictimer.cpp @@ -118,13 +118,19 @@ QT_BEGIN_NAMESPACE void QBasicTimer::start(int msec, QObject *obj) { QAbstractEventDispatcher *eventDispatcher = QAbstractEventDispatcher::instance(); - if (!eventDispatcher) { + if (Q_UNLIKELY(!eventDispatcher)) { qWarning("QBasicTimer::start: QBasicTimer can only be used with threads started with QThread"); return; } + if (Q_UNLIKELY(obj && obj->thread() != eventDispatcher->thread())) { + qWarning("QBasicTimer::start: Timers cannot be started from another thread"); + return; + } if (id) { - eventDispatcher->unregisterTimer(id); - QAbstractEventDispatcherPrivate::releaseTimerId(id); + if (Q_LIKELY(eventDispatcher->unregisterTimer(id))) + QAbstractEventDispatcherPrivate::releaseTimerId(id); + else + qWarning("QBasicTimer::start: Stopping previous timer failed. Possibly trying to stop from a different thread"); } id = 0; if (obj) @@ -145,13 +151,23 @@ void QBasicTimer::start(int msec, QObject *obj) void QBasicTimer::start(int msec, Qt::TimerType timerType, QObject *obj) { QAbstractEventDispatcher *eventDispatcher = QAbstractEventDispatcher::instance(); - if (!eventDispatcher) { + if (Q_UNLIKELY(msec < 0)) { + qWarning("QBasicTimer::start: Timers cannot have negative timeouts"); + return; + } + if (Q_UNLIKELY(!eventDispatcher)) { qWarning("QBasicTimer::start: QBasicTimer can only be used with threads started with QThread"); return; } + if (Q_UNLIKELY(obj && obj->thread() != eventDispatcher->thread())) { + qWarning("QBasicTimer::start: Timers cannot be started from another thread"); + return; + } if (id) { - eventDispatcher->unregisterTimer(id); - QAbstractEventDispatcherPrivate::releaseTimerId(id); + if (Q_LIKELY(eventDispatcher->unregisterTimer(id))) + QAbstractEventDispatcherPrivate::releaseTimerId(id); + else + qWarning("QBasicTimer::start: Stopping previous timer failed. Possibly trying to stop from a different thread"); } id = 0; if (obj) @@ -167,9 +183,13 @@ void QBasicTimer::stop() { if (id) { QAbstractEventDispatcher *eventDispatcher = QAbstractEventDispatcher::instance(); - if (eventDispatcher) - eventDispatcher->unregisterTimer(id); - QAbstractEventDispatcherPrivate::releaseTimerId(id); + if (eventDispatcher) { + if (Q_UNLIKELY(!eventDispatcher->unregisterTimer(id))) { + qWarning("QBasicTimer::stop: Failed. Possibly trying to stop from a different thread"); + return; + } + QAbstractEventDispatcherPrivate::releaseTimerId(id); + } } id = 0; } diff --git a/src/corelib/kernel/qeventdispatcher_glib.cpp b/src/corelib/kernel/qeventdispatcher_glib.cpp index e87e830c39..c4a5fc1ea8 100644 --- a/src/corelib/kernel/qeventdispatcher_glib.cpp +++ b/src/corelib/kernel/qeventdispatcher_glib.cpp @@ -518,7 +518,7 @@ void QEventDispatcherGlib::registerTimer(int timerId, int interval, Qt::TimerTyp qWarning("QEventDispatcherGlib::registerTimer: invalid arguments"); return; } else if (object->thread() != thread() || thread() != QThread::currentThread()) { - qWarning("QObject::startTimer: timers cannot be started from another thread"); + qWarning("QEventDispatcherGlib::registerTimer: timers cannot be started from another thread"); return; } #endif @@ -534,7 +534,7 @@ bool QEventDispatcherGlib::unregisterTimer(int timerId) qWarning("QEventDispatcherGlib::unregisterTimer: invalid argument"); return false; } else if (thread() != QThread::currentThread()) { - qWarning("QObject::killTimer: timers cannot be stopped from another thread"); + qWarning("QEventDispatcherGlib::unregisterTimer: timers cannot be stopped from another thread"); return false; } #endif @@ -550,7 +550,7 @@ bool QEventDispatcherGlib::unregisterTimers(QObject *object) qWarning("QEventDispatcherGlib::unregisterTimers: invalid argument"); return false; } else if (object->thread() != thread() || thread() != QThread::currentThread()) { - qWarning("QObject::killTimers: timers cannot be stopped from another thread"); + qWarning("QEventDispatcherGlib::unregisterTimers: timers cannot be stopped from another thread"); return false; } #endif diff --git a/src/corelib/kernel/qeventdispatcher_unix.cpp b/src/corelib/kernel/qeventdispatcher_unix.cpp index 69363bc3c9..9674fd5417 100644 --- a/src/corelib/kernel/qeventdispatcher_unix.cpp +++ b/src/corelib/kernel/qeventdispatcher_unix.cpp @@ -338,7 +338,7 @@ void QEventDispatcherUNIX::registerTimer(int timerId, int interval, Qt::TimerTyp qWarning("QEventDispatcherUNIX::registerTimer: invalid arguments"); return; } else if (obj->thread() != thread() || thread() != QThread::currentThread()) { - qWarning("QObject::startTimer: timers cannot be started from another thread"); + qWarning("QEventDispatcherUNIX::registerTimer: timers cannot be started from another thread"); return; } #endif @@ -357,7 +357,7 @@ bool QEventDispatcherUNIX::unregisterTimer(int timerId) qWarning("QEventDispatcherUNIX::unregisterTimer: invalid argument"); return false; } else if (thread() != QThread::currentThread()) { - qWarning("QObject::killTimer: timers cannot be stopped from another thread"); + qWarning("QEventDispatcherUNIX::unregisterTimer: timers cannot be stopped from another thread"); return false; } #endif @@ -376,7 +376,7 @@ bool QEventDispatcherUNIX::unregisterTimers(QObject *object) qWarning("QEventDispatcherUNIX::unregisterTimers: invalid argument"); return false; } else if (object->thread() != thread() || thread() != QThread::currentThread()) { - qWarning("QObject::killTimers: timers cannot be stopped from another thread"); + qWarning("QEventDispatcherUNIX::unregisterTimers: timers cannot be stopped from another thread"); return false; } #endif diff --git a/src/corelib/kernel/qeventdispatcher_win.cpp b/src/corelib/kernel/qeventdispatcher_win.cpp index f38ac7bf26..b7594d1dd3 100644 --- a/src/corelib/kernel/qeventdispatcher_win.cpp +++ b/src/corelib/kernel/qeventdispatcher_win.cpp @@ -908,13 +908,15 @@ void QEventDispatcherWin32::unregisterSocketNotifier(QSocketNotifier *notifier) void QEventDispatcherWin32::registerTimer(int timerId, int interval, Qt::TimerType timerType, QObject *object) { +#ifndef QT_NO_DEBUG if (timerId < 1 || interval < 0 || !object) { qWarning("QEventDispatcherWin32::registerTimer: invalid arguments"); return; } else if (object->thread() != thread() || thread() != QThread::currentThread()) { - qWarning("QObject::startTimer: timers cannot be started from another thread"); + qWarning("QEventDispatcherWin32::registerTimer: timers cannot be started from another thread"); return; } +#endif Q_D(QEventDispatcherWin32); @@ -936,15 +938,17 @@ void QEventDispatcherWin32::registerTimer(int timerId, int interval, Qt::TimerTy bool QEventDispatcherWin32::unregisterTimer(int timerId) { +#ifndef QT_NO_DEBUG if (timerId < 1) { qWarning("QEventDispatcherWin32::unregisterTimer: invalid argument"); return false; } QThread *currentThread = QThread::currentThread(); if (thread() != currentThread) { - qWarning("QObject::killTimer: timers cannot be stopped from another thread"); + qWarning("QEventDispatcherWin32::unregisterTimer: timers cannot be stopped from another thread"); return false; } +#endif Q_D(QEventDispatcherWin32); if (d->timerVec.isEmpty() || timerId <= 0) @@ -962,15 +966,17 @@ bool QEventDispatcherWin32::unregisterTimer(int timerId) bool QEventDispatcherWin32::unregisterTimers(QObject *object) { +#ifndef QT_NO_DEBUG if (!object) { qWarning("QEventDispatcherWin32::unregisterTimers: invalid argument"); return false; } QThread *currentThread = QThread::currentThread(); if (object->thread() != thread() || thread() != currentThread) { - qWarning("QObject::killTimers: timers cannot be stopped from another thread"); + qWarning("QEventDispatcherWin32::unregisterTimers: timers cannot be stopped from another thread"); return false; } +#endif Q_D(QEventDispatcherWin32); if (d->timerVec.isEmpty()) diff --git a/src/corelib/kernel/qeventdispatcher_winrt.cpp b/src/corelib/kernel/qeventdispatcher_winrt.cpp index aec0981677..d75d30f48e 100644 --- a/src/corelib/kernel/qeventdispatcher_winrt.cpp +++ b/src/corelib/kernel/qeventdispatcher_winrt.cpp @@ -254,13 +254,15 @@ void QEventDispatcherWinRT::registerTimer(int timerId, int interval, Qt::TimerTy { Q_UNUSED(timerType); +#ifndef QT_NO_DEBUG if (timerId < 1 || interval < 0 || !object) { qWarning("QEventDispatcherWinRT::registerTimer: invalid arguments"); return; } else if (object->thread() != thread() || thread() != QThread::currentThread()) { - qWarning("QObject::startTimer: timers cannot be started from another thread"); + qWarning("QEventDispatcherWinRT::registerTimer: timers cannot be started from another thread"); return; } +#endif Q_D(QEventDispatcherWinRT); @@ -289,14 +291,16 @@ void QEventDispatcherWinRT::registerTimer(int timerId, int interval, Qt::TimerTy bool QEventDispatcherWinRT::unregisterTimer(int timerId) { +#ifndef QT_NO_DEBUG if (timerId < 1) { qWarning("QEventDispatcherWinRT::unregisterTimer: invalid argument"); return false; } if (thread() != QThread::currentThread()) { - qWarning("QObject::killTimer: timers cannot be stopped from another thread"); + qWarning("QEventDispatcherWinRT::unregisterTimer: timers cannot be stopped from another thread"); return false; } +#endif Q_D(QEventDispatcherWinRT); @@ -310,15 +314,17 @@ bool QEventDispatcherWinRT::unregisterTimer(int timerId) bool QEventDispatcherWinRT::unregisterTimers(QObject *object) { +#ifndef QT_NO_DEBUG if (!object) { qWarning("QEventDispatcherWinRT::unregisterTimers: invalid argument"); return false; } QThread *currentThread = QThread::currentThread(); if (object->thread() != thread() || thread() != currentThread) { - qWarning("QObject::killTimers: timers cannot be stopped from another thread"); + qWarning("QEventDispatcherWinRT::unregisterTimers: timers cannot be stopped from another thread"); return false; } +#endif Q_D(QEventDispatcherWinRT); for (QHash::iterator it = d->timerDict.begin(); it != d->timerDict.end();) { diff --git a/src/corelib/kernel/qobject.cpp b/src/corelib/kernel/qobject.cpp index 45bf4b62c5..bf814f36b9 100644 --- a/src/corelib/kernel/qobject.cpp +++ b/src/corelib/kernel/qobject.cpp @@ -225,13 +225,17 @@ QObjectPrivate::QObjectPrivate(int version) QObjectPrivate::~QObjectPrivate() { if (extraData && !extraData->runningTimers.isEmpty()) { - // unregister pending timers - if (threadData->eventDispatcher.load()) - threadData->eventDispatcher.load()->unregisterTimers(q_ptr); - - // release the timer ids back to the pool - for (int i = 0; i < extraData->runningTimers.size(); ++i) - QAbstractEventDispatcherPrivate::releaseTimerId(extraData->runningTimers.at(i)); + if (Q_LIKELY(threadData->thread == QThread::currentThread())) { + // unregister pending timers + if (threadData->eventDispatcher.load()) + threadData->eventDispatcher.load()->unregisterTimers(q_ptr); + + // release the timer ids back to the pool + for (int i = 0; i < extraData->runningTimers.size(); ++i) + QAbstractEventDispatcherPrivate::releaseTimerId(extraData->runningTimers.at(i)); + } else { + qWarning("QObject::~QObject: Timers cannot be stopped from another thread"); + } } if (postedEvents) @@ -1612,15 +1616,18 @@ int QObject::startTimer(int interval, Qt::TimerType timerType) { Q_D(QObject); - if (interval < 0) { + if (Q_UNLIKELY(interval < 0)) { qWarning("QObject::startTimer: Timers cannot have negative intervals"); return 0; } - - if (!d->threadData->eventDispatcher.load()) { + if (Q_UNLIKELY(!d->threadData->eventDispatcher.load())) { qWarning("QObject::startTimer: Timers can only be used with threads started with QThread"); return 0; } + if (Q_UNLIKELY(thread() != QThread::currentThread())) { + qWarning("QObject::startTimer: Timers cannot be started from another thread"); + return 0; + } int timerId = d->threadData->eventDispatcher.load()->registerTimer(interval, timerType, this); if (!d->extraData) d->extraData = new QObjectPrivate::ExtraData; @@ -1640,6 +1647,10 @@ int QObject::startTimer(int interval, Qt::TimerType timerType) void QObject::killTimer(int id) { Q_D(QObject); + if (Q_UNLIKELY(thread() != QThread::currentThread())) { + qWarning("QObject::killTimer: Timers cannot be stopped from another thread"); + return; + } if (id) { int at = d->extraData ? d->extraData->runningTimers.indexOf(id) : -1; if (at == -1) { diff --git a/src/corelib/kernel/qsocketnotifier.cpp b/src/corelib/kernel/qsocketnotifier.cpp index 6470873d9a..2598f02a19 100644 --- a/src/corelib/kernel/qsocketnotifier.cpp +++ b/src/corelib/kernel/qsocketnotifier.cpp @@ -274,6 +274,10 @@ void QSocketNotifier::setEnabled(bool enable) if (!d->threadData->eventDispatcher.load()) // perhaps application/thread is shutting down return; + if (Q_UNLIKELY(thread() != QThread::currentThread())) { + qWarning("QSocketNotifier: Socket notifiers cannot be enabled or disabled from another thread"); + return; + } if (d->snenabled) d->threadData->eventDispatcher.load()->registerSocketNotifier(this); else diff --git a/src/corelib/kernel/qtimer.cpp b/src/corelib/kernel/qtimer.cpp index 97aae6f7e0..5c09b483c6 100644 --- a/src/corelib/kernel/qtimer.cpp +++ b/src/corelib/kernel/qtimer.cpp @@ -395,6 +395,10 @@ void QTimer::singleShot(int msec, const QObject *receiver, const char *member) */ void QTimer::singleShot(int msec, Qt::TimerType timerType, const QObject *receiver, const char *member) { + if (Q_UNLIKELY(msec < 0)) { + qWarning("QTimer::singleShot: Timers cannot have negative timeouts"); + return; + } if (receiver && member) { if (msec == 0) { // special code shortpath for 0-timers diff --git a/src/corelib/kernel/qwineventnotifier.cpp b/src/corelib/kernel/qwineventnotifier.cpp index c694237dc3..d2c623d489 100644 --- a/src/corelib/kernel/qwineventnotifier.cpp +++ b/src/corelib/kernel/qwineventnotifier.cpp @@ -140,11 +140,11 @@ QWinEventNotifier::QWinEventNotifier(HANDLE hEvent, QObject *parent) { Q_D(QWinEventNotifier); QAbstractEventDispatcher *eventDispatcher = d->threadData->eventDispatcher.load(); - if (!eventDispatcher) { + if (Q_UNLIKELY(!eventDispatcher)) { qWarning("QWinEventNotifier: Can only be used with threads started with QThread"); - } else { - eventDispatcher->registerEventNotifier(this); + return; } + eventDispatcher->registerEventNotifier(this); d->enabled = true; } @@ -215,6 +215,10 @@ void QWinEventNotifier::setEnabled(bool enable) QAbstractEventDispatcher *eventDispatcher = d->threadData->eventDispatcher.load(); if (!eventDispatcher) // perhaps application is shutting down return; + if (Q_UNLIKELY(thread() != QThread::currentThread())) { + qWarning("QWinEventNotifier: Event notifiers cannot be enabled or disabled from another thread"); + return; + } if (enable) eventDispatcher->registerEventNotifier(this); -- cgit v1.2.3