commit 08672605906581c818ae428d9f2a2de489278d34
author Taylor Brandstetter <deadbeef@webrtc.org> Fri Mar 02 23:20:33 2018
committer Commit Bot <commit-bot@chromium.org> Mon Mar 05 20:09:20 2018
tree 34dba4721c78368cb9feaa33045c12feb423becf
parent 98cd810d315bd7ddb6b972175438fd2d04d8dd7c

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

diff --git a/rtc_base/signalthread.cc b/rtc_base/signalthread.cc
index 4bf7d7c..48a677e 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 @@
   }
 }
 
+SignalThread::Worker::Worker(SignalThread* parent)
+    : Thread(MakeUnique<NullSocketServer>(), /*do_init=*/false),
+      parent_(parent) {
+  DoInit();
+}
+
 SignalThread::Worker::~Worker() {
   Stop();
 }

rtc_base/signalthread.h

diff --git a/rtc_base/signalthread.h b/rtc_base/signalthread.h
index 2c11da4..8daaa08 100644
--- a/rtc_base/signalthread.h
+++ b/rtc_base/signalthread.h
@@ -104,9 +104,7 @@
 
   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;

rtc_base/thread.cc

diff --git a/rtc_base/thread.cc b/rtc_base/thread.cc
index 7493341..bb7b591 100644
--- a/rtc_base/thread.cc
+++ b/rtc_base/thread.cc
@@ -124,15 +124,25 @@
 // 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 @@
 #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 @@
 }
 
 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.

rtc_base/thread.h

diff --git a/rtc_base/thread.h b/rtc_base/thread.h
index ab6c7b1..568764e 100644
--- a/rtc_base/thread.h
+++ b/rtc_base/thread.h
@@ -111,6 +111,11 @@
 
   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).