Re: [PATCH] tsan: fix false positive for pthread_cond_clockwait

[Martin Liška]

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;
  }