[asan] Reports suppressions for ASan recovery mode (compiler-rt part).

[m.ostapenko]

Summary

This is the compiler-rt part of suppression functionality for ASan recovery mode (here is the LLVM core part: http://reviews.llvm.org/D15079).

In current implementation, I'm trying to reuse UBSan approach with SourceLocations, so moving SourceLocation class definition from ubsan to sanitizer_common sources.

[ygribov]

lib/asan/asan_flags.inc

143 ↗

(On Diff #41396)

Do we need this flag? AFAIK UBSan suppresses dups unconditionally.

lib/asan/asan_rtl.cc

135 ↗

(On Diff #41396)

Whitespace padding too large.

test/asan/TestCases/Posix/halt_on_error-torture-2.cc

1 ↗

(On Diff #41396)

I wonder if this heavy multithreaded test is worth it.

[m.ostapenko]

lib/asan/asan_flags.inc

143 ↗

(On Diff #41396)

I added this flag just to preserve your halt_on_error-torture.cc stress testcase, that is designed to deal with multiple reports.

[m.ostapenko]

Updating the patch according to nits in related discussion (http://reviews.llvm.org/D15079).

[kcc]

lib/asan/asan_flags.inc

141 ↗

(On Diff #41517)

maybe use names similar to ones in tsan?
e.g. suppress_equal_pcs?

lib/asan/asan_rtl.cc

117 ↗

(On Diff #41517)

can't you make it __sanitizer::atomic_uint32_t ?

121 ↗

(On Diff #41517)

Bad.
We should not duplicate this kind of functionality.
Why not call Die() here?

129 ↗

(On Diff #41517)

this is actually racy, isn't it? :)

test/asan/TestCases/halt_on_error-2.c

6 ↗

(On Diff #41517)

I would just add
CHECK-NOT: ERROR
as a 4-th CHECK line

Also, add a run-line that actually produces 30 error messages and checks for that.

[ygribov]

lib/asan/asan_flags.inc

141 ↗

(On Diff #41517)

And also make the flag common.

[m.ostapenko]

lib/asan/asan_flags.inc

141 ↗

(On Diff #41517)

Ok.

lib/asan/asan_rtl.cc

117 ↗

(On Diff #41517)

Right, thank you.

121 ↗

(On Diff #41517)

Ok.

129 ↗

(On Diff #41517)

Yes, but do we actually need to be atomic here? I mean, if we hit the race, we would just jump over one entry in array, so it would contain 0. I don't think exact precision is critical here. Atomic operation may introduce unnecessary slowdown... not sure about this.

test/asan/TestCases/halt_on_error-2.c

6 ↗

(On Diff #41517)

Ok.

[m.ostapenko]

Updating according to nits.

[kcc]

lib/asan/asan_rtl.cc

128 ↗

(On Diff #41741)

phrasing? extra would?

Also, it's enough to iterate from pc_num-1 to avoid the race

test/asan/TestCases/Posix/halt_on_error-torture.cc

11 ↗

(On Diff #41741)

What's ASAN_SUPPRESS_EQUAL_PCS for?

test/asan/TestCases/halt_on_error-2.c

7 ↗

(On Diff #41741)

Instead of creating a separate test it better to use a different RUN line with another --check-prefix.

[m.ostapenko]

Updating the diff.

It seems we cannot get rid of races without using mutex, because we should protect access to different parts of AsanBuggyPcPool array. However, using a mutex here is undesirable due to performance issue. So, for now, I see such alternatives how to implement suppression:

1. Use mutex. This is probably an overkill.
2. Use atomic pc_num counter and non-atomic accesses to AsanBuggyPcPool. Here, we would have races on accesses to AsanBuggyPcPool, that may lead to duplication in reports (e.g. when 2 threads with same PC are searching in AsanBuggyPcPool and no one hasn't updated the array yet). Also, we'll need to add one additional check for AsanBuggyPcPool overflow before updating the pool because it is possible that multiple threads would try to update it simultaneously.

This patch implements the second approach for now, thought it's OK to have few duplications in favor of performance.

Konstantin, what do you think about it?

[kcc]

In D15080#302479, @m.ostepenko wrote:

Updating the diff.

It seems we cannot get rid of races without using mutex,

Oh, sure we can. :)

?> because we should protect access to different parts of AsanBuggyPcPool array. However, using a mutex here is undesirable due to performance issue. So, for now, I see such alternatives how to implement suppression:

1. Use mutex. This is probably an overkill.

Yep, no mutex please.

2. Use atomic pc_num counter and non-atomic accesses to AsanBuggyPcPool. Here, we would have races on accesses to AsanBuggyPcPool, that may lead to duplication in reports (e.g. when 2 threads with same PC are searching in AsanBuggyPcPool and no one hasn't updated the array yet). Also, we'll need to add one additional check for AsanBuggyPcPool overflow before updating the pool because it is possible that multiple threads would try to update it simultaneously.

This patch implements the second approach for now, thought it's OK to have few duplications in favor of performance.

Summoning lock-free synchronization guru

Konstantin, what do you think about it?

[m.ostapenko]

because we should protect access to different parts of AsanBuggyPcPool array.

I mean, before we can update AsanBuggyPcPool[pc_num], we need to iterate over the whole [0, pc_num - 1] range of array. To avoid reports duplication, we should ensure, that nobody modifies the array during lookup and I'm not sure we can achieve this w/o mutex (or something that blocks the for loop). But let's wait for Dmitry for comments.

[m.ostapenko]

Okay, let's do another try.

New patch implements a lock-free approach (thanks Yura for idea!) based on CAS. The only drawback of the method is that we should iterate over AsanBuggyPcPool from it's beginning each time we perform lookup, that may become a performance bottleneck. However, this way we managed to avoid races on AsanBuggyPcPool without using mutex and the only race we have is on pc_num counter, used for fast path (early returning) only. So, I wonder, if something like this can resolve the issue?

[dvyukov]

lib/asan/asan_rtl.cc

116 ↗

(On Diff #42169)

This is unnecessary complex and racy. And also produces excessive contention on AsanBuggyPcPool due to compare_exchange even when we are suppressing already reported pc. What you need is:

static bool SuppressErrorReport(uptr pc) {
	for (unsigned i = 0; i < kAsanBuggyPcPoolSize; i++) {
		u64 cmp = atomic_load_relaxed(&AsanBuggyPcPool[i]);
		if (cmp == 0 && atomic_compare_exchange_strong(&AsanBuggyPcPool[i], cmp, pc, memory_order_relaxed))
			return false;
		if (cmp == pc)
			return true;
	}
	Die();
}

[ygribov]

lib/asan/asan_rtl.cc

123 ↗

(On Diff #42169)

You already have this check after loop. Why do you need this preliminary check? It'll slow fast path.

129 ↗

(On Diff #42169)

I'm not familiar with atomics but I think that CAS is much slower than ordinary load. Can you do loads for fast path and only switch to CAS when reaching NULL?

136 ↗

(On Diff #42169)

It's better to avoid this extra atomic by using value returned by atomic_compare_exchange.

146 ↗

(On Diff #42169)

I believe we can get rid of pc_num completely.

147 ↗

(On Diff #42169)

This function seems to always return false.

166 ↗

(On Diff #42169)

I wonder if it makes sense to move suppression check to ReportGenericError. It'll remove duplicate code and also automatically handle other cases (ReportDoubleFree, etc.).

test/asan/TestCases/halt_on_error-2.c

4 ↗

(On Diff #42169)

Why no grep here?

7 ↗

(On Diff #42169)

Can you dump to %t.log or something like that? Also you don't need true.

25 ↗

(On Diff #42169)

You can control this with cmdline args and avoid double compilation.

[ygribov]

Oops, collided with Dima.

[m.ostapenko]

test/asan/TestCases/halt_on_error-2.c

4 ↗

(On Diff #42169)

I would just add
CHECK-NOT: ERROR
as a 4-th CHECK line

Just addressing this comment.

[m.ostapenko]

Dmitry, Yura, thanks for your hits. Updating the patch.

[dvyukov]

The lock-free part LGTM.

[kcc]

lib/asan/asan_report.cc

34 ↗

(On Diff #42186)

use static for all global vars here, please.

lib/sanitizer_common/sanitizer_flags.inc

197 ↗

(On Diff #42186)

no spaces around =

test/asan/TestCases/halt_on_error-2.c

1 ↗

(On Diff #42186)

Can we rename this test to have suppress_equal_pcs in the name (e.g. halt_on_error_suppress_equal_pcs.c).
Having _2 in the name is not helpful.

5 ↗

(On Diff #42186)

I want to have two more RUN lines here:
one that has suppress_equal_pcs=true and checks the suppress_equal_pcs=true is the same as default.
another that has suppress_equal_pcs=false and checks that this is different from default.

[m.ostapenko]

Addressing the nits.

[kcc]

LGTM with one small nit

lib/sanitizer_common/sanitizer_flags.inc

197 ↗

(On Diff #42268)

add "(asan only)" to the descr string

[ygribov]

lib/sanitizer_common/sanitizer_flags.inc

197 ↗

(On Diff #42268)

Do we want to enable this for UBSan as well btw?

[kcc]

lib/sanitizer_common/sanitizer_flags.inc

197 ↗

(On Diff #42268)

It hasn't been a problem for us in the past.
Besides, ubsan has a mode with no runtime, so this flag will not work there.
But if you think we need it there, sure, why not.
(The code will need to be moved to sanitizer_common then)

[samsonov]

lib/sanitizer_common/sanitizer_flags.inc

197 ↗

(On Diff #42268)

UBSan already has a somewhat robust deduplication based on source locations.

[ygribov]

lib/sanitizer_common/sanitizer_flags.inc

197 ↗

(On Diff #42268)

Yeah, I meant a flag to disable those.

compiler-rt/trunk/lib/asan/asan_report.cc

 
 namespace __asan {
 
 // -------------------- User-specified callbacks ----------------- {{{1
 static void (*error_report_callback)(const char*);
 static char *error_message_buffer = nullptr;
 static uptr error_message_buffer_pos = 0;
 static BlockingMutex error_message_buf_mutex(LINKER_INITIALIZED);
+static const unsigned kAsanBuggyPcPoolSize = 25;
+static __sanitizer::atomic_uint64_t AsanBuggyPcPool[kAsanBuggyPcPoolSize];
 
 struct ReportData {
   uptr pc;
   uptr sp;
   uptr bp;
   uptr addr;
   bool is_write;
   uptr access_size;
void ReportMacMzReallocUnknown(uptr addr, uptr zone_ptr, const char *zone_name,
   Printf("mz_realloc(%p) -- attempting to realloc unallocated memory.\n"
              "This is an unrecoverable problem, exiting now.\n",
              addr);
   PrintZoneForPointer(addr, zone_ptr, zone_name);
   stack->Print();
   DescribeHeapAddress(addr, 1);
 }
 
+// -------------- SuppressErrorReport -------------- {{{1
+// Avoid error reports duplicating for ASan recover mode.
+static bool SuppressErrorReport(uptr pc) {
+  if (!common_flags()->suppress_equal_pcs) return false;
+  for (unsigned i = 0; i < kAsanBuggyPcPoolSize; i++) {
+    u64 cmp = atomic_load_relaxed(&AsanBuggyPcPool[i]);
+    if (cmp == 0 && atomic_compare_exchange_strong(&AsanBuggyPcPool[i], &cmp,
+                                                   pc, memory_order_relaxed))
+      return false;
+    if (cmp == pc) return true;
+  }
+  Die();
+}
+
 void ReportGenericError(uptr pc, uptr bp, uptr sp, uptr addr, bool is_write,
                         uptr access_size, u32 exp, bool fatal) {
+  if (!fatal && SuppressErrorReport(pc)) return;
   ENABLE_FRAME_POINTER;
 
   // Optimization experiments.
   // The experiments can be used to evaluate potential optimizations that remove
   // instrumentation (assess false negatives). Instead of completely removing
   // some instrumentation, compiler can emit special calls into runtime
   // (e.g. __asan_report_exp_load1 instead of __asan_report_load1) and pass
   // mask of experiments (exp).

compiler-rt/trunk/lib/sanitizer_common/sanitizer_flags.inc

COMMON_FLAG(bool, decorate_proc_maps, false, "If set, decorate sanitizer "
                                              "mappings in /proc/self/maps with "
                                              "user-readable names")
 COMMON_FLAG(int, exitcode, 1, "Override the program exit status if the tool "
                               "found an error")
 COMMON_FLAG(
     bool, abort_on_error, SANITIZER_MAC,
     "If set, the tool calls abort() instead of _exit() after printing the "
     "error report.")
+COMMON_FLAG(bool, suppress_equal_pcs, true,
+            "Deduplicate multiple reports for single source location in "
+            "halt_on_error=false mode (asan only).")

compiler-rt/trunk/test/asan/TestCases/Posix/halt_on_error-signals.c

 // Test interaction of Asan recovery mode with asynch signals.
 //
 // RUN: %clang_asan -fsanitize-recover=address -pthread %s -o %t
 //
 // RUN: rm -f %t.log
-// RUN: %env_asan_opts=halt_on_error=false %run %t 100 >%t.log 2>&1 || true
+// RUN: %env_asan_opts=halt_on_error=false:suppress_equal_pcs=false %run %t 100 >%t.log 2>&1 || true
 // Collision will almost always get triggered but we still need to check the unlikely case:
 // RUN: FileCheck --check-prefix=CHECK-COLLISION %s < %t.log || FileCheck --check-prefix=CHECK-NO-COLLISION %s < %t.log
 
 #define _SVID_SOURCE 1  // SA_NODEFER
 
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>

compiler-rt/trunk/test/asan/TestCases/Posix/halt_on_error-torture.cc

 // Stress test recovery mode with many threads.
 //
 // RUN: %clangxx_asan -fsanitize-recover=address -pthread %s -o %t
 //
-// RUN: %env_asan_opts=halt_on_error=false %run %t 1 10 >1.txt 2>&1
+// RUN: %env_asan_opts=halt_on_error=false:suppress_equal_pcs=false %run %t 1 10 >1.txt 2>&1
 // RUN: FileCheck %s < 1.txt
 // RUN: [ $(grep -c 'ERROR: AddressSanitizer: use-after-poison' 1.txt) -eq 10 ]
 // RUN: FileCheck --check-prefix=CHECK-NO-COLLISION %s < 1.txt
 //
 // Collisions are unlikely but still possible so we need the ||.
+// RUN: %env_asan_opts=halt_on_error=false:suppress_equal_pcs=false %run %t 10 20 >10.txt 2>&1 || true
+// This one is racy although _very_ unlikely to fail:
+// RUN: FileCheck %s < 10.txt
+// RUN: FileCheck --check-prefix=CHECK-COLLISION %s < 1.txt || FileCheck --check-prefix=CHECK-NO-COLLISION %s < 1.txt
+//
+// Collisions are unlikely but still possible so we need the ||.
 // RUN: %env_asan_opts=halt_on_error=false %run %t 10 20 >10.txt 2>&1 || true
 // This one is racy although _very_ unlikely to fail:
 // RUN: FileCheck %s < 10.txt
 // RUN: FileCheck --check-prefix=CHECK-COLLISION %s < 1.txt || FileCheck --check-prefix=CHECK-NO-COLLISION %s < 1.txt
 
 #include <stdio.h>
 #include <stdlib.h>
 #include <pthread.h>

compiler-rt/trunk/test/asan/TestCases/halt_on_error_suppress_equal_pcs.cc

+// Test reports dedupication for recovery mode.
+//
+// RUN: %clang_asan -fsanitize-recover=address %s -o %t
+//
+// Check for reports dedupication.
+// RUN: %env_asan_opts=halt_on_error=false %run %t 2>&1 | FileCheck %s
+//
+// Check that we die after reaching different reports number threshold.
+// RUN: %env_asan_opts=halt_on_error=false not %run %t 1 > %t1.log 2>&1
+// RUN: [ $(grep -c 'ERROR: AddressSanitizer: stack-buffer-overflow' %t1.log) -eq 25 ]
+//
+// Check suppress_equal_pcs=true behavior is equal to default one.
+// RUN: %env_asan_opts=halt_on_error=false:suppress_equal_pcs=true %run %t 2>&1 | FileCheck %s
+//
+// Check suppress_equal_pcs=false behavior isn't equal to default one.
+// RUN: %env_asan_opts=halt_on_error=false:suppress_equal_pcs=false %run %t > %t2.log 2>&1
+// RUN: [ $(grep -c 'ERROR: AddressSanitizer: stack-buffer-overflow' %t2.log) -eq 30 ]
+
+#define ACCESS_ARRAY_FIVE_ELEMENTS(array, i)     \
+  array[i] = i;                                  \
+  array[i + 1] = i + 1;                          \
+  array[i + 2] = i + 2;                          \
+  array[i + 3] = i + 3;                          \
+  array[i + 4] = i + 4;                          \
+
+volatile int ten = 10;
+unsigned kNumIterations = 10;
+
+int main(int argc, char **argv) {
+  char a[10];
+  char b[10];
+
+  if (argc == 1) {
+    for (int i = 0; i < kNumIterations; ++i) {
+      // CHECK: READ of size 1
+      volatile int res = a[ten + i];
+      // CHECK: WRITE of size 1
+      a[i + ten] = res + 3;
+      // CHECK: READ of size 1
+      res = a[ten + i];
+      // CHECK-NOT: ERROR
+    }
+  } else {
+    for (int i = 0; i < kNumIterations; ++i) {
+      ACCESS_ARRAY_FIVE_ELEMENTS(a, ten);
+      ACCESS_ARRAY_FIVE_ELEMENTS(a, ten + 5);
+      ACCESS_ARRAY_FIVE_ELEMENTS(a, ten + 10);
+      ACCESS_ARRAY_FIVE_ELEMENTS(b, ten);
+      ACCESS_ARRAY_FIVE_ELEMENTS(b, ten + 5);
+      ACCESS_ARRAY_FIVE_ELEMENTS(b, ten + 10);
+    }
+  }
+  return 0;
+}
+