[tsan] Don't report bugs from interceptors called from libignored modules

[kubamracek]

Summary

I'd like to expand the ignore_noninstrumented_modules=1 flag to cover all report types (it currently only suppresses data race detection). We're seeing some false positives from system libraries where they guard the mutex acquisition via atomics (so the deadlock cannot happen). The general approach that I'd like to achieve with ignore_noninstrumented_modules=1 is to only report bugs where we know they actually come from instrumented code, regardless of bug type (race, mutex problem, deadlock).

This patch make sure we don't report deadlocks and other bug types when we're inside an interceptor that was called from a noninstrumented module (when ignore_noninstrumented_modules=1 is set). Adding a testcase that shows that deadlock detection still works on Darwin (to make sure we're not silencing too many reports).

[kcc]

I don't like this change -- it is very un-intuitive that reads/writes affects deadlock checker.
Also remember that deadlock detector does not require instrumentation at all.
Can we solve it with a blacklist?

[kubamracek]

remember that deadlock detector does not require instrumentation at all.

Unfortunately, the fact that TSan is able to detect issues on non-instrumented code brings more harm than benefits, at least on macOS/iOS. Almost all reports that don't come from instrumented code are false positives. Our users are usually fine with recompiling their source code with instrumentation and they expect to only find issues with their code, not some 3rd party library. Same applies to mutexes and deadlocks. If a user wants to see reports from non-instrumented libraries, they can just turn the ignore_noninstrumented_modules flag to zero.

Can we solve it with a blacklist?

Maybe, but to use libignore (which I would need for this), the same patch would be needed. Libignore currently sets ignore_reads_and_writes, which doesn't prevent reporting deadlocks.

it is very un-intuitive that reads/writes affects deadlock checker.

That's why the patches renamed ignore_reads_and_writes to ignore_reads_writes_and_reports. Would it help if I added a separate ignore_reports (or even ignore_deadlocks) flag?

[dvyukov]

When an interceptor comes from a non-instrumented library, we set thr->in_ignored_lib=true. This causes SCOPED_TSAN_INTERCEPTOR to call the real function an skip all tsan processing. Consequently mutex lock/unlock operations in non-instrumented libraries are not handled by tsan already. So it's not clear to me how you get deadlock reports there. What am I missing?

One problem is if one mutex in deadlock cycles is taken from non-instrumented code, but the second mutex is taken from instrumented code, tsan won't report the deadlock on the mutex operations coming from non-instrumented code, but it will still report it on operations coming from instrumented code. So it seems to me that a better way to handle it is to hide all mutex operations in non-instrumented code from tsan. But it already seems to be happening, so I am confused.

[kubamracek]

In D31449#714004, @dvyukov wrote:

When an interceptor comes from a non-instrumented library, we set thr->in_ignored_lib=true. This causes SCOPED_TSAN_INTERCEPTOR to call the real function an skip all tsan processing. Consequently mutex lock/unlock operations in non-instrumented libraries are not handled by tsan already. So it's not clear to me how you get deadlock reports there. What am I missing?

There's two issues with the current state of things:

1. in_ignored_lib=true only affects second (nested) calls to other interceptor. So if a libignored library directly calls pthread_mutex_lock+pthread_mutex_unlock, those interceptors are still called and they detect deadlocks.
2. Even if libignore affects direct interceptor calls, we'd have even more issues, because the pthread_mutex_lock+pthread_mutex_unlock interceptors provide important synchronization and if we just ignore them, we'll get even more false positives.

What I'm suggesting is: Non-instrumented modules should still call interceptors like pthread_mutex_lock+pthread_mutex_unlock, but these should just not report any issues, including deadlocks. Do you disagree?

[dvyukov]

lib/tsan/dd/dd_rtl.cc

40

This is a Thread from dd_rtl.h, this is not ThreadState from tsan_rtl.h. It does not have that field.

[dvyukov]

1. s/ignore_reads_and_writes/ignore_reads_writes_and_reports/ does not look like a good idea to me, because these things are not necessary related. If we go this route we need separate flags.
2. From the description it seems that your intention is to ignore all reports, but you are ignoring only deadlocks. There are many more types of reports.

How about:

 bool OutputReport(ThreadState *thr, const ScopedReport &srep) {
-  if (!flags()->report_bugs)
+  if (!flags()->report_bugs || thr->suppress_reports)
     return false;
   atomic_store_relaxed(&ctx->last_symbolize_time_ns, NanoTime());
   const ReportDesc *rep = srep.GetReport();

This will be a minimalistic change and it will suppress all reports and won't affect anything other than non-instrumented libs.
But note that it will effectively nullify the external API thing as it will never report anything.

[kubamracek]

Addressing review comments, renaming.

From the description it seems that your intention is to ignore all reports, but you are ignoring only deadlocks. There are many more types of reports.

Right, this patch now suppresses all types.

note that it will effectively nullify the external API thing as it will never report anything.

I tested this and the external API still works fine. This is because this API isn't an interceptor and we're not using ScopedInterceptor in it.

[kubamracek]

ping

[kubamracek]

Is this okay to land?

lib/tsan/dd/dd_rtl.cc

 static void PrintStackTrace(Thread *thr, u32 stk) {
   StackTrace stack = StackDepotGet(stk);
   thr->ignore_interceptors = true;
   stack.Print();
   thr->ignore_interceptors = false;
 }
 
 static void ReportDeadlock(Thread *thr, DDReport *rep) {
-  if (rep == 0)
+  if (rep == 0 || thr->ignore_reads_writes_and_reports)
     return;
   BlockingMutexLock lock(&ctx->report_mutex);
   Printf("==============================\n");
   Printf("WARNING: lock-order-inversion (potential deadlock)\n");
   for (int i = 0; i < rep->n; i++) {
     Printf("Thread %d locks mutex %llu while holding mutex %llu:\n",
       rep->loop[i].thr_ctx, rep->loop[i].mtx_ctx1, rep->loop[i].mtx_ctx0);
     PrintStackTrace(thr, rep->loop[i].stk[1]);

lib/tsan/rtl/tsan_interceptors.cc

TSAN_INTERCEPTOR(void *, mmap, void *addr, SIZE_T sz, int prot, int flags,
   SCOPED_TSAN_INTERCEPTOR(mmap, addr, sz, prot, flags, fd, off);
   if (!fix_mmap_addr(&addr, sz, flags))
     return MAP_FAILED;
   void *res = REAL(mmap)(addr, sz, prot, flags, fd, off);
   if (res != MAP_FAILED) {
     if (fd > 0)
       FdAccess(thr, pc, fd);
 
-    if (thr->ignore_reads_and_writes == 0)
+    if (thr->ignore_reads_writes_and_reports == 0)
       MemoryRangeImitateWrite(thr, pc, (uptr)res, sz);
     else
       MemoryResetRange(thr, pc, (uptr)res, sz);
   }
   return res;
 }
 
 #if SANITIZER_LINUX
 TSAN_INTERCEPTOR(void *, mmap64, void *addr, SIZE_T sz, int prot, int flags,
                  int fd, OFF64_T off) {
   SCOPED_TSAN_INTERCEPTOR(mmap64, addr, sz, prot, flags, fd, off);
   if (!fix_mmap_addr(&addr, sz, flags))
     return MAP_FAILED;
   void *res = REAL(mmap64)(addr, sz, prot, flags, fd, off);
   if (res != MAP_FAILED) {
     if (fd > 0)
       FdAccess(thr, pc, fd);
 
-    if (thr->ignore_reads_and_writes == 0)
+    if (thr->ignore_reads_writes_and_reports == 0)
       MemoryRangeImitateWrite(thr, pc, (uptr)res, sz);
     else
       MemoryResetRange(thr, pc, (uptr)res, sz);
   }
   return res;
 }
 #define TSAN_MAYBE_INTERCEPT_MMAP64 TSAN_INTERCEPT(mmap64)
 #else

 static void CallUserSignalHandler(ThreadState *thr, bool sync, bool acquire,
     bool sigact, int sig, my_siginfo_t *info, void *uctx) {
   if (acquire)
     Acquire(thr, 0, (uptr)&sigactions[sig]);
   // Signals are generally asynchronous, so if we receive a signals when
   // ignores are enabled we should disable ignores. This is critical for sync
   // and interceptors, because otherwise we can miss syncronization and report
   // false races.
-  int ignore_reads_and_writes = thr->ignore_reads_and_writes;
+  int ignore_reads_writes_and_reports = thr->ignore_reads_writes_and_reports;
   int ignore_interceptors = thr->ignore_interceptors;
   int ignore_sync = thr->ignore_sync;
   if (!ctx->after_multithreaded_fork) {
-    thr->ignore_reads_and_writes = 0;
+    thr->ignore_reads_writes_and_reports = 0;
     thr->fast_state.ClearIgnoreBit();
     thr->ignore_interceptors = 0;
     thr->ignore_sync = 0;
   }
   // Ensure that the handler does not spoil errno.
   const int saved_errno = errno;
   errno = 99;
   // This code races with sigaction. Be careful to not read sa_sigaction twice.
   // Also need to remember pc for reporting before the call,
   // because the handler can reset it.
   volatile uptr pc = sigact ?
      (uptr)sigactions[sig].sa_sigaction :
      (uptr)sigactions[sig].sa_handler;
   if (pc != (uptr)SIG_DFL && pc != (uptr)SIG_IGN) {
     if (sigact)
       ((sigactionhandler_t)pc)(sig, info, uctx);
     else
       ((sighandler_t)pc)(sig);
   }
   if (!ctx->after_multithreaded_fork) {
-    thr->ignore_reads_and_writes = ignore_reads_and_writes;
-    if (ignore_reads_and_writes)
+    thr->ignore_reads_writes_and_reports = ignore_reads_writes_and_reports;
+    if (ignore_reads_writes_and_reports)
       thr->fast_state.SetIgnoreBit();
     thr->ignore_interceptors = ignore_interceptors;
     thr->ignore_sync = ignore_sync;
   }
   // We do not detect errno spoiling for SIGTERM,
   // because some SIGTERM handlers do spoil errno but reraise SIGTERM,
   // tsan reports false positive in such case.
   // It's difficult to properly detect this situation (reraise),

lib/tsan/rtl/tsan_mman.cc

void user_free(ThreadState *thr, uptr pc, void *p, bool signal) {
   allocator()->Deallocate(&thr->proc()->alloc_cache, p);
   if (signal)
     SignalUnsafeCall(thr, pc);
 }
 
 void OnUserAlloc(ThreadState *thr, uptr pc, uptr p, uptr sz, bool write) {
   DPrintf("#%d: alloc(%zu) = %p\n", thr->tid, sz, p);
   ctx->metamap.AllocBlock(thr, pc, p, sz);
-  if (write && thr->ignore_reads_and_writes == 0)
+  if (write && thr->ignore_reads_writes_and_reports == 0)
     MemoryRangeImitateWrite(thr, pc, (uptr)p, sz);
   else
     MemoryResetRange(thr, pc, (uptr)p, sz);
 }
 
 void OnUserFree(ThreadState *thr, uptr pc, uptr p, bool write) {
   CHECK_NE(p, (void*)0);
   uptr sz = ctx->metamap.FreeBlock(thr->proc(), p);
   DPrintf("#%d: free(%p, %zu)\n", thr->tid, p, sz);
-  if (write && thr->ignore_reads_and_writes == 0)
+  if (write && thr->ignore_reads_writes_and_reports == 0)
     MemoryRangeFreed(thr, pc, (uptr)p, sz);
 }
 
 void *user_realloc(ThreadState *thr, uptr pc, void *p, uptr sz) {
   // FIXME: Handle "shrinking" more efficiently,
   // it seems that some software actually does this.
   void *p2 = user_alloc(thr, pc, sz);
   if (p2 == 0)

lib/tsan/rtl/tsan_rtl.h

struct ThreadState {
   // QUESTION: can we can squeeze this into ThreadState::Fast?
   // E.g. ThreadState::Fast is a 44-bit, 32 are taken by synch_epoch and 12 are
   // taken by epoch between synchs.
   // This way we can save one load from tls.
   u64 fast_synch_epoch;
   // This is a slow path flag. On fast path, fast_state.GetIgnoreBit() is read.
   // We do not distinguish beteween ignoring reads and writes
   // for better performance.
-  int ignore_reads_and_writes;
+  int ignore_reads_writes_and_reports;
   int ignore_sync;
   // Go does not support ignores.
 #if !SANITIZER_GO
   IgnoreSet mop_ignore_set;
   IgnoreSet sync_ignore_set;
 #endif
   // C/C++ uses fixed size shadow stack embed into Trace.
   // Go uses malloc-allocated shadow stack with dynamic size.

lib/tsan/rtl/tsan_rtl.cc

 // The objects are allocated in TLS, so one may rely on zero-initialization.
 ThreadState::ThreadState(Context *ctx, int tid, int unique_id, u64 epoch,
                          unsigned reuse_count,
                          uptr stk_addr, uptr stk_size,
                          uptr tls_addr, uptr tls_size)
   : fast_state(tid, epoch)
   // Do not touch these, rely on zero initialization,
   // they may be accessed before the ctor.
-  // , ignore_reads_and_writes()
+  // , ignore_reads_writes_and_reports()
   // , ignore_interceptors()
   , clock(tid, reuse_count)
 #if !SANITIZER_GO
   , jmp_bufs(MBlockJmpBuf)
 #endif
   , tid(tid)
   , unique_id(unique_id)
   , stk_addr(stk_addr)

 #if !SANITIZER_GO
   DCHECK_LT(thr->shadow_stack_pos, thr->shadow_stack_end);
 #endif
   thr->shadow_stack_pos--;
 }
 
 void ThreadIgnoreBegin(ThreadState *thr, uptr pc, bool save_stack) {
   DPrintf("#%d: ThreadIgnoreBegin\n", thr->tid);
-  thr->ignore_reads_and_writes++;
-  CHECK_GT(thr->ignore_reads_and_writes, 0);
+  thr->ignore_reads_writes_and_reports++;
+  CHECK_GT(thr->ignore_reads_writes_and_reports, 0);
   thr->fast_state.SetIgnoreBit();
 #if !SANITIZER_GO
   if (save_stack && !ctx->after_multithreaded_fork)
     thr->mop_ignore_set.Add(CurrentStackId(thr, pc));
 #endif
 }
 
 void ThreadIgnoreEnd(ThreadState *thr, uptr pc) {
   DPrintf("#%d: ThreadIgnoreEnd\n", thr->tid);
-  CHECK_GT(thr->ignore_reads_and_writes, 0);
-  thr->ignore_reads_and_writes--;
-  if (thr->ignore_reads_and_writes == 0) {
+  CHECK_GT(thr->ignore_reads_writes_and_reports, 0);
+  thr->ignore_reads_writes_and_reports--;
+  if (thr->ignore_reads_writes_and_reports == 0) {
     thr->fast_state.ClearIgnoreBit();
 #if !SANITIZER_GO
     thr->mop_ignore_set.Reset();
 #endif
   }
 }
 
 #if !SANITIZER_GO

lib/tsan/rtl/tsan_rtl_mutex.cc

void AcquireReleaseImpl(ThreadState *thr, uptr pc, SyncClock *c) {
   thr->clock.set(thr->fast_state.epoch());
   thr->fast_synch_epoch = thr->fast_state.epoch();
   thr->clock.acq_rel(&thr->proc()->clock_cache, c);
   StatInc(thr, StatSyncAcquire);
   StatInc(thr, StatSyncRelease);
 }
 
 void ReportDeadlock(ThreadState *thr, uptr pc, DDReport *r) {
-  if (r == 0)
+  if (r == 0 || thr->ignore_reads_writes_and_reports)
     return;
   ThreadRegistryLock l(ctx->thread_registry);
   ScopedReport rep(ReportTypeDeadlock);
   for (int i = 0; i < r->n; i++) {
     rep.AddMutex(r->loop[i].mtx_ctx0);
     rep.AddUniqueTid((int)r->loop[i].thr_ctx);
     rep.AddThread((int)r->loop[i].thr_ctx);
   }

lib/tsan/rtl/tsan_rtl_report.cc

 void TsanCheckFailed(const char *file, int line, const char *cond,
                      u64 v1, u64 v2) {
   // There is high probability that interceptors will check-fail as well,
   // on the other hand there is no sense in processing interceptors
   // since we are going to die soon.
   ScopedIgnoreInterceptors ignore;
 #if !SANITIZER_GO
   cur_thread()->ignore_sync++;
-  cur_thread()->ignore_reads_and_writes++;
+  cur_thread()->ignore_reads_writes_and_reports++;
 #endif
   Printf("FATAL: ThreadSanitizer CHECK failed: "
          "%s:%d \"%s\" (0x%zx, 0x%zx)\n",
          file, line, cond, (uptr)v1, (uptr)v2);
   PrintCurrentStackSlow(StackTrace::GetCurrentPc());
   Die();
 }
 

lib/tsan/rtl/tsan_rtl_thread.cc

for (uptr i = 0; i < set->Size(); i++) {
     PrintStack(SymbolizeStackId(set->At(i)));
   }
   Die();
 }
 
 static void ThreadCheckIgnore(ThreadState *thr) {
   if (ctx->after_multithreaded_fork)
     return;
-  if (thr->ignore_reads_and_writes)
+  if (thr->ignore_reads_writes_and_reports)
     ReportIgnoresEnabled(thr->tctx, &thr->mop_ignore_set);
   if (thr->ignore_sync)
     ReportIgnoresEnabled(thr->tctx, &thr->sync_ignore_set);
 }
 #else
 static void ThreadCheckIgnore(ThreadState *thr) {}
 #endif
 

test/tsan/Darwin/deadlock.mm

+// RUN: %clang_tsan %s -o %t -framework Foundation
+// RUN: %deflake %run %t 2>&1 | FileCheck %s
+
+#import <Foundation/Foundation.h>
+
+#import "../test.h"
+
+pthread_mutex_t m1;
+pthread_mutex_t m2;
+
+int main(int argc, const char *argv[]) {
+  barrier_init(&barrier, 2);
+  fprintf(stderr, "Hello world.\n");
+
+  pthread_mutex_init(&m1, NULL);
+  pthread_mutex_init(&m2, NULL);
+  
+  [NSThread detachNewThreadWithBlock:^{
+    pthread_mutex_lock(&m1);
+    pthread_mutex_lock(&m2);
+    pthread_mutex_unlock(&m2);
+    pthread_mutex_unlock(&m1);
+
+    barrier_wait(&barrier);
+  }];
+  [NSThread detachNewThreadWithBlock:^{
+    barrier_wait(&barrier);
+
+    pthread_mutex_lock(&m2);
+    pthread_mutex_lock(&m1);
+    pthread_mutex_unlock(&m1);
+    pthread_mutex_unlock(&m2);
+
+    dispatch_sync(dispatch_get_main_queue(), ^{
+      CFRunLoopStop(CFRunLoopGetCurrent());
+    });
+  }];
+
+  CFRunLoopRun();
+  
+  fprintf(stderr, "Done.\n");
+  return 0;
+}
+
+// CHECK: Hello world.
+// CHECK: WARNING: ThreadSanitizer: lock-order-inversion (potential deadlock)
+// CHECK: Done.