Fix race condition in QThread::setPriority

The value of priority was read without the mutex locked, from within
the thread.

Had to extract a QThreadPrivate::setPriority method so that it can be called
with the mutex already locked. So if the main thread calls setPriority
while the thread is starting, it will be either be before or after the
"re-set priority" code at thread startup, but at least not in the middle of it.

Change-Id: I7a054f68623f61482c749274da66f3b2dcd8bcee
Reviewed-by: Olivier Goffart <ogoffart@woboq.com>

4 files changed, 42 insertions, 41 deletions

diff --git a/src/corelib/thread/qthread.cpp b/src/corelib/thread/qthread.cpp
index d230c8a145..4d5bee3154 100644
--- a/src/corelib/thread/qthread.cpp
+++ b/src/corelib/thread/qthread.cpp
@@ -593,6 +593,16 @@ void QThread::run()
 
     \sa Priority, priority(), start()
 */
+void QThread::setPriority(Priority priority)
+{
+    Q_D(QThread);
+    QMutexLocker locker(&d->mutex);
+    if (!d->running) {
+        qWarning("QThread::setPriority: Cannot set priority, thread is not running");
+        return;
+    }
+    d->setPriority(priority);
+}
 
 /*!
     \since 4.1
diff --git a/src/corelib/thread/qthread_p.h b/src/corelib/thread/qthread_p.h
index 7e963fdcd1..9d773b3c1c 100644
--- a/src/corelib/thread/qthread_p.h
+++ b/src/corelib/thread/qthread_p.h
@@ -142,6 +142,8 @@ public:
     QThreadPrivate(QThreadData *d = 0);
     ~QThreadPrivate();
 
+    void setPriority(QThread::Priority prio);
+
     mutable QMutex mutex;
     QAtomicInt quitLockRef;
 
diff --git a/src/corelib/thread/qthread_unix.cpp b/src/corelib/thread/qthread_unix.cpp
index b7e94366fe..f123e1813b 100644
--- a/src/corelib/thread/qthread_unix.cpp
+++ b/src/corelib/thread/qthread_unix.cpp
@@ -300,17 +300,18 @@ void *QThreadPrivate::start(void *arg)
     QThread *thr = reinterpret_cast<QThread *>(arg);
     QThreadData *data = QThreadData::get2(thr);
 
-    // do we need to reset the thread priority?
-    if (int(thr->d_func()->priority) & ThreadPriorityResetFlag) {
-        thr->setPriority(QThread::Priority(thr->d_func()->priority & ~ThreadPriorityResetFlag));
-    }
-
-    data->threadId = (Qt::HANDLE)pthread_self();
-    set_thread_data(data);
-
-    data->ref();
     {
         QMutexLocker locker(&thr->d_func()->mutex);
+
+        // do we need to reset the thread priority?
+        if (int(thr->d_func()->priority) & ThreadPriorityResetFlag) {
+            thr->d_func()->setPriority(QThread::Priority(thr->d_func()->priority & ~ThreadPriorityResetFlag));
+        }
+
+        data->threadId = (Qt::HANDLE)pthread_self();
+        set_thread_data(data);
+
+        data->ref();
         data->quitNow = thr->d_func()->exited;
     }
 
@@ -687,16 +688,10 @@ void QThread::setTerminationEnabled(bool enabled)
 #endif
 }
 
-void QThread::setPriority(Priority priority)
+// Caller must lock the mutex
+void QThreadPrivate::setPriority(QThread::Priority threadPriority)
 {
-    Q_D(QThread);
-    QMutexLocker locker(&d->mutex);
-    if (!d->running) {
-        qWarning("QThread::setPriority: Cannot set priority, thread is not running");
-        return;
-    }
-
-    d->priority = priority;
+    priority = threadPriority;
 
     // copied from start() with a few modifications:
 
@@ -704,7 +699,7 @@ void QThread::setPriority(Priority priority)
     int sched_policy;
     sched_param param;
 
-    if (pthread_getschedparam(d->thread_id, &sched_policy, &param) != 0) {
+    if (pthread_getschedparam(thread_id, &sched_policy, &param) != 0) {
         // failed to get the scheduling policy, don't bother setting
         // the priority
         qWarning("QThread::setPriority: Cannot get scheduler parameters");
@@ -720,15 +715,15 @@ void QThread::setPriority(Priority priority)
     }
 
     param.sched_priority = prio;
-    int status = pthread_setschedparam(d->thread_id, sched_policy, &param);
+    int status = pthread_setschedparam(thread_id, sched_policy, &param);
 
 # ifdef SCHED_IDLE
     // were we trying to set to idle priority and failed?
     if (status == -1 && sched_policy == SCHED_IDLE && errno == EINVAL) {
         // reset to lowest priority possible
-        pthread_getschedparam(d->thread_id, &sched_policy, &param);
+        pthread_getschedparam(thread_id, &sched_policy, &param);
         param.sched_priority = sched_get_priority_min(sched_policy);
-        pthread_setschedparam(d->thread_id, sched_policy, &param);
+        pthread_setschedparam(thread_id, sched_policy, &param);
     }
 # else
     Q_UNUSED(status);
diff --git a/src/corelib/thread/qthread_win.cpp b/src/corelib/thread/qthread_win.cpp
index ddf499f3bb..a0fac8eff2 100644
--- a/src/corelib/thread/qthread_win.cpp
+++ b/src/corelib/thread/qthread_win.cpp
@@ -588,55 +588,49 @@ void QThread::setTerminationEnabled(bool enabled)
     }
 }
 
-void QThread::setPriority(Priority priority)
+// Caller must hold the mutex
+void QThreadPrivate::setPriority(QThread::Priority threadPriority)
 {
-    Q_D(QThread);
-    QMutexLocker locker(&d->mutex);
-    if (!d->running) {
-        qWarning("QThread::setPriority: Cannot set priority, thread is not running");
-        return;
-    }
-
     // copied from start() with a few modifications:
 
     int prio;
-    d->priority = priority;
-    switch (d->priority) {
-    case IdlePriority:
+    priority = threadPriority;
+    switch (priority) {
+    case QThread::IdlePriority:
         prio = THREAD_PRIORITY_IDLE;
         break;
 
-    case LowestPriority:
+    case QThread::LowestPriority:
         prio = THREAD_PRIORITY_LOWEST;
         break;
 
-    case LowPriority:
+    case QThread::LowPriority:
         prio = THREAD_PRIORITY_BELOW_NORMAL;
         break;
 
-    case NormalPriority:
+    case QThread::NormalPriority:
         prio = THREAD_PRIORITY_NORMAL;
         break;
 
-    case HighPriority:
+    case QThread::HighPriority:
         prio = THREAD_PRIORITY_ABOVE_NORMAL;
         break;
 
-    case HighestPriority:
+    case QThread::HighestPriority:
         prio = THREAD_PRIORITY_HIGHEST;
         break;
 
-    case TimeCriticalPriority:
+    case QThread::TimeCriticalPriority:
         prio = THREAD_PRIORITY_TIME_CRITICAL;
         break;
 
-    case InheritPriority:
+    case QThread::InheritPriority:
     default:
         qWarning("QThread::setPriority: Argument cannot be InheritPriority");
         return;
     }
 
-    if (!SetThreadPriority(d->handle, prio)) {
+    if (!SetThreadPriority(handle, prio)) {
         qErrnoWarning("QThread::setPriority: Failed to set thread priority");
     }
 }