* [PATCH] tsan: fix false positive for pthread_cond_clockwait
@ 2021-05-07 17:07 Michael de Lang
2021-05-13 7:33 ` Martin Liška
0 siblings, 1 reply; 4+ messages in thread
From: Michael de Lang @ 2021-05-07 17:07 UTC (permalink / raw)
To: gcc-patches
pthread_cond_clockwait isn't added to TSAN_INTERCEPTORS which leads to
false positives regarding double locking of a mutex. This was
uncovered by a user reporting an issue to the google sanitizer github:
https://github.com/google/sanitizers/issues/1259
This patch copies code from the fix made in llvm:
https://github.com/llvm/llvm-project/commit/16eb853ffdd1a1ad7c95455b7795c5f004402e46
However, because the tsan related source code hasn't been kept in sync
with llvm, I had to make some modifications.
Given that this is my first contibution to gcc, let me know if I've
missed anything.
Met vriendelijke groet,
Michael de Lang
+++ b/gcc/testsuite/g++.dg/tsan/pthread_cond_clockwait.C
@@ -0,0 +1,31 @@
+// Test pthread_cond_clockwait not generating false positives with tsan
+// { dg-do run { target { { *-*-linux* *-*-gnu* *-*-uclinux* } && pthread } } }
+// { dg-options "-fsanitize=thread -lpthread" }
+
+#include <pthread.h>
+
+pthread_cond_t cv;
+pthread_mutex_t mtx;
+
+void *fn(void *vp) {
+ pthread_mutex_lock(&mtx);
+ pthread_cond_signal(&cv);
+ pthread_mutex_unlock(&mtx);
+ return NULL;
+}
+
+int main() {
+ pthread_mutex_lock(&mtx);
+
+ pthread_t tid;
+ pthread_create(&tid, NULL, fn, NULL);
+
+ struct timespec ts;
+ clock_gettime(CLOCK_MONOTONIC, &ts);
+ ts.tv_sec += 10;
+ pthread_cond_clockwait(&cv, &mtx, CLOCK_MONOTONIC, &ts);
+ pthread_mutex_unlock(&mtx);
+
+ pthread_join(tid, NULL);
+ return 0;
+}
diff --git a/libsanitizer/tsan/tsan_interceptors_posix.cpp
b/libsanitizer/tsan/tsan_interceptors_posix.cpp
index aa04d8dfb67..7b3d0a917de 100644
--- a/libsanitizer/tsan/tsan_interceptors_posix.cpp
+++ b/libsanitizer/tsan/tsan_interceptors_posix.cpp
@@ -1126,7 +1126,10 @@ struct CondMutexUnlockCtx {
ScopedInterceptor *si;
ThreadState *thr;
uptr pc;
+ void *c;
void *m;
+ void *abstime;
+ __sanitizer_clockid_t clock;
};
static void cond_mutex_unlock(CondMutexUnlockCtx *arg) {
@@ -1152,19 +1155,18 @@ INTERCEPTOR(int, pthread_cond_init, void *c, void *a) {
}
static int cond_wait(ThreadState *thr, uptr pc, ScopedInterceptor *si,
- int (*fn)(void *c, void *m, void *abstime), void *c,
- void *m, void *t) {
+ int (*fn)(void *arg), void *c,
+ void *m, void *t, __sanitizer_clockid_t clock) {
MemoryAccessRange(thr, pc, (uptr)c, sizeof(uptr), false);
MutexUnlock(thr, pc, (uptr)m);
- CondMutexUnlockCtx arg = {si, thr, pc, m};
+ CondMutexUnlockCtx arg = {si, thr, pc, c, m, t, clock};
int res = 0;
// This ensures that we handle mutex lock even in case of pthread_cancel.
// See test/tsan/cond_cancel.cpp.
{
// Enable signal delivery while the thread is blocked.
BlockingCall bc(thr);
- res = call_pthread_cancel_with_cleanup(
- fn, c, m, t, (void (*)(void *arg))cond_mutex_unlock, &arg);
+ res = call_pthread_cancel_with_cleanup(fn, (void (*)(void
*arg))cond_mutex_unlock, &arg);
}
if (res == errno_EOWNERDEAD) MutexRepair(thr, pc, (uptr)m);
MutexPostLock(thr, pc, (uptr)m, MutexFlagDoPreLockOnPostLock);
@@ -1174,25 +1176,34 @@ static int cond_wait(ThreadState *thr, uptr
pc, ScopedInterceptor *si,
INTERCEPTOR(int, pthread_cond_wait, void *c, void *m) {
void *cond = init_cond(c);
SCOPED_TSAN_INTERCEPTOR(pthread_cond_wait, cond, m);
- return cond_wait(thr, pc, &si, (int (*)(void *c, void *m, void
*abstime))REAL(
- pthread_cond_wait),
- cond, m, 0);
+ return cond_wait(thr, pc, &si, [](void *a) { CondMutexUnlockCtx
*arg = (CondMutexUnlockCtx*)a; return REAL(pthread_cond_wait)(arg->c,
arg->m); },
+ cond, m, 0, 0);
}
INTERCEPTOR(int, pthread_cond_timedwait, void *c, void *m, void *abstime) {
void *cond = init_cond(c);
SCOPED_TSAN_INTERCEPTOR(pthread_cond_timedwait, cond, m, abstime);
- return cond_wait(thr, pc, &si, REAL(pthread_cond_timedwait), cond, m,
- abstime);
+ return cond_wait(thr, pc, &si, [](void *a) { CondMutexUnlockCtx
*arg = (CondMutexUnlockCtx*)a; return
REAL(pthread_cond_timedwait)(arg->c, arg->m, arg->abstime); }, cond,
m,
+ abstime, 0);
}
+#if SANITIZER_LINUX
+INTERCEPTOR(int, pthread_cond_clockwait, void *c, void *m,
__sanitizer_clockid_t clock, void *abstime) {
+ void *cond = init_cond(c);
+ SCOPED_TSAN_INTERCEPTOR(pthread_cond_clockwait, cond, m, clock, abstime);
+ return cond_wait(thr, pc, &si,
+ [](void *a) { CondMutexUnlockCtx *arg =
(CondMutexUnlockCtx*)a; return REAL(pthread_cond_clockwait)(arg->c,
arg->m, arg->clock, arg->abstime); },
+ cond, m, abstime, clock);
+}
+#endif
+
#if SANITIZER_MAC
INTERCEPTOR(int, pthread_cond_timedwait_relative_np, void *c, void *m,
void *reltime) {
void *cond = init_cond(c);
SCOPED_TSAN_INTERCEPTOR(pthread_cond_timedwait_relative_np, cond,
m, reltime);
- return cond_wait(thr, pc, &si,
REAL(pthread_cond_timedwait_relative_np), cond,
- m, reltime);
+ return cond_wait(thr, pc, &si, [](void *a) { CondMutexUnlockCtx
*arg = (CondMutexUnlockCtx*)a; return
REAL(pthread_cond_timedwait_relative_np)(arg->c, arg->m,
arg->abstime); }, cond,
+ m, reltime, 0);
}
#endif
@@ -2697,6 +2708,9 @@ void InitializeInterceptors() {
TSAN_INTERCEPT_VER(pthread_cond_wait, PTHREAD_ABI_BASE);
TSAN_INTERCEPT_VER(pthread_cond_timedwait, PTHREAD_ABI_BASE);
TSAN_INTERCEPT_VER(pthread_cond_destroy, PTHREAD_ABI_BASE);
+#if SANITIZER_LINUX
+ TSAN_INTERCEPT(pthread_cond_clockwait);
+#endif
TSAN_INTERCEPT(pthread_mutex_init);
TSAN_INTERCEPT(pthread_mutex_destroy);
diff --git a/libsanitizer/tsan/tsan_platform.h
b/libsanitizer/tsan/tsan_platform.h
index 16169cab666..d973136f7ae 100644
--- a/libsanitizer/tsan/tsan_platform.h
+++ b/libsanitizer/tsan/tsan_platform.h
@@ -1040,9 +1040,8 @@ int ExtractRecvmsgFDs(void *msg, int *fds, int nfd);
uptr ExtractLongJmpSp(uptr *env);
void ImitateTlsWrite(ThreadState *thr, uptr tls_addr, uptr tls_size);
-int call_pthread_cancel_with_cleanup(int(*fn)(void *c, void *m,
- void *abstime), void *c, void *m, void *abstime,
- void(*cleanup)(void *arg), void *arg);
+int call_pthread_cancel_with_cleanup(int(*fn)(void *arg),
+ void(*cleanup)(void *arg), void *cleanup_arg);
void DestroyThreadState();
void PlatformCleanUpThreadState(ThreadState *thr);
diff --git a/libsanitizer/tsan/tsan_platform_linux.cpp
b/libsanitizer/tsan/tsan_platform_linux.cpp
index d136dcb1cec..d0ac995dfb2 100644
--- a/libsanitizer/tsan/tsan_platform_linux.cpp
+++ b/libsanitizer/tsan/tsan_platform_linux.cpp
@@ -443,14 +443,13 @@ void ImitateTlsWrite(ThreadState *thr, uptr
tls_addr, uptr tls_size) {
// Note: this function runs with async signals enabled,
// so it must not touch any tsan state.
-int call_pthread_cancel_with_cleanup(int(*fn)(void *c, void *m,
- void *abstime), void *c, void *m, void *abstime,
+int call_pthread_cancel_with_cleanup(int(*fn)(void *arg),
void(*cleanup)(void *arg), void *arg) {
// pthread_cleanup_push/pop are hardcore macros mess.
// We can't intercept nor call them w/o including pthread.h.
int res;
pthread_cleanup_push(cleanup, arg);
- res = fn(c, m, abstime);
+ res = fn(arg);
pthread_cleanup_pop(0);
return res;
}
diff --git a/libsanitizer/tsan/tsan_platform_mac.cpp
b/libsanitizer/tsan/tsan_platform_mac.cpp
index ec2c5fb1621..59427b9cb6c 100644
--- a/libsanitizer/tsan/tsan_platform_mac.cpp
+++ b/libsanitizer/tsan/tsan_platform_mac.cpp
@@ -306,14 +306,13 @@ void ImitateTlsWrite(ThreadState *thr, uptr
tls_addr, uptr tls_size) {
#if !SANITIZER_GO
// Note: this function runs with async signals enabled,
// so it must not touch any tsan state.
-int call_pthread_cancel_with_cleanup(int(*fn)(void *c, void *m,
- void *abstime), void *c, void *m, void *abstime,
+int call_pthread_cancel_with_cleanup(int(*fn)(void *arg),
void(*cleanup)(void *arg), void *arg) {
// pthread_cleanup_push/pop are hardcore macros mess.
// We can't intercept nor call them w/o including pthread.h.
int res;
pthread_cleanup_push(cleanup, arg);
- res = fn(c, m, abstime);
+ res = fn(arg);
pthread_cleanup_pop(0);
return res;
}
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] tsan: fix false positive for pthread_cond_clockwait
2021-05-07 17:07 [PATCH] tsan: fix false positive for pthread_cond_clockwait Michael de Lang
@ 2021-05-13 7:33 ` Martin Liška
2021-05-13 17:10 ` Michael de Lang
0 siblings, 1 reply; 4+ messages in thread
From: Martin Liška @ 2021-05-13 7:33 UTC (permalink / raw)
To: Michael de Lang, gcc-patches
On 5/7/21 7:07 PM, Michael de Lang via Gcc-patches wrote:
> pthread_cond_clockwait isn't added to TSAN_INTERCEPTORS which leads to
> false positives regarding double locking of a mutex. This was
> uncovered by a user reporting an issue to the google sanitizer github:
> https://github.com/google/sanitizers/issues/1259
>
> This patch copies code from the fix made in llvm:
> https://github.com/llvm/llvm-project/commit/16eb853ffdd1a1ad7c95455b7795c5f004402e46
Hello.
Thank you for looking into this.
>
> However, because the tsan related source code hasn't been kept in sync
> with llvm, I had to make some modifications.
We merge from master rougtly twice a year. I've just merged LLVM upstream to our master.
>
> Given that this is my first contibution to gcc, let me know if I've
> missed anything.
Please take a look at the following steps:
https://gcc.gnu.org/contribute.html
We still want your test-case, can you please resend the patch on the current master?
Thanks!
Cheers,
Martin
>
> Met vriendelijke groet,
> Michael de Lang
>
> +++ b/gcc/testsuite/g++.dg/tsan/pthread_cond_clockwait.C
> @@ -0,0 +1,31 @@
> +// Test pthread_cond_clockwait not generating false positives with tsan
> +// { dg-do run { target { { *-*-linux* *-*-gnu* *-*-uclinux* } && pthread } } }
> +// { dg-options "-fsanitize=thread -lpthread" }
> +
> +#include <pthread.h>
> +
> +pthread_cond_t cv;
> +pthread_mutex_t mtx;
> +
> +void *fn(void *vp) {
> + pthread_mutex_lock(&mtx);
> + pthread_cond_signal(&cv);
> + pthread_mutex_unlock(&mtx);
> + return NULL;
> +}
> +
> +int main() {
> + pthread_mutex_lock(&mtx);
> +
> + pthread_t tid;
> + pthread_create(&tid, NULL, fn, NULL);
> +
> + struct timespec ts;
> + clock_gettime(CLOCK_MONOTONIC, &ts);
> + ts.tv_sec += 10;
> + pthread_cond_clockwait(&cv, &mtx, CLOCK_MONOTONIC, &ts);
> + pthread_mutex_unlock(&mtx);
> +
> + pthread_join(tid, NULL);
> + return 0;
> +}
> diff --git a/libsanitizer/tsan/tsan_interceptors_posix.cpp
> b/libsanitizer/tsan/tsan_interceptors_posix.cpp
> index aa04d8dfb67..7b3d0a917de 100644
> --- a/libsanitizer/tsan/tsan_interceptors_posix.cpp
> +++ b/libsanitizer/tsan/tsan_interceptors_posix.cpp
> @@ -1126,7 +1126,10 @@ struct CondMutexUnlockCtx {
> ScopedInterceptor *si;
> ThreadState *thr;
> uptr pc;
> + void *c;
> void *m;
> + void *abstime;
> + __sanitizer_clockid_t clock;
> };
>
> static void cond_mutex_unlock(CondMutexUnlockCtx *arg) {
> @@ -1152,19 +1155,18 @@ INTERCEPTOR(int, pthread_cond_init, void *c, void *a) {
> }
>
> static int cond_wait(ThreadState *thr, uptr pc, ScopedInterceptor *si,
> - int (*fn)(void *c, void *m, void *abstime), void *c,
> - void *m, void *t) {
> + int (*fn)(void *arg), void *c,
> + void *m, void *t, __sanitizer_clockid_t clock) {
> MemoryAccessRange(thr, pc, (uptr)c, sizeof(uptr), false);
> MutexUnlock(thr, pc, (uptr)m);
> - CondMutexUnlockCtx arg = {si, thr, pc, m};
> + CondMutexUnlockCtx arg = {si, thr, pc, c, m, t, clock};
> int res = 0;
> // This ensures that we handle mutex lock even in case of pthread_cancel.
> // See test/tsan/cond_cancel.cpp.
> {
> // Enable signal delivery while the thread is blocked.
> BlockingCall bc(thr);
> - res = call_pthread_cancel_with_cleanup(
> - fn, c, m, t, (void (*)(void *arg))cond_mutex_unlock, &arg);
> + res = call_pthread_cancel_with_cleanup(fn, (void (*)(void
> *arg))cond_mutex_unlock, &arg);
> }
> if (res == errno_EOWNERDEAD) MutexRepair(thr, pc, (uptr)m);
> MutexPostLock(thr, pc, (uptr)m, MutexFlagDoPreLockOnPostLock);
> @@ -1174,25 +1176,34 @@ static int cond_wait(ThreadState *thr, uptr
> pc, ScopedInterceptor *si,
> INTERCEPTOR(int, pthread_cond_wait, void *c, void *m) {
> void *cond = init_cond(c);
> SCOPED_TSAN_INTERCEPTOR(pthread_cond_wait, cond, m);
> - return cond_wait(thr, pc, &si, (int (*)(void *c, void *m, void
> *abstime))REAL(
> - pthread_cond_wait),
> - cond, m, 0);
> + return cond_wait(thr, pc, &si, [](void *a) { CondMutexUnlockCtx
> *arg = (CondMutexUnlockCtx*)a; return REAL(pthread_cond_wait)(arg->c,
> arg->m); },
> + cond, m, 0, 0);
> }
>
> INTERCEPTOR(int, pthread_cond_timedwait, void *c, void *m, void *abstime) {
> void *cond = init_cond(c);
> SCOPED_TSAN_INTERCEPTOR(pthread_cond_timedwait, cond, m, abstime);
> - return cond_wait(thr, pc, &si, REAL(pthread_cond_timedwait), cond, m,
> - abstime);
> + return cond_wait(thr, pc, &si, [](void *a) { CondMutexUnlockCtx
> *arg = (CondMutexUnlockCtx*)a; return
> REAL(pthread_cond_timedwait)(arg->c, arg->m, arg->abstime); }, cond,
> m,
> + abstime, 0);
> }
>
> +#if SANITIZER_LINUX
> +INTERCEPTOR(int, pthread_cond_clockwait, void *c, void *m,
> __sanitizer_clockid_t clock, void *abstime) {
> + void *cond = init_cond(c);
> + SCOPED_TSAN_INTERCEPTOR(pthread_cond_clockwait, cond, m, clock, abstime);
> + return cond_wait(thr, pc, &si,
> + [](void *a) { CondMutexUnlockCtx *arg =
> (CondMutexUnlockCtx*)a; return REAL(pthread_cond_clockwait)(arg->c,
> arg->m, arg->clock, arg->abstime); },
> + cond, m, abstime, clock);
> +}
> +#endif
> +
> #if SANITIZER_MAC
> INTERCEPTOR(int, pthread_cond_timedwait_relative_np, void *c, void *m,
> void *reltime) {
> void *cond = init_cond(c);
> SCOPED_TSAN_INTERCEPTOR(pthread_cond_timedwait_relative_np, cond,
> m, reltime);
> - return cond_wait(thr, pc, &si,
> REAL(pthread_cond_timedwait_relative_np), cond,
> - m, reltime);
> + return cond_wait(thr, pc, &si, [](void *a) { CondMutexUnlockCtx
> *arg = (CondMutexUnlockCtx*)a; return
> REAL(pthread_cond_timedwait_relative_np)(arg->c, arg->m,
> arg->abstime); }, cond,
> + m, reltime, 0);
> }
> #endif
>
> @@ -2697,6 +2708,9 @@ void InitializeInterceptors() {
> TSAN_INTERCEPT_VER(pthread_cond_wait, PTHREAD_ABI_BASE);
> TSAN_INTERCEPT_VER(pthread_cond_timedwait, PTHREAD_ABI_BASE);
> TSAN_INTERCEPT_VER(pthread_cond_destroy, PTHREAD_ABI_BASE);
> +#if SANITIZER_LINUX
> + TSAN_INTERCEPT(pthread_cond_clockwait);
> +#endif
>
> TSAN_INTERCEPT(pthread_mutex_init);
> TSAN_INTERCEPT(pthread_mutex_destroy);
> diff --git a/libsanitizer/tsan/tsan_platform.h
> b/libsanitizer/tsan/tsan_platform.h
> index 16169cab666..d973136f7ae 100644
> --- a/libsanitizer/tsan/tsan_platform.h
> +++ b/libsanitizer/tsan/tsan_platform.h
> @@ -1040,9 +1040,8 @@ int ExtractRecvmsgFDs(void *msg, int *fds, int nfd);
> uptr ExtractLongJmpSp(uptr *env);
> void ImitateTlsWrite(ThreadState *thr, uptr tls_addr, uptr tls_size);
>
> -int call_pthread_cancel_with_cleanup(int(*fn)(void *c, void *m,
> - void *abstime), void *c, void *m, void *abstime,
> - void(*cleanup)(void *arg), void *arg);
> +int call_pthread_cancel_with_cleanup(int(*fn)(void *arg),
> + void(*cleanup)(void *arg), void *cleanup_arg);
>
> void DestroyThreadState();
> void PlatformCleanUpThreadState(ThreadState *thr);
> diff --git a/libsanitizer/tsan/tsan_platform_linux.cpp
> b/libsanitizer/tsan/tsan_platform_linux.cpp
> index d136dcb1cec..d0ac995dfb2 100644
> --- a/libsanitizer/tsan/tsan_platform_linux.cpp
> +++ b/libsanitizer/tsan/tsan_platform_linux.cpp
> @@ -443,14 +443,13 @@ void ImitateTlsWrite(ThreadState *thr, uptr
> tls_addr, uptr tls_size) {
>
> // Note: this function runs with async signals enabled,
> // so it must not touch any tsan state.
> -int call_pthread_cancel_with_cleanup(int(*fn)(void *c, void *m,
> - void *abstime), void *c, void *m, void *abstime,
> +int call_pthread_cancel_with_cleanup(int(*fn)(void *arg),
> void(*cleanup)(void *arg), void *arg) {
> // pthread_cleanup_push/pop are hardcore macros mess.
> // We can't intercept nor call them w/o including pthread.h.
> int res;
> pthread_cleanup_push(cleanup, arg);
> - res = fn(c, m, abstime);
> + res = fn(arg);
> pthread_cleanup_pop(0);
> return res;
> }
> diff --git a/libsanitizer/tsan/tsan_platform_mac.cpp
> b/libsanitizer/tsan/tsan_platform_mac.cpp
> index ec2c5fb1621..59427b9cb6c 100644
> --- a/libsanitizer/tsan/tsan_platform_mac.cpp
> +++ b/libsanitizer/tsan/tsan_platform_mac.cpp
> @@ -306,14 +306,13 @@ void ImitateTlsWrite(ThreadState *thr, uptr
> tls_addr, uptr tls_size) {
> #if !SANITIZER_GO
> // Note: this function runs with async signals enabled,
> // so it must not touch any tsan state.
> -int call_pthread_cancel_with_cleanup(int(*fn)(void *c, void *m,
> - void *abstime), void *c, void *m, void *abstime,
> +int call_pthread_cancel_with_cleanup(int(*fn)(void *arg),
> void(*cleanup)(void *arg), void *arg) {
> // pthread_cleanup_push/pop are hardcore macros mess.
> // We can't intercept nor call them w/o including pthread.h.
> int res;
> pthread_cleanup_push(cleanup, arg);
> - res = fn(c, m, abstime);
> + res = fn(arg);
> pthread_cleanup_pop(0);
> return res;
> }
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] tsan: fix false positive for pthread_cond_clockwait
2021-05-13 7:33 ` Martin Liška
@ 2021-05-13 17:10 ` Michael de Lang
2021-05-14 10:10 ` Martin Liška
0 siblings, 1 reply; 4+ messages in thread
From: Michael de Lang @ 2021-05-13 17:10 UTC (permalink / raw)
To: Martin Liška; +Cc: gcc-patches
Thanks for updating LLVM to upstream. I've added the rebased patch below.
Met vriendelijke groet,
Michael de Lang
gcc/ChangeLog
* g++.dg/tsan/pthread_cond_clockwait.C: new testcase
diff --git a/gcc/testsuite/g++.dg/tsan/pthread_cond_clockwait.C
b/gcc/testsuite/g++.dg/tsan/pthread_cond_clockwait.C
new file mode 100644
index 00000000000..82d6a5c8329
--- /dev/null
+++ b/gcc/testsuite/g++.dg/tsan/pthread_cond_clockwait.C
@@ -0,0 +1,31 @@
+// Test pthread_cond_clockwait not generating false positives with tsan
+// { dg-do run { target { { *-*-linux* *-*-gnu* *-*-uclinux* } && pthread } } }
+// { dg-options "-fsanitize=thread -lpthread" }
+
+#include <pthread.h>
+
+pthread_cond_t cv;
+pthread_mutex_t mtx;
+
+void *fn(void *vp) {
+ pthread_mutex_lock(&mtx);
+ pthread_cond_signal(&cv);
+ pthread_mutex_unlock(&mtx);
+ return NULL;
+}
+
+int main() {
+ pthread_mutex_lock(&mtx);
+
+ pthread_t tid;
+ pthread_create(&tid, NULL, fn, NULL);
+
+ struct timespec ts;
+ clock_gettime(CLOCK_MONOTONIC, &ts);
+ ts.tv_sec += 10;
+ pthread_cond_clockwait(&cv, &mtx, CLOCK_MONOTONIC, &ts);
+ pthread_mutex_unlock(&mtx);
+
+ pthread_join(tid, NULL);
+ return 0;
+}
On Thu, 13 May 2021 at 09:33, Martin Liška <mliska@suse.cz> wrote:
>
> On 5/7/21 7:07 PM, Michael de Lang via Gcc-patches wrote:
> > pthread_cond_clockwait isn't added to TSAN_INTERCEPTORS which leads to
> > false positives regarding double locking of a mutex. This was
> > uncovered by a user reporting an issue to the google sanitizer github:
> > https://github.com/google/sanitizers/issues/1259
> >
> > This patch copies code from the fix made in llvm:
> > https://github.com/llvm/llvm-project/commit/16eb853ffdd1a1ad7c95455b7795c5f004402e46
>
> Hello.
>
> Thank you for looking into this.
>
> >
> > However, because the tsan related source code hasn't been kept in sync
> > with llvm, I had to make some modifications.
>
> We merge from master rougtly twice a year. I've just merged LLVM upstream to our master.
>
> >
> > Given that this is my first contibution to gcc, let me know if I've
> > missed anything.
>
> Please take a look at the following steps:
> https://gcc.gnu.org/contribute.html
>
> We still want your test-case, can you please resend the patch on the current master?
>
> Thanks!
> Cheers,
> Martin
>
> >
> > Met vriendelijke groet,
> > Michael de Lang
> >
> > +++ b/gcc/testsuite/g++.dg/tsan/pthread_cond_clockwait.C
> > @@ -0,0 +1,31 @@
> > +// Test pthread_cond_clockwait not generating false positives with tsan
> > +// { dg-do run { target { { *-*-linux* *-*-gnu* *-*-uclinux* } && pthread } } }
> > +// { dg-options "-fsanitize=thread -lpthread" }
> > +
> > +#include <pthread.h>
> > +
> > +pthread_cond_t cv;
> > +pthread_mutex_t mtx;
> > +
> > +void *fn(void *vp) {
> > + pthread_mutex_lock(&mtx);
> > + pthread_cond_signal(&cv);
> > + pthread_mutex_unlock(&mtx);
> > + return NULL;
> > +}
> > +
> > +int main() {
> > + pthread_mutex_lock(&mtx);
> > +
> > + pthread_t tid;
> > + pthread_create(&tid, NULL, fn, NULL);
> > +
> > + struct timespec ts;
> > + clock_gettime(CLOCK_MONOTONIC, &ts);
> > + ts.tv_sec += 10;
> > + pthread_cond_clockwait(&cv, &mtx, CLOCK_MONOTONIC, &ts);
> > + pthread_mutex_unlock(&mtx);
> > +
> > + pthread_join(tid, NULL);
> > + return 0;
> > +}
> > diff --git a/libsanitizer/tsan/tsan_interceptors_posix.cpp
> > b/libsanitizer/tsan/tsan_interceptors_posix.cpp
> > index aa04d8dfb67..7b3d0a917de 100644
> > --- a/libsanitizer/tsan/tsan_interceptors_posix.cpp
> > +++ b/libsanitizer/tsan/tsan_interceptors_posix.cpp
> > @@ -1126,7 +1126,10 @@ struct CondMutexUnlockCtx {
> > ScopedInterceptor *si;
> > ThreadState *thr;
> > uptr pc;
> > + void *c;
> > void *m;
> > + void *abstime;
> > + __sanitizer_clockid_t clock;
> > };
> >
> > static void cond_mutex_unlock(CondMutexUnlockCtx *arg) {
> > @@ -1152,19 +1155,18 @@ INTERCEPTOR(int, pthread_cond_init, void *c, void *a) {
> > }
> >
> > static int cond_wait(ThreadState *thr, uptr pc, ScopedInterceptor *si,
> > - int (*fn)(void *c, void *m, void *abstime), void *c,
> > - void *m, void *t) {
> > + int (*fn)(void *arg), void *c,
> > + void *m, void *t, __sanitizer_clockid_t clock) {
> > MemoryAccessRange(thr, pc, (uptr)c, sizeof(uptr), false);
> > MutexUnlock(thr, pc, (uptr)m);
> > - CondMutexUnlockCtx arg = {si, thr, pc, m};
> > + CondMutexUnlockCtx arg = {si, thr, pc, c, m, t, clock};
> > int res = 0;
> > // This ensures that we handle mutex lock even in case of pthread_cancel.
> > // See test/tsan/cond_cancel.cpp.
> > {
> > // Enable signal delivery while the thread is blocked.
> > BlockingCall bc(thr);
> > - res = call_pthread_cancel_with_cleanup(
> > - fn, c, m, t, (void (*)(void *arg))cond_mutex_unlock, &arg);
> > + res = call_pthread_cancel_with_cleanup(fn, (void (*)(void
> > *arg))cond_mutex_unlock, &arg);
> > }
> > if (res == errno_EOWNERDEAD) MutexRepair(thr, pc, (uptr)m);
> > MutexPostLock(thr, pc, (uptr)m, MutexFlagDoPreLockOnPostLock);
> > @@ -1174,25 +1176,34 @@ static int cond_wait(ThreadState *thr, uptr
> > pc, ScopedInterceptor *si,
> > INTERCEPTOR(int, pthread_cond_wait, void *c, void *m) {
> > void *cond = init_cond(c);
> > SCOPED_TSAN_INTERCEPTOR(pthread_cond_wait, cond, m);
> > - return cond_wait(thr, pc, &si, (int (*)(void *c, void *m, void
> > *abstime))REAL(
> > - pthread_cond_wait),
> > - cond, m, 0);
> > + return cond_wait(thr, pc, &si, [](void *a) { CondMutexUnlockCtx
> > *arg = (CondMutexUnlockCtx*)a; return REAL(pthread_cond_wait)(arg->c,
> > arg->m); },
> > + cond, m, 0, 0);
> > }
> >
> > INTERCEPTOR(int, pthread_cond_timedwait, void *c, void *m, void *abstime) {
> > void *cond = init_cond(c);
> > SCOPED_TSAN_INTERCEPTOR(pthread_cond_timedwait, cond, m, abstime);
> > - return cond_wait(thr, pc, &si, REAL(pthread_cond_timedwait), cond, m,
> > - abstime);
> > + return cond_wait(thr, pc, &si, [](void *a) { CondMutexUnlockCtx
> > *arg = (CondMutexUnlockCtx*)a; return
> > REAL(pthread_cond_timedwait)(arg->c, arg->m, arg->abstime); }, cond,
> > m,
> > + abstime, 0);
> > }
> >
> > +#if SANITIZER_LINUX
> > +INTERCEPTOR(int, pthread_cond_clockwait, void *c, void *m,
> > __sanitizer_clockid_t clock, void *abstime) {
> > + void *cond = init_cond(c);
> > + SCOPED_TSAN_INTERCEPTOR(pthread_cond_clockwait, cond, m, clock, abstime);
> > + return cond_wait(thr, pc, &si,
> > + [](void *a) { CondMutexUnlockCtx *arg =
> > (CondMutexUnlockCtx*)a; return REAL(pthread_cond_clockwait)(arg->c,
> > arg->m, arg->clock, arg->abstime); },
> > + cond, m, abstime, clock);
> > +}
> > +#endif
> > +
> > #if SANITIZER_MAC
> > INTERCEPTOR(int, pthread_cond_timedwait_relative_np, void *c, void *m,
> > void *reltime) {
> > void *cond = init_cond(c);
> > SCOPED_TSAN_INTERCEPTOR(pthread_cond_timedwait_relative_np, cond,
> > m, reltime);
> > - return cond_wait(thr, pc, &si,
> > REAL(pthread_cond_timedwait_relative_np), cond,
> > - m, reltime);
> > + return cond_wait(thr, pc, &si, [](void *a) { CondMutexUnlockCtx
> > *arg = (CondMutexUnlockCtx*)a; return
> > REAL(pthread_cond_timedwait_relative_np)(arg->c, arg->m,
> > arg->abstime); }, cond,
> > + m, reltime, 0);
> > }
> > #endif
> >
> > @@ -2697,6 +2708,9 @@ void InitializeInterceptors() {
> > TSAN_INTERCEPT_VER(pthread_cond_wait, PTHREAD_ABI_BASE);
> > TSAN_INTERCEPT_VER(pthread_cond_timedwait, PTHREAD_ABI_BASE);
> > TSAN_INTERCEPT_VER(pthread_cond_destroy, PTHREAD_ABI_BASE);
> > +#if SANITIZER_LINUX
> > + TSAN_INTERCEPT(pthread_cond_clockwait);
> > +#endif
> >
> > TSAN_INTERCEPT(pthread_mutex_init);
> > TSAN_INTERCEPT(pthread_mutex_destroy);
> > diff --git a/libsanitizer/tsan/tsan_platform.h
> > b/libsanitizer/tsan/tsan_platform.h
> > index 16169cab666..d973136f7ae 100644
> > --- a/libsanitizer/tsan/tsan_platform.h
> > +++ b/libsanitizer/tsan/tsan_platform.h
> > @@ -1040,9 +1040,8 @@ int ExtractRecvmsgFDs(void *msg, int *fds, int nfd);
> > uptr ExtractLongJmpSp(uptr *env);
> > void ImitateTlsWrite(ThreadState *thr, uptr tls_addr, uptr tls_size);
> >
> > -int call_pthread_cancel_with_cleanup(int(*fn)(void *c, void *m,
> > - void *abstime), void *c, void *m, void *abstime,
> > - void(*cleanup)(void *arg), void *arg);
> > +int call_pthread_cancel_with_cleanup(int(*fn)(void *arg),
> > + void(*cleanup)(void *arg), void *cleanup_arg);
> >
> > void DestroyThreadState();
> > void PlatformCleanUpThreadState(ThreadState *thr);
> > diff --git a/libsanitizer/tsan/tsan_platform_linux.cpp
> > b/libsanitizer/tsan/tsan_platform_linux.cpp
> > index d136dcb1cec..d0ac995dfb2 100644
> > --- a/libsanitizer/tsan/tsan_platform_linux.cpp
> > +++ b/libsanitizer/tsan/tsan_platform_linux.cpp
> > @@ -443,14 +443,13 @@ void ImitateTlsWrite(ThreadState *thr, uptr
> > tls_addr, uptr tls_size) {
> >
> > // Note: this function runs with async signals enabled,
> > // so it must not touch any tsan state.
> > -int call_pthread_cancel_with_cleanup(int(*fn)(void *c, void *m,
> > - void *abstime), void *c, void *m, void *abstime,
> > +int call_pthread_cancel_with_cleanup(int(*fn)(void *arg),
> > void(*cleanup)(void *arg), void *arg) {
> > // pthread_cleanup_push/pop are hardcore macros mess.
> > // We can't intercept nor call them w/o including pthread.h.
> > int res;
> > pthread_cleanup_push(cleanup, arg);
> > - res = fn(c, m, abstime);
> > + res = fn(arg);
> > pthread_cleanup_pop(0);
> > return res;
> > }
> > diff --git a/libsanitizer/tsan/tsan_platform_mac.cpp
> > b/libsanitizer/tsan/tsan_platform_mac.cpp
> > index ec2c5fb1621..59427b9cb6c 100644
> > --- a/libsanitizer/tsan/tsan_platform_mac.cpp
> > +++ b/libsanitizer/tsan/tsan_platform_mac.cpp
> > @@ -306,14 +306,13 @@ void ImitateTlsWrite(ThreadState *thr, uptr
> > tls_addr, uptr tls_size) {
> > #if !SANITIZER_GO
> > // Note: this function runs with async signals enabled,
> > // so it must not touch any tsan state.
> > -int call_pthread_cancel_with_cleanup(int(*fn)(void *c, void *m,
> > - void *abstime), void *c, void *m, void *abstime,
> > +int call_pthread_cancel_with_cleanup(int(*fn)(void *arg),
> > void(*cleanup)(void *arg), void *arg) {
> > // pthread_cleanup_push/pop are hardcore macros mess.
> > // We can't intercept nor call them w/o including pthread.h.
> > int res;
> > pthread_cleanup_push(cleanup, arg);
> > - res = fn(c, m, abstime);
> > + res = fn(arg);
> > pthread_cleanup_pop(0);
> > return res;
> > }
> >
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] tsan: fix false positive for pthread_cond_clockwait
2021-05-13 17:10 ` Michael de Lang
@ 2021-05-14 10:10 ` Martin Liška
0 siblings, 0 replies; 4+ messages in thread
From: Martin Liška @ 2021-05-14 10:10 UTC (permalink / raw)
To: Michael de Lang; +Cc: gcc-patches
On 5/13/21 7:10 PM, Michael de Lang wrote:
> |Thanks for updating LLVM to upstream. I've added the rebased patch below.|
Thanks, I've just tested and pushed your commit:
https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=80b4ce1a5190ebe764b1009afae57dcef45f92c2
Martin
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-05-14 10:10 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-07 17:07 [PATCH] tsan: fix false positive for pthread_cond_clockwait Michael de Lang
2021-05-13 7:33 ` Martin Liška
2021-05-13 17:10 ` Michael de Lang
2021-05-14 10:10 ` Martin Liška
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).