tsan: fix handling of dup2

Previously tsan modelled dup2(oldfd, newfd) as write on newfd.
We hit several cases where the write 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.
So model dup2 as read on newfd instead.

llvm-svn: 240687

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

@@ -91,7 +91,8 @@ static FdDesc *fddesc(ThreadState *thr, uptr pc, int fd) {
 }
 
 // pd must be already ref'ed.
-static void init(ThreadState *thr, uptr pc, int fd, FdSync *s) {
+static void init(ThreadState *thr, uptr pc, int fd, FdSync *s,
+    bool write = true) {
   FdDesc *d = fddesc(thr, pc, fd);
   // As a matter of fact, we don't intercept all close calls.
   // See e.g. libc __res_iclose().
@@ -109,8 +110,13 @@ static void init(ThreadState *thr, uptr pc, int fd, FdSync *s) {
   }
   d->creation_tid = thr->tid;
   d->creation_stack = CurrentStackId(thr, pc);
-  // To catch races between fd usage and open.
-  MemoryRangeImitateWrite(thr, pc, (uptr)d, 8);
+  if (write) {
+    // To catch races between fd usage and open.
+    MemoryRangeImitateWrite(thr, pc, (uptr)d, 8);
+  } else {
+    // See the dup-related comment in FdClose.
+    MemoryRead(thr, pc, (uptr)d, kSizeLog8);
+  }
 }
 
 void FdInit() {
@@ -181,13 +187,25 @@ void FdAccess(ThreadState *thr, uptr pc, int fd) {
   MemoryRead(thr, pc, (uptr)d, kSizeLog8);
 }
 
-void FdClose(ThreadState *thr, uptr pc, int fd) {
+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);
-  // To catch races between fd usage and close.
-  MemoryWrite(thr, pc, (uptr)d, kSizeLog8);
+  if (write) {
+    // To catch races between fd usage and close.
+    MemoryWrite(thr, pc, (uptr)d, kSizeLog8);
+  } 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.
+    MemoryRead(thr, pc, (uptr)d, kSizeLog8);
+  }
   // 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);
@@ -204,15 +222,15 @@ void FdFileCreate(ThreadState *thr, uptr pc, int fd) {
   init(thr, pc, fd, &fdctx.filesync);
 }
 
-void FdDup(ThreadState *thr, uptr pc, int oldfd, int newfd) {
+void FdDup(ThreadState *thr, uptr pc, int oldfd, int newfd, bool write) {
   DPrintf("#%d: FdDup(%d, %d)\n", thr->tid, oldfd, newfd);
   if (bogusfd(oldfd) || bogusfd(newfd))
     return;
   // Ignore the case when user dups not yet connected socket.
   FdDesc *od = fddesc(thr, pc, oldfd);
   MemoryRead(thr, pc, (uptr)od, kSizeLog8);
-  FdClose(thr, pc, newfd);
-  init(thr, pc, newfd, ref(od->sync));
+  FdClose(thr, pc, newfd, write);
+  init(thr, pc, newfd, ref(od->sync), write);
 }
 
 void FdPipeCreate(ThreadState *thr, uptr pc, int rfd, int wfd) {

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

@@ -42,9 +42,9 @@ void FdInit();
 void FdAcquire(ThreadState *thr, uptr pc, int fd);
 void FdRelease(ThreadState *thr, uptr pc, int fd);
 void FdAccess(ThreadState *thr, uptr pc, int fd);
-void FdClose(ThreadState *thr, uptr pc, int fd);
+void FdClose(ThreadState *thr, uptr pc, int fd, bool write = true);
 void FdFileCreate(ThreadState *thr, uptr pc, int fd);
-void FdDup(ThreadState *thr, uptr pc, int oldfd, int newfd);
+void FdDup(ThreadState *thr, uptr pc, int oldfd, int newfd, bool write);
 void FdPipeCreate(ThreadState *thr, uptr pc, int rfd, int wfd);
 void FdEventCreate(ThreadState *thr, uptr pc, int fd);
 void FdSignalCreate(ThreadState *thr, uptr pc, int fd);

compiler-rt/lib/tsan/rtl/tsan_interceptors.cc

@@ -1519,7 +1519,7 @@ TSAN_INTERCEPTOR(int, dup, int oldfd) {
   SCOPED_TSAN_INTERCEPTOR(dup, oldfd);
   int newfd = REAL(dup)(oldfd);
   if (oldfd >= 0 && newfd >= 0 && newfd != oldfd)
-    FdDup(thr, pc, oldfd, newfd);
+    FdDup(thr, pc, oldfd, newfd, true);
   return newfd;
 }
 
@@ -1527,7 +1527,7 @@ TSAN_INTERCEPTOR(int, dup2, int oldfd, int newfd) {
   SCOPED_TSAN_INTERCEPTOR(dup2, oldfd, newfd);
   int newfd2 = REAL(dup2)(oldfd, newfd);
   if (oldfd >= 0 && newfd2 >= 0 && newfd2 != oldfd)
-    FdDup(thr, pc, oldfd, newfd2);
+    FdDup(thr, pc, oldfd, newfd2, false);
   return newfd2;
 }
 
@@ -1535,7 +1535,7 @@ TSAN_INTERCEPTOR(int, dup3, int oldfd, int newfd, int flags) {
   SCOPED_TSAN_INTERCEPTOR(dup3, oldfd, newfd, flags);
   int newfd2 = REAL(dup3)(oldfd, newfd, flags);
   if (oldfd >= 0 && newfd2 >= 0 && newfd2 != oldfd)
-    FdDup(thr, pc, oldfd, newfd2);
+    FdDup(thr, pc, oldfd, newfd2, false);
   return newfd2;
 }
 

compiler-rt/test/tsan/fd_dup_norace2.cc

@@ -0,0 +1,40 @@
+// RUN: %clangxx_tsan -O1 %s -o %t && %run %t 2>&1 | FileCheck %s
+#include "test.h"
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <fcntl.h>
+
+// dup2(oldfd, newfd) races with read(newfd).
+// This is not reported as race because:
+// 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.
+
+int fd;
+
+void *Thread(void *x) {
+  char buf;
+  if (read(fd, &buf, 1) != 1)
+    exit(printf("read failed\n"));
+  return 0;
+}
+
+int main() {
+  fd = open("/dev/random", O_RDONLY);
+  int fd2 = open("/dev/random", O_RDONLY);
+  if (fd == -1 || fd2 == -1)
+    exit(printf("open failed\n"));
+  pthread_t th;
+  pthread_create(&th, 0, Thread, 0);
+  if (dup2(fd2, fd) == -1)
+    exit(printf("dup2 failed\n"));
+  pthread_join(th, 0);
+  if (close(fd) == -1)
+    exit(printf("close failed\n"));
+  if (close(fd2) == -1)
+    exit(printf("close failed\n"));
+  printf("DONE\n");
+}
+
+// CHECK-NOT: WARNING: ThreadSanitizer: data race
+// CHECK: DONE

compiler-rt/test/tsan/fd_dup_race.cc

@@ -0,0 +1,33 @@
+// RUN: %clangxx_tsan -O1 %s -o %t && %deflake %run %t 2>&1 | FileCheck %s
+#include "test.h"
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <fcntl.h>
+
+// dup2(oldfd, newfd) races with close(newfd).
+
+int fd;
+
+void *Thread(void *x) {
+  barrier_wait(&barrier);
+  if (close(fd) == -1)
+    exit(printf("close failed\n"));
+  return 0;
+}
+
+int main() {
+  barrier_init(&barrier, 2);
+  fd = open("/dev/random", O_RDONLY);
+  int fd2 = open("/dev/random", O_RDONLY);
+  if (fd == -1 || fd2 == -1)
+    exit(printf("open failed\n"));
+  pthread_t th;
+  pthread_create(&th, 0, Thread, 0);
+  if (dup2(fd2, fd) == -1)
+    exit(printf("dup2 failed\n"));
+  barrier_wait(&barrier);
+  pthread_join(th, 0);
+  printf("DONE\n");
+}
+
+// CHECK: WARNING: ThreadSanitizer: data race