From patchwork Fri May 7 17:07:56 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Michael de Lang X-Patchwork-Id: 1475607 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=gcc.gnu.org (client-ip=8.43.85.97; helo=sourceware.org; envelope-from=gcc-patches-bounces@gcc.gnu.org; receiver=) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.a=rsa-sha256 header.s=default header.b=IVpeREEv; dkim-atps=neutral Received: from sourceware.org (ip-8-43-85-97.sourceware.org [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 4FcH2l0N0xz9sXS for ; Sat, 8 May 2021 03:08:14 +1000 (AEST) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 74F95385381C; Fri, 7 May 2021 17:08:11 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 74F95385381C DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1620407291; bh=h9XDeBPSinLv7CvjTIDa+7oZ+k5gWSc06GY4+WsBn9o=; h=Date:Subject:To:List-Id:List-Unsubscribe:List-Archive:List-Post: List-Help:List-Subscribe:From:Reply-To:From; b=IVpeREEvUKzDnLphoL4+ZnZ9vH3X15bHOFpTGbwoAGbkVymFOV4sqaPUA1w7pvoyL ayojhIiamCnAoBedx2yM5E2wJgqPkKpKN9oRmFnAny7t5pqHZYhXtoHJYOz9yGkTG2 0SzZqaOSFkboANY2S1/o8dGcQa7g255/MLrxE7VU= X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from mail-ed1-x52f.google.com (mail-ed1-x52f.google.com [IPv6:2a00:1450:4864:20::52f]) by sourceware.org (Postfix) with ESMTPS id 035BC3857C52 for ; Fri, 7 May 2021 17:08:08 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 035BC3857C52 Received: by mail-ed1-x52f.google.com with SMTP id b17so11081454ede.0 for ; Fri, 07 May 2021 10:08:07 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:from:date:message-id:subject:to; bh=h9XDeBPSinLv7CvjTIDa+7oZ+k5gWSc06GY4+WsBn9o=; b=dD0a0uIvw2zt6yi99jgAoJ8Rab9hEzjSYDyfTfrDr9lF1/2rCZpOhmnzwSdSAbNh52 /edHyOeWwtsa52guMgYCtrlFPKBrBapK9LcQ9o4APu8Ueeli5tkS4YQR0NK/c3v9zJwc yWUbWbwBYUQxIEQXTu6JaQwCtPXclglOuX9G31bHtnrbDT99QCqswKejv75kjPkswQyG /E24JMX4JcZS94LX2IQdRTOGlo3t8oN5A8bYg7C4gpiACt8hV4D7YIiJrdL7TkVdOMtU SXdHZlEpjvShM5XBye9JfG8pP+u+CttTFPgpGNnNA5jEZam2ShNInmJgufCq8UJX3SpF LDmA== X-Gm-Message-State: AOAM533xFgAYdy3Ph5cFj3xaDw3cDY87+9wNF2MBfiogujTi6LV/DL0g kj3wc2T/SGNuJl6wwGQ8XEa5WpNZHsNY1B4XtGLhH3NI X-Google-Smtp-Source: ABdhPJxpZYox6TDVM8v+robPOW54pKwv5U/Q70N7jSzNyDmtbxO7L/OEOqnWY/QCCCvwJZP2d8MkXEN+EsUIpNNz4UA= X-Received: by 2002:a50:ed0c:: with SMTP id j12mr12560060eds.12.1620407286748; Fri, 07 May 2021 10:08:06 -0700 (PDT) MIME-Version: 1.0 Date: Fri, 7 May 2021 19:07:56 +0200 Message-ID: Subject: [PATCH] tsan: fix false positive for pthread_cond_clockwait To: gcc-patches@gcc.gnu.org X-Spam-Status: No, score=-11.3 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-Patchwork-Original-From: Michael de Lang via Gcc-patches From: Michael de Lang Reply-To: Michael de Lang Errors-To: gcc-patches-bounces@gcc.gnu.org Sender: "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_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;