[asan] Fix nested error detection

[vitalybuka]

Summary

Fixes https://github.com/google/sanitizers/issues/858

[vitalybuka]

Spin on reporting_thread_tid_

[vitalybuka]

Reordered code.

[vitalybuka]

Reformat

[vitalybuka]

Test comment and fixed typo in the test name

[dvyukov]

compiler-rt/lib/asan/asan_report.cc

126–128

This looks like a data race.
While one thread executes ScopedInErrorReport, another thread can do something like:

halt_on_error_ = fatal;
if (flags()->halt_on_error) halt_on_error_ = true;

And then the first thread can fail to halt and the program will hang for 100 seconds.

Or, there can be 2 thread: one with fatal=true and another with fatal=false, and asan will halt after a non-fatal error which can cause user confusion.

127–128

This looks somewhat fishy. Is there any known problem with making this an infinite loop as it is now?
Pretty sure SleepForMillis can undersleep if SIGPOF profiling is enabled, and then we can terminate the process while another thread still reports an error.

[vitalybuka]

infinit loop and not crash for fatal errors

[vitalybuka]

compiler-rt/lib/asan/asan_report.cc

126–128

This looks like a data race.
While one thread executes ScopedInErrorReport, another thread can do something like:
halt_on_error_ = fatal;
if (flags()->halt_on_error) halt_on_error_ = true;

Not sure I understand you.
halt_on_error_ is not static, and only one thread suppose to access particular ScopedInErrorReport instance.

Or, there can be 2 thread: one with fatal=true and another with fatal=false, and asan will halt after a non-fatal error which can cause user confusion.

Correct. I noticed that this is weird too, but I believe it's preexisted behavior and not related to the bug I am fixing. So I was not trying to improve that.
If I understand correctly, to avoid such confusion, I can throw away "if (halt_on_error_) break;" And code after the loop.

127–128

Probably nothing known.
I'll switch back to the infinit one.

[dvyukov]

compiler-rt/lib/asan/asan_report.cc

126–128

Right. It's not static. Withdrawn.

[eugenis]

compiler-rt/lib/asan/asan_report.cc

127–128

What if GetCurrentTidOrInvalid return kInvalidTid? We will start reporting w/o claiming reporting_thread_tid_. Use a different default value?

[vitalybuka]

Different unclaimed sentinel

[vitalybuka]

Undo yeld

[eugenis]

compiler-rt/lib/asan/asan_report.cc

125

0xfffffe

compiler-rt/lib/asan/asan_report.cc

 }
 
 // -------------------- Different kinds of reports ----------------- {{{1
 
 // Use ScopedInErrorReport to run common actions just before and
 // immediately after printing error report.
 class ScopedInErrorReport {
  public:
   explicit ScopedInErrorReport(bool fatal = false) {
     halt_on_error_ = fatal || flags()->halt_on_error;
 
-    if (lock_.TryLock()) {
-      StartReporting();
-      return;
-    }
-
-    // ASan found two bugs in different threads simultaneously.
-
+    {
       u32 current_tid = GetCurrentTidOrInvalid();
-    if (reporting_thread_tid_ == current_tid ||
-        reporting_thread_tid_ == kInvalidTid) {
-      // This is either asynch signal or nested error during error reporting.
-      // Fail simple to avoid deadlocks in Report().
+      u32 expected_tid = kInvalidTid;
+
+      if (!atomic_compare_exchange_strong(&reporting_thread_tid_, &expected_tid,
+                                          current_tid, memory_order_release)) {
+        if (expected_tid == current_tid) {
+          // This is either asynch signal or nested error during error
+          // reporting. Fail simple to avoid deadlocks in Report().
 
           // Can't use Report() here because of potential deadlocks
           // in nested signal handlers.
           static const char msg[] =
               "AddressSanitizer: nested bug in the same thread, aborting.\n";
           CatastrophicErrorWrite(msg, sizeof(msg) - 1);
 
           internal__exit(common_flags()->exitcode);
-    }
-
-    if (halt_on_error_) {
+        } else if (halt_on_error_) {
           // Do not print more than one report, otherwise they will mix up.
           // Error reporting functions shouldn't return at this situation, as
           // they are effectively no-returns.
-
-      Report("AddressSanitizer: while reporting a bug found another one. "
+          Report(
+              "AddressSanitizer: while reporting a bug found another one. "
               "Ignoring.\n");
 
           // Sleep long enough to make sure that the thread which started
           // to print an error report will finish doing it.
           SleepForSeconds(Max(100, flags()->sleep_before_dying + 1));
 
           // If we're still not dead for some reason, use raw _exit() instead of
           // Die() to bypass any additional checks.
           internal__exit(common_flags()->exitcode);
-    } else {
-      // The other thread will eventually finish reporting
-      // so it's safe to wait
-      lock_.Lock();
+        }
+      }
     }
 
-    StartReporting();
+    // If we get here, then either no error reporting yet or it's in a different
+    // thread.
+    CommonSanitizerReportMutex.Lock();
+    // Make sure the registry and sanitizer report mutexes are locked while
+    // we're printing an error report.
+    // We can lock them only here to avoid self-deadlock in case of
+    // recursive reports.
+    asanThreadRegistry().Lock();
+    Printf(
+        "=================================================================\n");
   }
 
   ~ScopedInErrorReport() {
     ASAN_ON_ERROR();
     if (current_error_.IsValid()) current_error_.Print();
 
     // Make sure the current thread is announced.
     DescribeThread(GetCurrentThread());
if (halt_on_error_ && common_flags()->abort_on_error) {
       SetAbortMessage(buffer_copy.data());
     }
 
     // In halt_on_error = false mode, reset the current error object (before
     // unlocking).
     if (!halt_on_error_)
       internal_memset(&current_error_, 0, sizeof(current_error_));
 
-    CommonSanitizerReportMutex.Unlock();
-    reporting_thread_tid_ = kInvalidTid;
-    lock_.Unlock();
     if (halt_on_error_) {
       Report("ABORTING\n");
       Die();
     }
+
+    atomic_store_relaxed(&reporting_thread_tid_, kInvalidTid);
+    CommonSanitizerReportMutex.Unlock();
   }
 
   void ReportError(const ErrorDescription &description) {
     // Can only report one error per ScopedInErrorReport.
     CHECK_EQ(current_error_.kind, kErrorKindInvalid);
     current_error_ = description;
   }
 
   static ErrorDescription &CurrentError() {
     return current_error_;
   }
 
  private:
-  void StartReporting() {
-    // Make sure the registry and sanitizer report mutexes are locked while
-    // we're printing an error report.
-    // We can lock them only here to avoid self-deadlock in case of
-    // recursive reports.
-    asanThreadRegistry().Lock();
-    CommonSanitizerReportMutex.Lock();
-    reporting_thread_tid_ = GetCurrentTidOrInvalid();
-    Printf("===================================================="
-           "=============\n");
-  }
-
-  static StaticSpinMutex lock_;
-  static u32 reporting_thread_tid_;
+  static atomic_uint32_t reporting_thread_tid_;
   // Error currently being reported. This enables the destructor to interact
   // with the debugger and point it to an error description.
   static ErrorDescription current_error_;
   bool halt_on_error_;
 };
 
-StaticSpinMutex ScopedInErrorReport::lock_;
-u32 ScopedInErrorReport::reporting_thread_tid_ = kInvalidTid;
+atomic_uint32_t ScopedInErrorReport::reporting_thread_tid_ = {kInvalidTid};
 ErrorDescription ScopedInErrorReport::current_error_;
 
 void ReportDeadlySignal(const SignalContext &sig) {
   ScopedInErrorReport in_report(/*fatal*/ true);
   ErrorDeadlySignal error(GetCurrentTidOrInvalid(), sig);
   in_report.ReportError(error);
 }
 

compiler-rt/test/asan/TestCases/Posix/concurent_overflow.cc

+// RUN: %clangxx_asan -O0 %s -o %t && not %run %t 2>&1 | FileCheck %s
+
+#include <pthread.h>
+#include <stdlib.h>
+#include <unistd.h>
+
+static void *start_routine(void *arg) {
+  volatile int *counter = (volatile int *)arg;
+  char buf[8];
+  __atomic_sub_fetch(counter, 1, __ATOMIC_SEQ_CST);
+  while (*counter)
+    ;
+  buf[0] = buf[9];
+}
+
+int main(void) {
+  const int n_threads = 8;
+  int i, counter = n_threads;
+  pthread_t thread;
+
+  for (i = 0; i < n_threads; ++i)
+    pthread_create(&thread, NULL, &start_routine, (void *)&counter);
+  sleep(5);
+  return 0;
+}
+
+// CHECK-NOT: nested bug