[tsan] Keep thread/stack for closed FD

[vitalybuka]

Summary

Fixes b/242869837

[vitalybuka]

rebase

[vitalybuka]

-new line

[dvyukov]

compiler-rt/test/tsan/fd_location_closed.cpp

9

This barrier won't really ensure any particular ordering of the racing events, right?
Such tests w/o ordering used to be flaky. We generally create only 2 thread and fix the needed order of events with the barrier.

[vitalybuka]

update the test

compiler-rt/test/tsan/fd_location_closed.cpp

9

We need race between read and close, and then, when we start reporting, another close on the read thread must destroy the fd
If I put barrier like this it detects for me every time.
%deflake also should help

[vitalybuka]

compiler-rt/test/tsan/fd_location_closed.cpp

9

WARNING: ThreadSanitizer: data race (pid=4113552)
  Write of size 8 at 0x7bb000000060 by thread T2:
    #0 close compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp:1769:3 (fd_location_closed.cpp.tmp+0x58665)
    #1 Thread(void*) compiler-rt/test/tsan/fd_location_closed.cpp:11:3 (fd_location_closed.cpp.tmp+0xd59a5)

  Previous read of size 8 at 0x7bb000000060 by thread T1:
    #0 read compiler-rt/lib/tsan/rtl/../../sanitizer_common/sanitizer_common_interceptors.inc:1091:3 (fd_location_closed.cpp.tmp+0x5e684)
    #1 Thread(void*) compiler-rt/test/tsan/fd_location_closed.cpp:9:3 (fd_location_closed.cpp.tmp+0xd5992)

  Location is file descriptor 3 destroyed by thread T1 at:
    #0 close compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp:1769:3 (fd_location_closed.cpp.tmp+0x58665)
    #1 Thread(void*) compiler-rt/test/tsan/fd_location_closed.cpp:11:3 (fd_location_closed.cpp.tmp+0xd59a5)

  Thread T2 (tid=4113555, running) created by main thread at:
    #0 pthread_create compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp:1022:3 (fd_location_closed.cpp.tmp+0x537bd)
    #1 main compiler-rt/test/tsan/fd_location_closed.cpp:20:3 (fd_location_closed.cpp.tmp+0xd5a37)

  Thread T1 (tid=4113554, finished) created by main thread at:
    #0 pthread_create compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp:1022:3 (fd_location_closed.cpp.tmp+0x537bd)
    #1 main compiler-rt/test/tsan/fd_location_closed.cpp:19:3 (fd_location_closed.cpp.tmp+0xd5a22)

[DavidSpickett]

I see @dblaikie already reverted this but in case you want some output to look at here's one of Linaro's failing builds: https://lab.llvm.org/buildbot/#/builders/179/builds/4279

compiler-rt/lib/tsan/rtl/tsan_fd.h

 void FdSignalCreate(ThreadState *thr, uptr pc, int fd);
 void FdInotifyCreate(ThreadState *thr, uptr pc, int fd);
 void FdPollCreate(ThreadState *thr, uptr pc, int fd);
 void FdPollAdd(ThreadState *thr, uptr pc, int epfd, int fd);
 void FdSocketCreate(ThreadState *thr, uptr pc, int fd);
 void FdSocketAccept(ThreadState *thr, uptr pc, int fd, int newfd);
 void FdSocketConnecting(ThreadState *thr, uptr pc, int fd);
 void FdSocketConnect(ThreadState *thr, uptr pc, int fd);
-bool FdLocation(uptr addr, int *fd, Tid *tid, StackID *stack);
+bool FdLocation(uptr addr, int *fd, Tid *tid, StackID *stack, bool *closed);
 void FdOnFork(ThreadState *thr, uptr pc);
 
 uptr File2addr(const char *path);
 uptr Dir2addr(const char *path);
 
 }  // namespace __tsan
 
 #endif  // TSAN_INTERFACE_H

compiler-rt/lib/tsan/rtl/tsan_fd.cpp

 
 struct FdDesc {
   FdSync *sync;
   // This is used to establish write -> epoll_wait synchronization
   // where epoll_wait receives notification about the write.
   atomic_uintptr_t aux_sync;  // FdSync*
   Tid creation_tid;
   StackID creation_stack;
+  bool closed;
 };
 
 struct FdContext {
   atomic_uintptr_t tab[kTableSizeL1];
   // Addresses used for synchronization.
   FdSync globsync;
   FdSync filesync;
   FdSync socksync;
static void init(ThreadState *thr, uptr pc, int fd, FdSync *s,
   } else if (flags()->io_sync == 1) {
     d->sync = s;
   } else if (flags()->io_sync == 2) {
     unref(thr, pc, s);
     d->sync = &fdctx.globsync;
   }
   d->creation_tid = thr->tid;
   d->creation_stack = CurrentStackId(thr, pc);
+  d->closed = false;
   // This prevents false positives on fd_close_norace3.cpp test.
   // The mechanics of the false positive are not completely clear,
   // but it happens only if global reset is enabled (flush_memory_ms=1)
   // and may be related to lost writes during asynchronous MADV_DONTNEED.
   SlotLocker locker(thr);
   if (write) {
     // To catch races between fd usage and open.
     MemoryRangeImitateWrite(thr, pc, (uptr)d, 8);
if (tab == 0)
       break;
     for (int l2 = 0; l2 < kTableSizeL2; l2++) {
       FdDesc *d = &tab[l2];
       MemoryResetRange(thr, pc, (uptr)d, 8);
     }
   }
 }
 
-bool FdLocation(uptr addr, int *fd, Tid *tid, StackID *stack) {
+bool FdLocation(uptr addr, int *fd, Tid *tid, StackID *stack, bool *closed) {
   for (int l1 = 0; l1 < kTableSizeL1; l1++) {
     FdDesc *tab = (FdDesc*)atomic_load(&fdctx.tab[l1], memory_order_relaxed);
     if (tab == 0)
       break;
     if (addr >= (uptr)tab && addr < (uptr)(tab + kTableSizeL2)) {
       int l2 = (addr - (uptr)tab) / sizeof(FdDesc);
       FdDesc *d = &tab[l2];
       *fd = l1 * kTableSizeL1 + l2;
       *tid = d->creation_tid;
       *stack = d->creation_stack;
+      *closed = d->closed;
       return true;
     }
   }
   return false;
 }
 
 void FdAcquire(ThreadState *thr, uptr pc, int fd) {
   if (bogusfd(fd))
FdDesc *d = fddesc(thr, pc, fd);
     MemoryResetRange(thr, pc, (uptr)d, 8);
   }
   unref(thr, pc, d->sync);
   d->sync = 0;
   unref(thr, pc,
         reinterpret_cast<FdSync *>(
             atomic_load(&d->aux_sync, memory_order_relaxed)));
   atomic_store(&d->aux_sync, 0, memory_order_relaxed);
-  d->creation_tid = kInvalidTid;
-  d->creation_stack = kInvalidStackID;
+  d->closed = true;
+  d->creation_tid = thr->tid;
+  d->creation_stack = CurrentStackId(thr, pc);
 }
 
 void FdFileCreate(ThreadState *thr, uptr pc, int fd) {
   DPrintf("#%d: FdFileCreate(%d)\n", thr->tid, fd);
   if (bogusfd(fd))
     return;
   init(thr, pc, fd, &fdctx.filesync);
 }

compiler-rt/lib/tsan/rtl/tsan_report.h

 struct ReportLocation {
   ReportLocationType type = ReportLocationGlobal;
   DataInfo global = {};
   uptr heap_chunk_start = 0;
   uptr heap_chunk_size = 0;
   uptr external_tag = 0;
   Tid tid = kInvalidTid;
   int fd = 0;
+  bool fd_closed = false;
   bool suppressable = false;
   ReportStack *stack = nullptr;
 };
 
 struct ReportThread {
   Tid id;
   tid_t os_id;
   bool running;

compiler-rt/lib/tsan/rtl/tsan_report.cpp

if (!object_type) {
              thread_name(thrbuf, loc->tid));
     }
     print_stack = true;
   } else if (loc->type == ReportLocationStack) {
     Printf("  Location is stack of %s.\n\n", thread_name(thrbuf, loc->tid));
   } else if (loc->type == ReportLocationTLS) {
     Printf("  Location is TLS of %s.\n\n", thread_name(thrbuf, loc->tid));
   } else if (loc->type == ReportLocationFD) {
-    Printf("  Location is file descriptor %d created by %s at:\n",
-        loc->fd, thread_name(thrbuf, loc->tid));
+    Printf("  Location is file descriptor %d %s by %s at:\n", loc->fd,
+           loc->fd_closed ? "destroyed" : "created",
+           thread_name(thrbuf, loc->tid));
     print_stack = true;
   }
   Printf("%s", d.Default());
   if (print_stack)
     PrintStack(loc->stack);
 }
 
 static void PrintMutexShort(const ReportMutex *rm, const char *after) {

compiler-rt/lib/tsan/rtl/tsan_rtl_report.cpp

 
 void ScopedReportBase::AddLocation(uptr addr, uptr size) {
   if (addr == 0)
     return;
 #if !SANITIZER_GO
   int fd = -1;
   Tid creat_tid = kInvalidTid;
   StackID creat_stack = 0;
-  if (FdLocation(addr, &fd, &creat_tid, &creat_stack)) {
+  bool closed = false;
+  if (FdLocation(addr, &fd, &creat_tid, &creat_stack, &closed)) {
     auto *loc = New<ReportLocation>();
     loc->type = ReportLocationFD;
+    loc->fd_closed = closed;
     loc->fd = fd;
     loc->tid = creat_tid;
     loc->stack = SymbolizeStackId(creat_stack);
     rep_->locs.PushBack(loc);
-    ThreadContext *tctx = FindThreadByTidLocked(creat_tid);
-    if (tctx)
-      AddThread(tctx);
+    AddThread(creat_tid);
     return;
   }
   MBlock *b = 0;
   uptr block_begin = 0;
   Allocator *a = allocator();
   if (a->PointerIsMine((void*)addr)) {
     block_begin = (uptr)a->GetBlockBegin((void *)addr);
     if (block_begin)
       b = ctx->metamap.GetBlock(block_begin);
   }
   if (!b)
     b = JavaHeapBlock(addr, &block_begin);
   if (b != 0) {
     auto *loc = New<ReportLocation>();
     loc->type = ReportLocationHeap;
     loc->heap_chunk_start = block_begin;
     loc->heap_chunk_size = b->siz;
     loc->external_tag = b->tag;
     loc->tid = b->tid;
     loc->stack = SymbolizeStackId(b->stk);
     rep_->locs.PushBack(loc);
-    if (ThreadContext *tctx = FindThreadByTidLocked(b->tid))
-      AddThread(tctx);
+    AddThread(b->tid);
     return;
   }
   bool is_stack = false;
   if (ThreadContext *tctx = IsThreadStackOrTls(addr, &is_stack)) {
     auto *loc = New<ReportLocation>();
     loc->type = is_stack ? ReportLocationStack : ReportLocationTLS;
     loc->tid = tctx->tid;
     rep_->locs.PushBack(loc);

compiler-rt/test/tsan/fd_location_closed.cpp

+// RUN: %clangxx_tsan -O1 %s -o %t && %deflake %run %t |& FileCheck %s
+#include "test.h"
+
+#include <fcntl.h>
+
+void *Thread(void *x) {
+  int fd = (long)x;
+  barrier_wait(&barrier);
+  char buf;
+  read(fd, &buf, 1);
+  close(fd);
+  return NULL;
+}
+
+int main() {
+
+  const int N = 10;
+  barrier_init(&barrier, N);
+  int fd = open("/dev/random", O_RDONLY);
+  pthread_t t[N];
+  for (auto &thr : t)
+    pthread_create(&thr, NULL, Thread, (void *)(long)fd);
+
+  for (auto &thr : t)
+    pthread_join(thr, NULL);
+}
+
+// CHECK: WARNING: ThreadSanitizer: data race
+// CHECK:   Location is file descriptor {{[0-9]+}} destroyed by thread
+// CHECK:     #0 close
+// CHECK:     #1 Thread