From 08672605906581c818ae428d9f2a2de489278d34 Mon Sep 17 00:00:00 2001
From: Taylor Brandstetter <deadbeef@webrtc.org>
Date: Fri, 2 Mar 2018 15:20:33 -0800
Subject: [PATCH] Fixing data race on vptr of Thread subclasses.

Thread's constructor calls DoInit, which registers itself with the
MessageQueueManager. This could result in the vptr being read before
the subclass has had a chance to modify it (for example, if another
thread happens to call MessageQueueManager::Clear at the right time).

This is exactly why there's a "DoInit" method, which is intended to be
called by the fully instantiated subclass. This was being done between
MessageQueue/Thread, but not between Thread and its subclasses.

Bug: webrtc:3911
Change-Id: I94d8855da56d9aaf22470ddca12d0b1dd5de249d
Reviewed-on: https://webrtc-review.googlesource.com/59466
Reviewed-by: Bjorn Mellem <mellem@webrtc.org>
Reviewed-by: Niels Moller <nisse@webrtc.org>
Commit-Queue: Taylor Brandstetter <deadbeef@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#22297}
---
 rtc_base/signalthread.cc |  7 +++++++
 rtc_base/signalthread.h  |  4 +---
 rtc_base/thread.cc       | 27 ++++++++++++++++++++-------
 rtc_base/thread.h        |  5 +++++
 4 files changed, 33 insertions(+), 10 deletions(-)

diff --git a/rtc_base/signalthread.cc b/rtc_base/signalthread.cc
index 4bf7d7c4eb..48a677e378 100644
--- a/rtc_base/signalthread.cc
+++ b/rtc_base/signalthread.cc
@@ -11,6 +11,7 @@
 #include "rtc_base/signalthread.h"
 
 #include "rtc_base/checks.h"
+#include "rtc_base/ptr_util.h"
 
 namespace rtc {
 
@@ -124,6 +125,12 @@ void SignalThread::OnMessage(Message *msg) {
   }
 }
 
+SignalThread::Worker::Worker(SignalThread* parent)
+    : Thread(MakeUnique<NullSocketServer>(), /*do_init=*/false),
+      parent_(parent) {
+  DoInit();
+}
+
 SignalThread::Worker::~Worker() {
   Stop();
 }
diff --git a/rtc_base/signalthread.h b/rtc_base/signalthread.h
index 2c11da4507..8daaa0830e 100644
--- a/rtc_base/signalthread.h
+++ b/rtc_base/signalthread.h
@@ -104,9 +104,7 @@ class SignalThread
 
   class Worker : public Thread {
    public:
-    explicit Worker(SignalThread* parent)
-        : Thread(std::unique_ptr<SocketServer>(new NullSocketServer())),
-          parent_(parent) {}
+    explicit Worker(SignalThread* parent);
     ~Worker() override;
     void Run() override;
     bool IsProcessingMessages() override;
diff --git a/rtc_base/thread.cc b/rtc_base/thread.cc
index 749334125f..bb7b5919e9 100644
--- a/rtc_base/thread.cc
+++ b/rtc_base/thread.cc
@@ -124,15 +124,25 @@ Thread::ScopedDisallowBlockingCalls::~ScopedDisallowBlockingCalls() {
 // DEPRECATED.
 Thread::Thread() : Thread(SocketServer::CreateDefault()) {}
 
-Thread::Thread(SocketServer* ss) : MessageQueue(ss, false) {
-  SetName("Thread", this);  // default name
-  DoInit();
-}
+Thread::Thread(SocketServer* ss) : Thread(ss, /*do_init=*/true) {}
 
 Thread::Thread(std::unique_ptr<SocketServer> ss)
+    : Thread(std::move(ss), /*do_init=*/true) {}
+
+Thread::Thread(SocketServer* ss, bool do_init)
+    : MessageQueue(ss, /*do_init=*/false) {
+  SetName("Thread", this);  // default name
+  if (do_init) {
+    DoInit();
+  }
+}
+
+Thread::Thread(std::unique_ptr<SocketServer> ss, bool do_init)
     : MessageQueue(std::move(ss), false) {
   SetName("Thread", this);  // default name
-  DoInit();
+  if (do_init) {
+    DoInit();
+  }
 }
 
 Thread::~Thread() {
@@ -524,7 +534,9 @@ bool Thread::IsRunning() {
 #endif
 }
 
-AutoThread::AutoThread() : Thread(SocketServer::CreateDefault()) {
+AutoThread::AutoThread()
+    : Thread(SocketServer::CreateDefault(), /*do_init=*/false) {
+  DoInit();
   if (!ThreadManager::Instance()->CurrentThread()) {
     ThreadManager::Instance()->SetCurrentThread(this);
   }
@@ -539,7 +551,8 @@ AutoThread::~AutoThread() {
 }
 
 AutoSocketServerThread::AutoSocketServerThread(SocketServer* ss)
-    : Thread(ss) {
+    : Thread(ss, /*do_init=*/false) {
+  DoInit();
   old_thread_ = ThreadManager::Instance()->CurrentThread();
   // Temporarily set the current thread to nullptr so that we can keep checks
   // around that catch unintentional pointer overwrites.
diff --git a/rtc_base/thread.h b/rtc_base/thread.h
index ab6c7b1b50..568764e65b 100644
--- a/rtc_base/thread.h
+++ b/rtc_base/thread.h
@@ -111,6 +111,11 @@ class RTC_LOCKABLE Thread : public MessageQueue {
 
   explicit Thread(SocketServer* ss);
   explicit Thread(std::unique_ptr<SocketServer> ss);
+  // Constructors meant for subclasses; they should call DoInit themselves and
+  // pass false for |do_init|, so that DoInit is called only on the fully
+  // instantiated class, which avoids a vptr data race.
+  Thread(SocketServer* ss, bool do_init);
+  Thread(std::unique_ptr<SocketServer> ss, bool do_init);
 
   // NOTE: ALL SUBCLASSES OF Thread MUST CALL Stop() IN THEIR DESTRUCTORS (or
   // guarantee Stop() is explicitly called before the subclass is destroyed).