From 99f78eb7085b19c78153bdfbff9d24a2098a2a57 Mon Sep 17 00:00:00 2001 From: Thiago Macieira Date: Mon, 4 Mar 2024 14:47:29 -0800 Subject: [PATCH] QTimer/QObject::startTimer: improve the detection of overflow Converting from int milliseconds to int64_t nanoseconds can't overflow (it won't even for picoseconds, so we'll be fine for a couple more decades), so we only need to address the cases where the millisecond value was passed in int64_t: that is, in the std::chrono::milliseconds overloads. For the other cases, I added a comment. Amends bfc7535a10f7a6e3723f354b41f08a0fe1d18719 to not allow the detected overflow to happen at all, which could cause the timer to become very small. Instead, we saturate to the maximum, which is about 292 years (just under 106752 days). That's longer than computers have existed, so the chance that some Qt application is still running on a computer without any reboots from today to 24th century is remote at best. This parallels QDeadlineTimer, which already has code to saturate when using milliseconds. Change-Id: I6818d78a57394e37857bfffd17b9b1465b6a5d19 Reviewed-by: Ahmad Samir --- src/corelib/compat/removed_api.cpp | 10 ++++++---- src/corelib/kernel/qobject.cpp | 2 ++ src/corelib/kernel/qsingleshottimer_p.h | 15 +++++++++++++++ src/corelib/kernel/qtimer.cpp | 8 ++++---- src/dbus/qdbusintegrator.cpp | 3 ++- 5 files changed, 29 insertions(+), 9 deletions(-) diff --git a/src/corelib/compat/removed_api.cpp b/src/corelib/compat/removed_api.cpp index c8cc969f819..24699355ec4 100644 --- a/src/corelib/compat/removed_api.cpp +++ b/src/corelib/compat/removed_api.cpp @@ -1012,11 +1012,13 @@ int QObject::startTimer(std::chrono::milliseconds time, Qt::TimerType timerType) { using namespace std::chrono; using ratio = std::ratio_divide; - if (nanoseconds::rep r; qMulOverflow(time.count(), &r)) { - qWarning("QObject::startTimer(std::chrono::milliseconds time ...): " - "'time' arg will overflow when converted to nanoseconds."); + nanoseconds::rep r; + if (qMulOverflow(time.count(), &r)) { + qWarning("QObject::startTimer(std::chrono::milliseconds): " + "'time' arg overflowed when converted to nanoseconds."); + r = nanoseconds::max().count(); } - return startTimer(nanoseconds{time}, timerType); + return startTimer(nanoseconds{r}, timerType); } #if QT_CONFIG(processenvironment) diff --git a/src/corelib/kernel/qobject.cpp b/src/corelib/kernel/qobject.cpp index 708b10a75ee..e1129c5d251 100644 --- a/src/corelib/kernel/qobject.cpp +++ b/src/corelib/kernel/qobject.cpp @@ -1816,6 +1816,8 @@ void QObjectPrivate::setThreadData_helper(QThreadData *currentData, QThreadData int QObject::startTimer(int interval, Qt::TimerType timerType) { + // no overflow can happen here: + // 2^31 ms * 1,000,000 always fits a 64-bit signed integer type return startTimer(std::chrono::milliseconds{interval}, timerType); } diff --git a/src/corelib/kernel/qsingleshottimer_p.h b/src/corelib/kernel/qsingleshottimer_p.h index d7e33c52212..dd1402f63a1 100644 --- a/src/corelib/kernel/qsingleshottimer_p.h +++ b/src/corelib/kernel/qsingleshottimer_p.h @@ -19,6 +19,7 @@ #include "qabstracteventdispatcher.h" #include "qcoreapplication.h" #include "qmetaobject_p.h" +#include "private/qnumeric_p.h" #include @@ -43,6 +44,20 @@ public: inline void startTimerForReceiver(Duration interval, Qt::TimerType timerType, const QObject *receiver); + static Duration fromMsecs(std::chrono::milliseconds ms) + { + using namespace std::chrono; + using ratio = std::ratio_divide; + static_assert(ratio::den == 1); + + Duration::rep r; + if (qMulOverflow(ms.count(), &r)) { + qWarning("QTimer::singleShot(std::chrono::milliseconds, ...): " + "interval argument overflowed when converted to nanoseconds."); + return Duration::max(); + } + return Duration{r}; + } Q_SIGNALS: void timeout(); diff --git a/src/corelib/kernel/qtimer.cpp b/src/corelib/kernel/qtimer.cpp index cc46c1433b5..294369c1b33 100644 --- a/src/corelib/kernel/qtimer.cpp +++ b/src/corelib/kernel/qtimer.cpp @@ -213,7 +213,7 @@ void QTimer::start() if (d->isActive()) // stop running timer stop(); - const auto newId = Qt::TimerId{QObject::startTimer(d->inter * 1ms, d->type)}; + Qt::TimerId newId{ QObject::startTimer(d->inter * 1ms, d->type) }; // overflow impossible if (newId > Qt::TimerId::Invalid) { d->id = newId; d->isActiveData.notify(); @@ -332,7 +332,7 @@ void QTimer::singleShotImpl(std::chrono::milliseconds msec, Qt::TimerType timerT return; } - new QSingleShotTimer(msec, timerType, receiver, slotObj); + new QSingleShotTimer(QSingleShotTimer::fromMsecs(msec), timerType, receiver, slotObj); } /*! @@ -396,7 +396,7 @@ void QTimer::singleShot(std::chrono::milliseconds msec, Qt::TimerType timerType, Qt::QueuedConnection); return; } - (void) new QSingleShotTimer(msec, timerType, receiver, member); + (void) new QSingleShotTimer(QSingleShotTimer::fromMsecs(msec), timerType, receiver, member); } } @@ -592,7 +592,7 @@ void QTimer::setInterval(std::chrono::milliseconds interval) d->inter.setValueBypassingBindings(msec); if (d->isActive()) { // create new timer QObject::killTimer(d->id); // restart timer - const auto newId = Qt::TimerId{QObject::startTimer(msec * 1ms, d->type)}; + Qt::TimerId newId{ QObject::startTimer(msec * 1ms, d->type) }; // overflow impossible if (newId > Qt::TimerId::Invalid) { // Restarted successfully. No need to update the active state. d->id = newId; diff --git a/src/dbus/qdbusintegrator.cpp b/src/dbus/qdbusintegrator.cpp index 89b9b2d17e6..836562f496d 100644 --- a/src/dbus/qdbusintegrator.cpp +++ b/src/dbus/qdbusintegrator.cpp @@ -151,7 +151,8 @@ static dbus_bool_t qDBusAddTimeout(DBusTimeout *timeout, void *data) Q_ASSERT(d->timeouts.key(timeout, 0) == 0); - int timerId = d->startTimer(std::chrono::milliseconds{q_dbus_timeout_get_interval(timeout)}); + using namespace std::chrono_literals; + int timerId = d->startTimer(q_dbus_timeout_get_interval(timeout) * 1ms); // no overflow possible if (!timerId) return false;