[compiler-rt] f831d6f - tsan: fix false positive during fd close
Dmitry Vyukov via llvm-commits
llvm-commits at lists.llvm.org
Tue Mar 8 01:41:02 PST 2022
Author: Dmitry Vyukov
Date: 2022-03-08T10:40:56+01:00
New Revision: f831d6fc800ccf22c1c09888fce3e3c8ebc2c992
URL: https://github.com/llvm/llvm-project/commit/f831d6fc800ccf22c1c09888fce3e3c8ebc2c992
DIFF: https://github.com/llvm/llvm-project/commit/f831d6fc800ccf22c1c09888fce3e3c8ebc2c992.diff
LOG: 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
Added:
compiler-rt/test/tsan/fd_close_race.cpp
Modified:
compiler-rt/lib/tsan/rtl/tsan_defs.h
compiler-rt/lib/tsan/rtl/tsan_fd.cpp
compiler-rt/lib/tsan/rtl/tsan_rtl_access.cpp
compiler-rt/lib/tsan/rtl/tsan_rtl_mutex.cpp
compiler-rt/test/tsan/stress.cpp
Removed:
################################################################################
diff --git a/compiler-rt/lib/tsan/rtl/tsan_defs.h b/compiler-rt/lib/tsan/rtl/tsan_defs.h
index 2e13e0e5486b5..1ffa3d6aec40b 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 9a6400c2e9f95..6c5835fbdc109 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 e77bfba277a57..7d771bfaad7f7 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<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);
diff --git a/compiler-rt/lib/tsan/rtl/tsan_rtl_mutex.cpp b/compiler-rt/lib/tsan/rtl/tsan_rtl_mutex.cpp
index 5d31005c2af0a..2e978852ea7d3 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 0000000000000..549f1dc276723
--- /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 <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
diff --git a/compiler-rt/test/tsan/stress.cpp b/compiler-rt/test/tsan/stress.cpp
index 756cb6488b4c7..9e1b3edfeb548 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
More information about the llvm-commits
mailing list