[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

158

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

can't you make it __sanitizer::atomic_uint32_t ?

121

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

129

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

test/asan/TestCases/halt_on_error-2.c

7

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

Right, thank you.

121

Ok.

129

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

7

Ok.

[m.ostapenko]

Updating according to nits.

[kcc]

lib/asan/asan_rtl.cc

128

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

What's ASAN_SUPPRESS_EQUAL_PCS for?

test/asan/TestCases/halt_on_error-2.c

7

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

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

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

129

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

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

146

I believe we can get rid of pc_num completely.

147

This function seems to always return false.

150

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

5

Why no grep here?

8

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

26

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

5

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

no spaces around =

test/asan/TestCases/halt_on_error-2.c

2

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.

6

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

add "(asan only)" to the descr string

[ygribov]

lib/sanitizer_common/sanitizer_flags.inc

197

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

[kcc]

lib/sanitizer_common/sanitizer_flags.inc

197

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

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

[ygribov]

lib/sanitizer_common/sanitizer_flags.inc

197

Yeah, I meant a flag to disable those.

lib/asan/asan_rtl.cc

 #include "ubsan/ubsan_init.h"
 #include "ubsan/ubsan_platform.h"
 
 int __asan_option_detect_stack_use_after_return;  // Global interface symbol.
 uptr *__asan_test_only_reported_buggy_pointer;  // Used only for testing asan.
 
 namespace __asan {
 
+const unsigned kAsanBuggyPcPoolSize = 25;
+
 uptr AsanMappingProfile[kAsanMappingProfileSize];
+uptr AsanBuggyPcPool[kAsanBuggyPcPoolSize];
 
 static void AsanDie() {
   static atomic_uint32_t num_calls;
   if (atomic_fetch_add(&num_calls, 1, memory_order_relaxed) != 0) {
     // Don't die twice - run a busy loop.
     while (1) { }
   }
   if (flags()->sleep_before_dying) {
if (common_flags()->use_madv_dontdump)
     DontDumpShadowMemory(beg, size);
 }
 
 // --------------- LowLevelAllocateCallbac ---------- {{{1
 static void OnLowLevelAllocate(uptr ptr, uptr size) {
   PoisonShadow(ptr, size, kAsanInternalHeapMagic);
 }
 
+// -------------- SuppressErrorReport -------------- {{{1
+// We need to avoid error reports duplicating for ASan recover mode.
+static bool SuppressErrorReport(uptr pc) {
+  if (!common_flags()->suppress_equal_pcs) return false;
+  static __sanitizer::atomic_uint32_t pc_num_atomic;
+  u32 pc_num = atomic_fetch_add(&pc_num_atomic, 1, memory_order_acquire);
+  // We have exceeded reasonable number of different reports, subsequent
+  // errors are likely artificial, so die here.
+  if (pc_num >= kAsanBuggyPcPoolSize)
+    Die();
+
+  for (int i = pc_num; i >= 0; --i)
+    if (pc == AsanBuggyPcPool[i]) return true;
+
+  // Race here, because some other thread may increase pc_num. This would lead
+  // to some pool entries would contain zeroes. This is OK, because we don't
+  // need exact presicion. Don't use atomic write due to performance issue.
+  AsanBuggyPcPool[pc_num] = pc;
+  return false;
+}
+
 // -------------------------- Run-time entry ------------------- {{{1
 // exported functions
 #define ASAN_REPORT_ERROR(type, is_write, size)                     \
 extern "C" NOINLINE INTERFACE_ATTRIBUTE                             \
 void __asan_report_ ## type ## size(uptr addr) {                    \
   GET_CALLER_PC_BP_SP;                                              \
   ReportGenericError(pc, bp, sp, addr, is_write, size, 0, true);    \
 }                                                                   \
 extern "C" NOINLINE INTERFACE_ATTRIBUTE                             \
 void __asan_report_exp_ ## type ## size(uptr addr, u32 exp) {       \
   GET_CALLER_PC_BP_SP;                                              \
   ReportGenericError(pc, bp, sp, addr, is_write, size, exp, true);  \
 }                                                                   \
 extern "C" NOINLINE INTERFACE_ATTRIBUTE                             \
 void __asan_report_ ## type ## size ## _noabort(uptr addr) {        \
   GET_CALLER_PC_BP_SP;                                              \
+  if (SuppressErrorReport(pc)) return;                              \
   ReportGenericError(pc, bp, sp, addr, is_write, size, 0, false);   \
 }                                                                   \
 
 ASAN_REPORT_ERROR(load, false, 1)
 ASAN_REPORT_ERROR(load, false, 2)
 ASAN_REPORT_ERROR(load, false, 4)
 ASAN_REPORT_ERROR(load, false, 8)
 ASAN_REPORT_ERROR(load, false, 16)
 ASAN_REPORT_ERROR(store, true, 1)
 ASAN_REPORT_ERROR(store, true, 2)
 ASAN_REPORT_ERROR(store, true, 4)
 ASAN_REPORT_ERROR(store, true, 8)
 ASAN_REPORT_ERROR(store, true, 16)
 
 #define ASAN_REPORT_ERROR_N(type, is_write)                                 \
 extern "C" NOINLINE INTERFACE_ATTRIBUTE                                     \
 void __asan_report_ ## type ## _n(uptr addr, uptr size) {                   \
   GET_CALLER_PC_BP_SP;                                                      \
   ReportGenericError(pc, bp, sp, addr, is_write, size, 0, true);            \
 }                                                                           \
 extern "C" NOINLINE INTERFACE_ATTRIBUTE                                     \
 void __asan_report_exp_ ## type ## _n(uptr addr, uptr size, u32 exp) {      \
   GET_CALLER_PC_BP_SP;                                                      \
   ReportGenericError(pc, bp, sp, addr, is_write, size, exp, true);          \
 }                                                                           \
 extern "C" NOINLINE INTERFACE_ATTRIBUTE                                     \
 void __asan_report_ ## type ## _n_noabort(uptr addr, uptr size) {           \
   GET_CALLER_PC_BP_SP;                                                      \
+  if (SuppressErrorReport(pc)) return;                                      \
   ReportGenericError(pc, bp, sp, addr, is_write, size, 0, false);           \
 }                                                                           \
 
 ASAN_REPORT_ERROR_N(load, false)
 ASAN_REPORT_ERROR_N(store, true)
 
 #define ASAN_MEMORY_ACCESS_CALLBACK_BODY(type, is_write, size, exp_arg, fatal) \
     uptr sp = MEM_TO_SHADOW(addr);                                             \
     uptr s = size <= SHADOW_GRANULARITY ? *reinterpret_cast<u8 *>(sp)          \
                                         : *reinterpret_cast<u16 *>(sp);        \
     if (UNLIKELY(s)) {                                                         \
       if (UNLIKELY(size >= SHADOW_GRANULARITY ||                               \
                    ((s8)((addr & (SHADOW_GRANULARITY - 1)) + size - 1)) >=     \
                        (s8)s)) {                                               \
         if (__asan_test_only_reported_buggy_pointer) {                         \
           *__asan_test_only_reported_buggy_pointer = addr;                     \
         } else {                                                               \
           GET_CALLER_PC_BP_SP;                                                 \
+          if (SuppressErrorReport(pc)) return;                                 \
           ReportGenericError(pc, bp, sp, addr, is_write, size, exp_arg,        \
                               fatal);                                          \
         }                                                                      \
       }                                                                        \
     }
 
 #define ASAN_MEMORY_ACCESS_CALLBACK(type, is_write, size)                      \
   extern "C" NOINLINE INTERFACE_ATTRIBUTE                                      \
void __asan_exp_loadN(uptr addr, uptr size, u32 exp) {
   }
 }
 
 extern "C"
 NOINLINE INTERFACE_ATTRIBUTE
 void __asan_loadN_noabort(uptr addr, uptr size) {
   if (__asan_region_is_poisoned(addr, size)) {
     GET_CALLER_PC_BP_SP;
+    if (SuppressErrorReport(pc)) return;
     ReportGenericError(pc, bp, sp, addr, false, size, 0, false);
   }
 }
 
 extern "C"
 NOINLINE INTERFACE_ATTRIBUTE
 void __asan_storeN(uptr addr, uptr size) {
   if (__asan_region_is_poisoned(addr, size)) {
void __asan_exp_storeN(uptr addr, uptr size, u32 exp) {
   }
 }
 
 extern "C"
 NOINLINE INTERFACE_ATTRIBUTE
 void __asan_storeN_noabort(uptr addr, uptr size) {
   if (__asan_region_is_poisoned(addr, size)) {
     GET_CALLER_PC_BP_SP;
+    if (SuppressErrorReport(pc)) return;
     ReportGenericError(pc, bp, sp, addr, true, size, 0, false);
   }
 }
 
 // Force the linker to keep the symbols for various ASan interface functions.
 // We want to keep those in the executable in order to let the instrumented
 // dynamic libraries access the symbol even if it is not used by the executable
 // itself. This should help if the build system is removing dead code at link

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.")

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>

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 %run %t 10 20 >10.txt 2>&1 || true
+// RUN: %env_asan_opts=halt_on_error=false ASAN_SUPPRESS_EQUAL_PCS=0 %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>
 #include <time.h>

test/asan/TestCases/halt_on_error-2.c

+// Test reports dedupication for recovery mode.
+//
+// RUN: %clang_asan -fsanitize-recover=address %s -o %t
+// RUN: %env_asan_opts=halt_on_error=false not %run %t 2>&1
+
+volatile int ten = 10;
+unsigned kNumIterations = 10;
+
+int main() {
+  char x[10];
+  for (int i = 0; i < kNumIterations; ++i) {
+    // CHECK: READ of size 1
+    volatile int res = x[ten + i];
+    // CHECK: WRITE of size 1
+     x[i + ten] = res + 3;
+    // CHECK: READ of size 1
+     res = x[ten + i];
+    // CHECK-NOT: READ of size 1
+    // CHECK-NOT: WRITE of size 1
+  }
+  return  0;
+}

test/asan/TestCases/halt_on_error-3.c

+// Test reports dedupication for recovery mode.
+//
+// RUN: %clang_asan -fsanitize-recover=address %s -o %t
+// RUN: %env_asan_opts=halt_on_error=false %run %t >3.txt 2>&1 || true
+// RUN: [ $(grep -c 'ERROR: AddressSanitizer: stack-buffer-overflow' 3.txt) -eq 25 ]
+
+#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() {
+  char a[10];
+  char b[10];
+  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;
+}
+