From f831d6fc800ccf22c1c09888fce3e3c8ebc2c992 Mon Sep 17 00:00:00 2001 From: Dmitry Vyukov Date: Mon, 7 Mar 2022 20:30:38 +0100 Subject: [PATCH] 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 | 1 + compiler-rt/lib/tsan/rtl/tsan_fd.cpp | 42 +++++++++++++++++----------- compiler-rt/lib/tsan/rtl/tsan_rtl_access.cpp | 8 +++--- compiler-rt/lib/tsan/rtl/tsan_rtl_mutex.cpp | 3 +- compiler-rt/test/tsan/fd_close_race.cpp | 26 +++++++++++++++++ compiler-rt/test/tsan/stress.cpp | 10 ++++++- 6 files changed, 67 insertions(+), 23 deletions(-) create mode 100644 compiler-rt/test/tsan/fd_close_race.cpp diff --git a/compiler-rt/lib/tsan/rtl/tsan_defs.h b/compiler-rt/lib/tsan/rtl/tsan_defs.h index 2e13e0e..1ffa3d6 100644 --- a/compiler-rt/lib/tsan/rtl/tsan_defs.h +++ b/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. diff --git a/compiler-rt/lib/tsan/rtl/tsan_fd.cpp b/compiler-rt/lib/tsan/rtl/tsan_fd.cpp index 9a6400c..6c5835f 100644 --- a/compiler-rt/lib/tsan/rtl/tsan_fd.cpp +++ b/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; diff --git a/compiler-rt/lib/tsan/rtl/tsan_rtl_access.cpp b/compiler-rt/lib/tsan/rtl/tsan_rtl_access.cpp index e77bfba..7d771bf 100644 --- a/compiler-rt/lib/tsan/rtl/tsan_rtl_access.cpp +++ b/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(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); diff --git a/compiler-rt/lib/tsan/rtl/tsan_rtl_mutex.cpp b/compiler-rt/lib/tsan/rtl/tsan_rtl_mutex.cpp index 5d31005..2e97885 100644 --- a/compiler-rt/lib/tsan/rtl/tsan_rtl_mutex.cpp +++ b/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); diff --git a/compiler-rt/test/tsan/fd_close_race.cpp b/compiler-rt/test/tsan/fd_close_race.cpp new file mode 100644 index 0000000..549f1dc2 --- /dev/null +++ b/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 +#include +#include + +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 diff --git a/compiler-rt/test/tsan/stress.cpp b/compiler-rt/test/tsan/stress.cpp index 756cb64..9e1b3ed 100644 --- a/compiler-rt/test/tsan/stress.cpp +++ b/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 -- 2.7.4