tsan: fix false positive during fd close

[dvyukov]

Summary

FdClose is a subjet to the same atomicity problem as MemoryRangeFreed
(memory state is not "monotoic" wrt race detection).
So we need to lock the thread slot in FdClose the same way we do
in MemoryRangeFreed.
This fixes the modified stress.cpp test.

[melver]

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

211

There doesn't appear to be an actual comment change here - clang-format changed this?

[dvyukov]

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

211

My patch changes indentation level, so 'arc diff' reformatted it as it crossed 80 cols boundary I guess.

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

enum : AccessType {
   kAccessWrite = 0,
   kAccessRead = 1 << 0,
   kAccessAtomic = 1 << 1,
   kAccessVptr = 1 << 2,  // read or write of an object virtual table pointer
   kAccessFree = 1 << 3,  // synthetic memory access during memory freeing
   kAccessExternalPC = 1 << 4,  // access PC can have kExternalPCBit set
   kAccessCheckOnly = 1 << 5,   // check for races, but don't store
   kAccessNoRodata = 1 << 6,    // don't check for .rodata marker
+  kAccessSlotLocked = 1 << 7,  // memory access with TidSlot locked
 };
 
 // Descriptor of user's memory block.
 struct MBlock {
   u64  siz : 48;
   u64  tag : 16;
   StackID stk;
   Tid tid;

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

void FdAccess(ThreadState *thr, uptr pc, int fd) {
   MemoryAccess(thr, pc, (uptr)d, 8, kAccessRead);
 }
 
 void FdClose(ThreadState *thr, uptr pc, int fd, bool write) {
   DPrintf("#%d: FdClose(%d)\n", thr->tid, fd);
   if (bogusfd(fd))
     return;
   FdDesc *d = fddesc(thr, pc, fd);
+  {
+    // Need to lock the slot to make MemoryAccess and MemoryResetRange atomic
+    // with respect to global reset. See the comment in MemoryRangeFreed.
+    SlotLocker locker(thr);
     if (!MustIgnoreInterceptor(thr)) {
       if (write) {
         // To catch races between fd usage and close.
-      MemoryAccess(thr, pc, (uptr)d, 8, kAccessWrite);
+        MemoryAccess(thr, pc, (uptr)d, 8,
+                     kAccessWrite | kAccessCheckOnly | kAccessSlotLocked);
       } else {
         // This path is used only by dup2/dup3 calls.
         // We do read instead of write because there is a number of legitimate
         // cases where write would lead to false positives:
-      // 1. Some software dups a closed pipe in place of a socket before closing
+        // 1. Some software dups a closed pipe in place of a socket before
+        // closing
         //    the socket (to prevent races actually).
         // 2. Some daemons dup /dev/null in place of stdin/stdout.
         // On the other hand we have not seen cases when write here catches real
         // bugs.
-      MemoryAccess(thr, pc, (uptr)d, 8, kAccessRead);
+        MemoryAccess(thr, pc, (uptr)d, 8,
+                     kAccessRead | kAccessCheckOnly | kAccessSlotLocked);
       }
     }
     // We need to clear it, because if we do not intercept any call out there
     // that creates fd, we will hit false postives.
     MemoryResetRange(thr, pc, (uptr)d, 8);
+  }
   unref(thr, pc, d->sync);
   d->sync = 0;
   d->creation_tid = kInvalidTid;
   d->creation_stack = kInvalidStackID;
 }
 
 void FdFileCreate(ThreadState *thr, uptr pc, int fd) {
   DPrintf("#%d: FdFileCreate(%d)\n", thr->tid, fd);

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

NOINLINE void DoReportRace(ThreadState* thr, RawShadow* shadow_mem, Shadow cur,
   // This prevents trapping on this address in future.
   for (uptr i = 0; i < kShadowCnt; i++)
     StoreShadow(&shadow_mem[i], i == 0 ? Shadow::kRodata : Shadow::kEmpty);
   // See the comment in MemoryRangeFreed as to why the slot is locked
   // for free memory accesses. ReportRace must not be called with
   // the slot locked because of the fork. But MemoryRangeFreed is not
   // called during fork because fork sets ignore_reads_and_writes,
   // so simply unlocking the slot should be fine.
-  if (typ & kAccessFree)
+  if (typ & kAccessSlotLocked)
     SlotUnlock(thr);
   ReportRace(thr, shadow_mem, cur, Shadow(old), typ);
-  if (typ & kAccessFree)
+  if (typ & kAccessSlotLocked)
     SlotLock(thr);
 }
 
 #if !TSAN_VECTORIZE
 ALWAYS_INLINE
 bool ContainsSameAccess(RawShadow* s, Shadow cur, int unused0, int unused1,
                         AccessType typ) {
   for (uptr i = 0; i < kShadowCnt; i++) {
void MemoryRangeFreed(ThreadState* thr, uptr pc, uptr addr, uptr size) {
   // so it's still good to access. It's not the case with data races.
   DCHECK(thr->slot_locked);
   DCHECK_EQ(addr % kShadowCell, 0);
   size = RoundUp(size, kShadowCell);
   // Processing more than 1k (2k of shadow) is expensive,
   // can cause excessive memory consumption (user does not necessary touch
   // the whole range) and most likely unnecessary.
   size = Min<uptr>(size, 1024);
-  const AccessType typ =
-      kAccessWrite | kAccessFree | kAccessCheckOnly | kAccessNoRodata;
+  const AccessType typ = kAccessWrite | kAccessFree | kAccessSlotLocked |
+                         kAccessCheckOnly | kAccessNoRodata;
   TraceMemoryAccessRange(thr, pc, addr, size, typ);
   RawShadow* shadow_mem = MemToShadow(addr);
   Shadow cur(thr->fast_state, 0, kShadowCell, typ);
 #if TSAN_VECTORIZE
   const m128 access = _mm_set1_epi32(static_cast<u32>(cur.raw()));
   const m128 freed = _mm_setr_epi32(
       static_cast<u32>(Shadow::FreedMarker()),
       static_cast<u32>(Shadow::FreedInfo(cur.sid(), cur.epoch())), 0, 0);

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

SlotLocker locker(thr);
           !s->IsFlagSet(MutexFlagBroken)) {
         s->SetFlags(MutexFlagBroken);
         unlock_locked = true;
       }
       s->Reset();
     }
     // Imitate a memory write to catch unlock-destroy races.
     if (pc && IsAppMem(addr))
-      MemoryAccess(thr, pc, addr, 1, kAccessWrite | kAccessFree);
+      MemoryAccess(thr, pc, addr, 1,
+                   kAccessWrite | kAccessFree | kAccessSlotLocked);
   }
   if (unlock_locked && ShouldReport(thr, ReportTypeMutexDestroyLocked))
     ReportDestroyLocked(thr, pc, addr, last_lock, creation_stack_id);
   thr->mset.DelAddr(addr, true);
   // s will be destroyed and freed in MetaMap::FreeBlock.
 }
 
 void MutexPreLock(ThreadState *thr, uptr pc, uptr addr, u32 flagz) {

compiler-rt/test/tsan/fd_close_race.cpp

+// RUN: %clangxx_tsan -O1 %s -o %t && %deflake %run %t | FileCheck %s
+#include "test.h"
+#include <fcntl.h>
+#include <sys/stat.h>
+#include <sys/types.h>
+
+void *Thread(void *arg) {
+  char buf;
+  read((long)arg, &buf, 1);
+  barrier_wait(&barrier);
+  return NULL;
+}
+
+int main() {
+  barrier_init(&barrier, 2);
+  int fd = open("/dev/random", O_RDONLY);
+  pthread_t t;
+  pthread_create(&t, NULL, Thread, (void *)(long)fd);
+  barrier_wait(&barrier);
+  close(fd);
+  pthread_join(t, NULL);
+  fprintf(stderr, "DONE\n");
+}
+
+// CHECK: WARNING: ThreadSanitizer: data race
+// CHECK: DONE

compiler-rt/test/tsan/stress.cpp

 __attribute__((noinline)) void *SecondaryThread(void *x) {
   __atomic_fetch_add(&atomic, 1, __ATOMIC_ACQ_REL);
   return NULL;
 }
 
 void *Thread(void *x) {
   const int me = (long)x;
   volatile long sink = 0;
+  int fd = -1;
   while (!stop) {
     // If me == 0, we do all of the following,
     // otherwise only 1 type of action.
     if (me == 0 || me == 1) {
       // just read the stop variable
     }
     if (me == 0 || me == 2) {
       __atomic_store_n(&atomic, sink, __ATOMIC_RELEASE);
while (!stop) {
     }
     if (me == 0 || me == 9) {
 #if RACE
       sink += racy++;
 #else
       sink += racy;
 #endif
     }
+    if (me == 0 || me == 10) {
+      fd = open("/dev/null", O_RDONLY);
+      if (fd != -1) {
+        close(fd);
+        fd = -1;
+      }
+    }
     // If you add more actions, update kActions in main.
   }
   return NULL;
 }
 
 int main() {
   ANNOTATE_BENIGN_RACE(stop);
   if (pipe(fds))
     exit((perror("pipe"), 1));
   if (fcntl(fds[0], F_SETFL, O_NONBLOCK))
     exit((perror("fcntl"), 1));
   if (fcntl(fds[1], F_SETFL, O_NONBLOCK))
     exit((perror("fcntl"), 1));
-  const int kActions = 10;
+  const int kActions = 11;
 #if RACE
   const int kMultiplier = 1;
 #else
   const int kMultiplier = 4;
 #endif
   pthread_t t[kActions * kMultiplier];
   for (int i = 0; i < kActions * kMultiplier; i++)
     pthread_create(&t[i], NULL, Thread, (void *)(long)(i % kActions));