[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 ↗

(On Diff #115783)

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.

129 ↗

(On Diff #115783)

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 ↗

(On Diff #115783)

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.

129 ↗

(On Diff #115783)

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

[dvyukov]

compiler-rt/lib/asan/asan_report.cc

126 ↗

(On Diff #115783)

Right. It's not static. Withdrawn.

[eugenis]

compiler-rt/lib/asan/asan_report.cc

127 ↗

(On Diff #115871)

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 ↗

(On Diff #116095)

0xfffffe

compiler-rt/trunk/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:
+  static const u32 kUnclaimedTid = 0xfffffe;
+  static_assert(kUnclaimedTid != kInvalidTid, "Must be different");
+
   explicit ScopedInErrorReport(bool fatal = false) {
     halt_on_error_ = fatal || flags()->halt_on_error;
+    u32 current_tid = GetCurrentTidOrInvalid();
 
-    if (lock_.TryLock()) {
+    for (;;) {
+      u32 expected_tid = kUnclaimedTid;
+      if (atomic_compare_exchange_strong(&reporting_thread_tid_, &expected_tid,
+                                         current_tid, memory_order_relaxed)) {
+        // We've claimed reporting_thread_tid_ so proceed.
         StartReporting();
         return;
       }
 
-    // ASan found two bugs in different threads simultaneously.
-
-    u32 current_tid = GetCurrentTidOrInvalid();
-    if (reporting_thread_tid_ == current_tid ||
-        reporting_thread_tid_ == kInvalidTid) {
+      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.
+        // 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_) {
-      // 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. "
-             "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();
+      SleepForMillis(100);
     }
-
-    StartReporting();
   }
 
   ~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_, kUnclaimedTid);
+    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() {
+    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();
-    CommonSanitizerReportMutex.Lock();
-    reporting_thread_tid_ = GetCurrentTidOrInvalid();
-    Printf("===================================================="
-           "=============\n");
+    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_ = {kUnclaimedTid};
 ErrorDescription ScopedInErrorReport::current_error_;
 
 void ReportDeadlySignal(const SignalContext &sig) {
   ScopedInErrorReport in_report(/*fatal*/ true);
   ErrorDeadlySignal error(GetCurrentTidOrInvalid(), sig);
   in_report.ReportError(error);
 }
 

compiler-rt/trunk/test/asan/TestCases/Posix/concurrent_overflow.cc

+// RUN: %clangxx_asan -O0 -w %s -o %t && not %run %t 2>&1 | FileCheck %s
+
+// Checks that concurrent reports will not trigger false "nested bug" reports.
+// Regression test for https://github.com/google/sanitizers/issues/858
+
+#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];
+  return 0;
+}
+
+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
+// CHECK: ERROR: AddressSanitizer: stack-buffer-overflow on address
+// CHECK: SUMMARY: AddressSanitizer: stack-buffer-overflow