tsan: fix false positive during fd close

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.

Reviewed By: vitalybuka, melver

Differential Revision: https://reviews.llvm.org/D121143

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

@@ -176,6 +176,7 @@ enum : AccessType {
   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.

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

@@ -195,25 +195,33 @@ void FdClose(ThreadState *thr, uptr pc, int fd, bool write) {
   if (bogusfd(fd))
     return;
   FdDesc *d = fddesc(thr, pc, fd);
-  if (!MustIgnoreInterceptor(thr)) {
-    if (write) {
-      // To catch races between fd usage and close.
-      MemoryAccess(thr, pc, (uptr)d, 8, kAccessWrite);
-    } 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
-      //    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);
+  {
+    // 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 | 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
+        //    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 | 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);
   }
-  // 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;

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

@@ -170,10 +170,10 @@ NOINLINE void DoReportRace(ThreadState* thr, RawShadow* shadow_mem, Shadow cur,
   // 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);
 }
 
@@ -611,8 +611,8 @@ void MemoryRangeFreed(ThreadState* thr, uptr pc, uptr addr, uptr size) {
   // 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);

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

@@ -128,7 +128,8 @@ void MutexDestroy(ThreadState *thr, uptr pc, uptr addr, u32 flagz) {
     }
     // 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);

compiler-rt/test/tsan/fd_close_race.cpp

@@ -0,0 +1,26 @@
+// 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

@@ -18,6 +18,7 @@ __attribute__((noinline)) void *SecondaryThread(void *x) {
 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.
@@ -57,6 +58,13 @@ void *Thread(void *x) {
       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;
@@ -70,7 +78,7 @@ int main() {
     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