tsan: always handle closing of file descriptors

If we miss both close of a file descriptor and a subsequent open
if the same file descriptor number, we report false positives
between operations on the old and on the new descriptors.

There are lots of ways to create new file descriptors, but for closing
there is mostly close call. So we try to handle at least it.
However, if the close happens in an ignored library, we miss it
and start reporting false positives.

Handle closing of file descriptors always, even in ignored libraries
(as we do for malloc/free and other critical functions).
But don't imitate memory accesses on close for ignored libraries.

FdClose checks validity of the fd (fd >= 0) itself,
so remove the excessive checks in the callers.

Reviewed By: melver

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

3 files changed, 39 insertions, 36 deletions

diff --git a/compiler-rt/lib/tsan/rtl/tsan_fd.cpp b/compiler-rt/lib/tsan/rtl/tsan_fd.cpp
index 255ffa8..9a6400c 100644
--- a/compiler-rt/lib/tsan/rtl/tsan_fd.cpp
+++ b/compiler-rt/lib/tsan/rtl/tsan_fd.cpp
@@ -11,9 +11,12 @@
 //===----------------------------------------------------------------------===//
 
 #include "tsan_fd.h"
-#include "tsan_rtl.h"
+
 #include <sanitizer_common/sanitizer_atomic.h>
 
+#include "tsan_interceptors.h"
+#include "tsan_rtl.h"
+
 namespace __tsan {
 
 const int kTableSizeL1 = 1024;
@@ -192,19 +195,21 @@ void FdClose(ThreadState *thr, uptr pc, int fd, bool write) {
   if (bogusfd(fd))
     return;
   FdDesc *d = fddesc(thr, pc, fd);
-  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);
+  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);
+    }
   }
   // We need to clear it, because if we do not intercept any call out there
   // that creates fd, we will hit false postives.
diff --git a/compiler-rt/lib/tsan/rtl/tsan_interceptors.h b/compiler-rt/lib/tsan/rtl/tsan_interceptors.h
index 61dbb81..88a54b5 100644
--- a/compiler-rt/lib/tsan/rtl/tsan_interceptors.h
+++ b/compiler-rt/lib/tsan/rtl/tsan_interceptors.h
@@ -36,6 +36,10 @@ inline bool in_symbolizer() {
 }
 #endif
 
+inline bool MustIgnoreInterceptor(ThreadState *thr) {
+  return !thr->is_inited || thr->ignore_interceptors || thr->in_ignored_lib;
+}
+
 }  // namespace __tsan
 
 #define SCOPED_INTERCEPTOR_RAW(func, ...)            \
@@ -60,10 +64,10 @@ inline bool in_symbolizer() {
 #  define CHECK_REAL_FUNC(func) DCHECK(REAL(func))
 #endif
 
-#define SCOPED_TSAN_INTERCEPTOR(func, ...)                                \
-  SCOPED_INTERCEPTOR_RAW(func, __VA_ARGS__);                              \
-  CHECK_REAL_FUNC(func);                                                  \
-  if (!thr->is_inited || thr->ignore_interceptors || thr->in_ignored_lib) \
+#define SCOPED_TSAN_INTERCEPTOR(func, ...)   \
+  SCOPED_INTERCEPTOR_RAW(func, __VA_ARGS__); \
+  CHECK_REAL_FUNC(func);                     \
+  if (MustIgnoreInterceptor(thr))            \
     return REAL(func)(__VA_ARGS__);
 
 #define SCOPED_TSAN_INTERCEPTOR_USER_CALLBACK_START() \
diff --git a/compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp b/compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp
index 1ebc305..c4f43d8 100644
--- a/compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp
+++ b/compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp
@@ -1681,11 +1681,10 @@ TSAN_INTERCEPTOR(int, eventfd, unsigned initval, int flags) {
 
 #if SANITIZER_LINUX
 TSAN_INTERCEPTOR(int, signalfd, int fd, void *mask, int flags) {
-  SCOPED_TSAN_INTERCEPTOR(signalfd, fd, mask, flags);
-  if (fd >= 0)
-    FdClose(thr, pc, fd);
+  SCOPED_INTERCEPTOR_RAW(signalfd, fd, mask, flags);
+  FdClose(thr, pc, fd);
   fd = REAL(signalfd)(fd, mask, flags);
-  if (fd >= 0)
+  if (!MustIgnoreInterceptor(thr))
     FdSignalCreate(thr, pc, fd);
   return fd;
 }
@@ -1762,17 +1761,15 @@ TSAN_INTERCEPTOR(int, listen, int fd, int backlog) {
 }
 
 TSAN_INTERCEPTOR(int, close, int fd) {
-  SCOPED_TSAN_INTERCEPTOR(close, fd);
-  if (fd >= 0)
-    FdClose(thr, pc, fd);
+  SCOPED_INTERCEPTOR_RAW(close, fd);
+  FdClose(thr, pc, fd);
   return REAL(close)(fd);
 }
 
 #if SANITIZER_LINUX
 TSAN_INTERCEPTOR(int, __close, int fd) {
-  SCOPED_TSAN_INTERCEPTOR(__close, fd);
-  if (fd >= 0)
-    FdClose(thr, pc, fd);
+  SCOPED_INTERCEPTOR_RAW(__close, fd);
+  FdClose(thr, pc, fd);
   return REAL(__close)(fd);
 }
 #define TSAN_MAYBE_INTERCEPT___CLOSE TSAN_INTERCEPT(__close)
@@ -1783,13 +1780,10 @@ TSAN_INTERCEPTOR(int, __close, int fd) {
 // glibc guts
 #if SANITIZER_LINUX && !SANITIZER_ANDROID
 TSAN_INTERCEPTOR(void, __res_iclose, void *state, bool free_addr) {
-  SCOPED_TSAN_INTERCEPTOR(__res_iclose, state, free_addr);
+  SCOPED_INTERCEPTOR_RAW(__res_iclose, state, free_addr);
   int fds[64];
   int cnt = ExtractResolvFDs(state, fds, ARRAY_SIZE(fds));
-  for (int i = 0; i < cnt; i++) {
-    if (fds[i] > 0)
-      FdClose(thr, pc, fds[i]);
-  }
+  for (int i = 0; i < cnt; i++) FdClose(thr, pc, fds[i]);
   REAL(__res_iclose)(state, free_addr);
 }
 #define TSAN_MAYBE_INTERCEPT___RES_ICLOSE TSAN_INTERCEPT(__res_iclose)
@@ -1870,7 +1864,7 @@ TSAN_INTERCEPTOR(int, rmdir, char *path) {
 }
 
 TSAN_INTERCEPTOR(int, closedir, void *dirp) {
-  SCOPED_TSAN_INTERCEPTOR(closedir, dirp);
+  SCOPED_INTERCEPTOR_RAW(closedir, dirp);
   if (dirp) {
     int fd = dirfd(dirp);
     FdClose(thr, pc, fd);
@@ -2375,7 +2369,7 @@ static void HandleRecvmsg(ThreadState *thr, uptr pc,
 #define COMMON_INTERCEPTOR_FILE_CLOSE(ctx, file) \
   if (file) {                                    \
     int fd = fileno_unlocked(file);              \
-    if (fd >= 0) FdClose(thr, pc, fd);           \
+    FdClose(thr, pc, fd);                        \
   }
 
 #define COMMON_INTERCEPTOR_DLOPEN(filename, flag) \
@@ -2582,7 +2576,7 @@ static USED void syscall_release(uptr pc, uptr addr) {
 }
 
 static void syscall_fd_close(uptr pc, int fd) {
-  TSAN_SYSCALL();
+  auto *thr = cur_thread();
   FdClose(thr, pc, fd);
 }