Change thread check to race check

webrtc/base/race_checker.cc

 /*
  *  Copyright (c) 2016 The WebRTC project authors. All Rights Reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
 #include "webrtc/base/race_checker.h"
 
 namespace rtc {
 
 RaceChecker::RaceChecker() {}
 
+// Note that the implementation here is in itself racy, but we pretend it does
+// not matter. If there is indeed a race (two threads both think access_count_
+// is zero) access_count_ will likely only be incremented once, but decremented
+// twice, causing the IsThreadRefEqual() check to return false at a later point.

[kwiberg-webrtc, 2016/09/20 19:29:07]

It may be incremented once instead of twice, possibly followed by both threads
writing to accessing_thread_. A similar race may occur if two threads try to
leave the critical section at the same time, or if one leaves at the same time
as one enters. The only possible bad effect of a race should be that we fail to
detect a race.

However, this assumes that all reads of accessing_thread_ retrieve a value that
was previously written. This is not guaranteed to be the case! E.g. if
PlatformThreadRef is 64 bits and we have a 32-bit system, the write may happen
in two parts.

I'd be more comfortable if RaceChecker used atomics so that it itself was race
free, or if it was only enabled in debug builds. I don't expect that problematic
behavior is very likely, though, so fixing this isn't a high priority.

[pbos-webrtc, 2016/09/20 19:36:02]

If two threads use it, and anyone at any point writes garbage into access_count_
(or generate -1 by double-decrementing or whatever) then the thread ref check
will fail for either thread A or B in the future since they won't set
accessing_thread_ even though they use the critical section.

Atomics cause synchronization and could also potentially make a data race less
likely to be found, plus it adds non-zero overhead, especially on ARM platforms
etc.

The idea was that this should be used in release as well, and the cost being
that this is more probabilistic than deterministic in detection. Though I doubt
it has a very high false negative rate, unless a compiler manages to completely
miscompile it since it's not supposed to be racy.

[kwiberg-webrtc, 2016/09/20 20:01:31]

Yes, garbage in access_count_ shouldn't be a problem. At most, it'll cause us to
fail to detect a race. It's garbage in accessing_thread_ that could be bad. On
Windows, we end up comparing such values as plain integers, which is safe even
if they contain garbage, but where we use pthreads we compare them with
pthread_equal(). If that function e.g. interprets (parts of) its two pthread_t
arguments as indexes and uses those indexes to look stuff up in memory, we have
a problem.
Yes on both counts, although I wouldn't suspect that atomic instructions would
cause data races to be all that much less likely to occur. Did you try it and
find a nonnegligible effect?

Implementing this with proper atomics would make it exactly equivalent to a
mutex that explodes whenever there is contention (i.e. in exactly the
circumstances where a real mutex would busy-wait or sleep). That seems like it
ought to work just fine.
OK. That may serve as a convincing argument for why using a mutex or even
atomics might be too expensive.
The compiler doesn't need to miscompile this code---if you have a race, that's
undefined behavior, so the compiler is fully within its rights to produce code
that behaves very badly if a race occurs. This code relies on compiler
implementations and hardware to be nice. As a result, you have to know a bunch
of low-level details about the various processors, OSes, and compilers that we
use in order to assess if it's safe.

 bool RaceChecker::Acquire() const {
   const PlatformThreadRef current_thread = CurrentThreadRef();
   // Set new accessing thread if this is a new use.
   if (access_count_++ == 0)
     accessing_thread_ = current_thread;
   // If this is being used concurrently this check will fail for the second
   // thread entering since it won't set the thread. Recursive use of checked
   // methods are OK since the accessing thread remains the same.
   const PlatformThreadRef accessing_thread = accessing_thread_;
   return IsThreadRefEqual(accessing_thread, current_thread);
@@ skipping 10 matching lines @@
 bool RaceCheckerScope::RaceDetected() const {
   return !race_check_ok_;
 }
 
 RaceCheckerScope::~RaceCheckerScope() {
   race_checker_->Release();
 }
 
 }  // namespace internal
 }  // namespace rtc