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>

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

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;
 

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);
+    priority = threadPriority;
-    QMutexLocker locker(&d->mutex);
-    if (!d->running) {
-        qWarning("QThread::setPriority: Cannot set priority, thread is not running");
-        return;
-    }
-
-    d->priority = priority;
 
     // 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);

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;
+    priority = threadPriority;
-    switch (d->priority) {
+    switch (priority) {
-    case IdlePriority:
+    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");
     }
 }