diff --git a/compiler-rt/lib/tsan/rtl/tsan_defs.h b/compiler-rt/lib/tsan/rtl/tsan_defs.h index 2e13e0e5486b51..1ffa3d6aec40bd 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 9a6400c2e9f954..6c5835fbdc1098 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 e77bfba277a577..7d771bfaad7f71 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 5d31005c2af0a0..2e978852ea7d37 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 00000000000000..549f1dc2767237 --- /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 756cb6488b4c77..9e1b3edfeb5480 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