From 3f043e13d5abcf55472fd757d0cf45b4167b40df Mon Sep 17 00:00:00 2001 From: Thurston Dang Date: Mon, 11 Nov 2024 23:49:48 +0000 Subject: [PATCH 1/7] Reapply "[sanitizer_common] AND signals in BlockSignals instead of deleting (#113443)" for non-Android Linux only The original patch (25fd366d6a7d40266ff27c134ed8beb0a90cc33b) was reverted in 083a5cdbeab09517d8345868970d4f41170d7ed2 because it broke some buildbots. This revised patch makes two changes: - No-op the change for Android: this had mysterious (but reproducible) test failures. - Only define KeepUnblocked if SANITIZER_LINUX: this avoids a build breakage on solaris, which does not support internal_sigismember. - Two other buildbot failures were non-sanitizer tests and are therefore unrelated. Original commit message: My earlier patch https://github.com/llvm/llvm-project/pull/98200 caused a regression because it unconditionally unblocked synchronous signals, even if the user program had deliberately blocked them. This patch fixes the issue by checking the current signal mask, as suggested by Vitaly. It also adds tests. Fixes #113385 --- .../lib/sanitizer_common/sanitizer_linux.cpp | 46 ++++++++--- .../lib/sanitizer_common/tests/CMakeLists.txt | 1 + .../tests/sanitizer_block_signals.cpp | 76 +++++++++++++++++++ 3 files changed, 111 insertions(+), 12 deletions(-) create mode 100644 compiler-rt/lib/sanitizer_common/tests/sanitizer_block_signals.cpp diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp b/compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp index 8b1850f85010cf..dac5683a090217 100644 --- a/compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp +++ b/compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp @@ -164,33 +164,55 @@ void SetSigProcMask(__sanitizer_sigset_t *set, __sanitizer_sigset_t *oldset) { CHECK_EQ(0, internal_sigprocmask(SIG_SETMASK, set, oldset)); } +# if SANITIZER_LINUX +// Deletes the specified signal from newset, if it is not present in oldset +// Equivalently: newset[signum] = newset[signum] & oldset[signum] +static void KeepUnblocked(__sanitizer_sigset_t &newset, + __sanitizer_sigset_t &oldset, int signum) { + // FIXME: Figure out why Android tests fail with internal_sigismember + // See also: comment in BlockSignals(). + // In the meantime, we prefer to sometimes incorrectly unblock signals. + if (SANITIZER_ANDROID || !internal_sigismember(&oldset, signum)) + internal_sigdelset(&newset, signum); +} +# endif + // Block asynchronous signals void BlockSignals(__sanitizer_sigset_t *oldset) { - __sanitizer_sigset_t set; - internal_sigfillset(&set); + __sanitizer_sigset_t currentset; +# if !SANITIZER_ANDROID + // FIXME: SetSigProcMask and pthread_sigmask cause mysterious failures + // See also: comment in KeepUnblocked(). + // In the meantime, we prefer to sometimes incorrectly unblock signals. + SetSigProcMask(NULL, ¤tset); +# endif + + __sanitizer_sigset_t newset; + internal_sigfillset(&newset); # if SANITIZER_LINUX && !SANITIZER_ANDROID // Glibc uses SIGSETXID signal during setuid call. If this signal is blocked // on any thread, setuid call hangs. // See test/sanitizer_common/TestCases/Linux/setuid.c. - internal_sigdelset(&set, 33); + KeepUnblocked(newset, currentset, 33); # endif # if SANITIZER_LINUX // Seccomp-BPF-sandboxed processes rely on SIGSYS to handle trapped syscalls. // If this signal is blocked, such calls cannot be handled and the process may // hang. - internal_sigdelset(&set, 31); + KeepUnblocked(newset, currentset, 31); // Don't block synchronous signals - internal_sigdelset(&set, SIGSEGV); - internal_sigdelset(&set, SIGBUS); - internal_sigdelset(&set, SIGILL); - internal_sigdelset(&set, SIGTRAP); - internal_sigdelset(&set, SIGABRT); - internal_sigdelset(&set, SIGFPE); - internal_sigdelset(&set, SIGPIPE); + // but also don't unblock signals that the user had deliberately blocked. + KeepUnblocked(newset, currentset, SIGSEGV); + KeepUnblocked(newset, currentset, SIGBUS); + KeepUnblocked(newset, currentset, SIGILL); + KeepUnblocked(newset, currentset, SIGTRAP); + KeepUnblocked(newset, currentset, SIGABRT); + KeepUnblocked(newset, currentset, SIGFPE); + KeepUnblocked(newset, currentset, SIGPIPE); # endif - SetSigProcMask(&set, oldset); + SetSigProcMask(&newset, oldset); } ScopedBlockSignals::ScopedBlockSignals(__sanitizer_sigset_t *copy) { diff --git a/compiler-rt/lib/sanitizer_common/tests/CMakeLists.txt b/compiler-rt/lib/sanitizer_common/tests/CMakeLists.txt index 2b4c15125263a9..fef8bb772e0e0d 100644 --- a/compiler-rt/lib/sanitizer_common/tests/CMakeLists.txt +++ b/compiler-rt/lib/sanitizer_common/tests/CMakeLists.txt @@ -15,6 +15,7 @@ set(SANITIZER_UNITTESTS sanitizer_array_ref_test.cpp sanitizer_atomic_test.cpp sanitizer_bitvector_test.cpp + sanitizer_block_signals.cpp sanitizer_bvgraph_test.cpp sanitizer_chained_origin_depot_test.cpp sanitizer_common_test.cpp diff --git a/compiler-rt/lib/sanitizer_common/tests/sanitizer_block_signals.cpp b/compiler-rt/lib/sanitizer_common/tests/sanitizer_block_signals.cpp new file mode 100644 index 00000000000000..b43648a8aef230 --- /dev/null +++ b/compiler-rt/lib/sanitizer_common/tests/sanitizer_block_signals.cpp @@ -0,0 +1,76 @@ +//===-- sanitizer_block_signals.cpp ---------------------------------------===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// +// +// This file is a part of sanitizer_common unit tests. +// +//===----------------------------------------------------------------------===// +#include +#include + +#include "gtest/gtest.h" +#include "sanitizer_common/sanitizer_linux.h" + +namespace __sanitizer { + +#if SANITIZER_LINUX && !SANITIZER_ANDROID +volatile int received_sig = -1; + +void signal_handler(int signum) { received_sig = signum; } + +TEST(SanitizerCommon, NoBlockSignals) { + // No signals blocked + signal(SIGUSR1, signal_handler); + raise(SIGUSR1); + EXPECT_EQ(received_sig, SIGUSR1); + + received_sig = -1; + signal(SIGPIPE, signal_handler); + raise(SIGPIPE); + EXPECT_EQ(received_sig, SIGPIPE); +} + +TEST(SanitizerCommon, BlockSignalsPlain) { + // ScopedBlockSignals; SIGUSR1 should be blocked but not SIGPIPE + { + __sanitizer_sigset_t sigset = {}; + ScopedBlockSignals block(&sigset); + + received_sig = -1; + signal(SIGUSR1, signal_handler); + raise(SIGUSR1); + EXPECT_EQ(received_sig, -1); + + received_sig = -1; + signal(SIGPIPE, signal_handler); + raise(SIGPIPE); + EXPECT_EQ(received_sig, SIGPIPE); + } + EXPECT_EQ(received_sig, SIGUSR1); +} + +TEST(SanitizerCommon, BlockSignalsExceptPipe) { + // Manually block SIGPIPE; ScopedBlockSignals should not unblock this + sigset_t block_sigset; + sigemptyset(&block_sigset); + sigaddset(&block_sigset, SIGPIPE); + sigprocmask(SIG_BLOCK, &block_sigset, NULL); + { + __sanitizer_sigset_t sigset = {}; + ScopedBlockSignals block(&sigset); + + received_sig = -1; + signal(SIGPIPE, signal_handler); + raise(SIGPIPE); + EXPECT_EQ(received_sig, -1); + } + sigprocmask(SIG_UNBLOCK, &block_sigset, NULL); + EXPECT_EQ(received_sig, SIGPIPE); +} +#endif // SANITIZER_LINUX && !SANITIZER_ANDROID + +} // namespace __sanitizer From e88b5fcc1d9bddc0a314ccc72bc47cd45aa6c9f4 Mon Sep 17 00:00:00 2001 From: Thurston Dang Date: Wed, 13 Nov 2024 21:10:10 +0000 Subject: [PATCH 2/7] Revert to pre-#98200 behavior for Android --- compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp b/compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp index dac5683a090217..9cd853201bbd60 100644 --- a/compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp +++ b/compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp @@ -169,9 +169,6 @@ void SetSigProcMask(__sanitizer_sigset_t *set, __sanitizer_sigset_t *oldset) { // Equivalently: newset[signum] = newset[signum] & oldset[signum] static void KeepUnblocked(__sanitizer_sigset_t &newset, __sanitizer_sigset_t &oldset, int signum) { - // FIXME: Figure out why Android tests fail with internal_sigismember - // See also: comment in BlockSignals(). - // In the meantime, we prefer to sometimes incorrectly unblock signals. if (SANITIZER_ANDROID || !internal_sigismember(&oldset, signum)) internal_sigdelset(&newset, signum); } @@ -181,9 +178,7 @@ static void KeepUnblocked(__sanitizer_sigset_t &newset, void BlockSignals(__sanitizer_sigset_t *oldset) { __sanitizer_sigset_t currentset; # if !SANITIZER_ANDROID - // FIXME: SetSigProcMask and pthread_sigmask cause mysterious failures - // See also: comment in KeepUnblocked(). - // In the meantime, we prefer to sometimes incorrectly unblock signals. + // FIXME: SetSigProcMask cause mysterious failures on Android SetSigProcMask(NULL, ¤tset); # endif @@ -200,9 +195,12 @@ void BlockSignals(__sanitizer_sigset_t *oldset) { // If this signal is blocked, such calls cannot be handled and the process may // hang. KeepUnblocked(newset, currentset, 31); +# endif +# if SANITIZER_LINUX && !SANITIZER_ANDROID // Don't block synchronous signals // but also don't unblock signals that the user had deliberately blocked. + // FIXME: this causes mysterious failures on Android KeepUnblocked(newset, currentset, SIGSEGV); KeepUnblocked(newset, currentset, SIGBUS); KeepUnblocked(newset, currentset, SIGILL); From 5789cca9258bd8cf249aa3594d65246a5e74d9a1 Mon Sep 17 00:00:00 2001 From: Thurston Dang Date: Wed, 13 Nov 2024 21:12:56 +0000 Subject: [PATCH 3/7] Add FIXME --- compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp b/compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp index 9cd853201bbd60..98472b24396c62 100644 --- a/compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp +++ b/compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp @@ -169,6 +169,7 @@ void SetSigProcMask(__sanitizer_sigset_t *set, __sanitizer_sigset_t *oldset) { // Equivalently: newset[signum] = newset[signum] & oldset[signum] static void KeepUnblocked(__sanitizer_sigset_t &newset, __sanitizer_sigset_t &oldset, int signum) { + // FIXME: this causes mysterious failures on Android if (SANITIZER_ANDROID || !internal_sigismember(&oldset, signum)) internal_sigdelset(&newset, signum); } From 7558d41d7edbdc0170a11a61db05ad4deaa5b12c Mon Sep 17 00:00:00 2001 From: Thurston Dang Date: Wed, 13 Nov 2024 21:23:02 +0000 Subject: [PATCH 4/7] Add https://github.com/google/sanitizers/issues/1816 to FIXMEs --- compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp b/compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp index 98472b24396c62..9505c7b1746f0f 100644 --- a/compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp +++ b/compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp @@ -169,7 +169,7 @@ void SetSigProcMask(__sanitizer_sigset_t *set, __sanitizer_sigset_t *oldset) { // Equivalently: newset[signum] = newset[signum] & oldset[signum] static void KeepUnblocked(__sanitizer_sigset_t &newset, __sanitizer_sigset_t &oldset, int signum) { - // FIXME: this causes mysterious failures on Android + // FIXME: https://github.com/google/sanitizers/issues/1816 if (SANITIZER_ANDROID || !internal_sigismember(&oldset, signum)) internal_sigdelset(&newset, signum); } @@ -179,7 +179,7 @@ static void KeepUnblocked(__sanitizer_sigset_t &newset, void BlockSignals(__sanitizer_sigset_t *oldset) { __sanitizer_sigset_t currentset; # if !SANITIZER_ANDROID - // FIXME: SetSigProcMask cause mysterious failures on Android + // FIXME: https://github.com/google/sanitizers/issues/1816 SetSigProcMask(NULL, ¤tset); # endif @@ -201,7 +201,7 @@ void BlockSignals(__sanitizer_sigset_t *oldset) { # if SANITIZER_LINUX && !SANITIZER_ANDROID // Don't block synchronous signals // but also don't unblock signals that the user had deliberately blocked. - // FIXME: this causes mysterious failures on Android + // FIXME: https://github.com/google/sanitizers/issues/1816 KeepUnblocked(newset, currentset, SIGSEGV); KeepUnblocked(newset, currentset, SIGBUS); KeepUnblocked(newset, currentset, SIGILL); From 8ec15506e05e9ebd317bd1dad2783c00b2920e4b Mon Sep 17 00:00:00 2001 From: Thurston Dang Date: Wed, 13 Nov 2024 23:49:17 +0000 Subject: [PATCH 5/7] Add SANITIZER_LINUX guard --- compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp b/compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp index 9505c7b1746f0f..f63d8975d42bda 100644 --- a/compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp +++ b/compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp @@ -177,11 +177,14 @@ static void KeepUnblocked(__sanitizer_sigset_t &newset, // Block asynchronous signals void BlockSignals(__sanitizer_sigset_t *oldset) { +# if SANITIZER_LINUX __sanitizer_sigset_t currentset; + # if !SANITIZER_ANDROID // FIXME: https://github.com/google/sanitizers/issues/1816 SetSigProcMask(NULL, ¤tset); -# endif +# endif // SANITIZER_ANDROID +# endif // SANITIZER_LINUX __sanitizer_sigset_t newset; internal_sigfillset(&newset); From 625f812233729bff4650725fd25ed927eb0854e2 Mon Sep 17 00:00:00 2001 From: Thurston Dang Date: Thu, 14 Nov 2024 00:07:27 +0000 Subject: [PATCH 6/7] Refactor --- .../lib/sanitizer_common/sanitizer_linux.cpp | 20 +++++++++---------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp b/compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp index f63d8975d42bda..ea496664cb1c0c 100644 --- a/compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp +++ b/compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp @@ -180,28 +180,25 @@ void BlockSignals(__sanitizer_sigset_t *oldset) { # if SANITIZER_LINUX __sanitizer_sigset_t currentset; -# if !SANITIZER_ANDROID + __sanitizer_sigset_t newset; + internal_sigfillset(&newset); + +# if !SANITIZER_ANDROID // FIXME: https://github.com/google/sanitizers/issues/1816 SetSigProcMask(NULL, ¤tset); -# endif // SANITIZER_ANDROID -# endif // SANITIZER_LINUX - __sanitizer_sigset_t newset; - internal_sigfillset(&newset); -# if SANITIZER_LINUX && !SANITIZER_ANDROID // Glibc uses SIGSETXID signal during setuid call. If this signal is blocked // on any thread, setuid call hangs. // See test/sanitizer_common/TestCases/Linux/setuid.c. KeepUnblocked(newset, currentset, 33); -# endif -# if SANITIZER_LINUX +# endif // !SANITIZER_ANDROID + // Seccomp-BPF-sandboxed processes rely on SIGSYS to handle trapped syscalls. // If this signal is blocked, such calls cannot be handled and the process may // hang. KeepUnblocked(newset, currentset, 31); -# endif -# if SANITIZER_LINUX && !SANITIZER_ANDROID +# if !SANITIZER_ANDROID // Don't block synchronous signals // but also don't unblock signals that the user had deliberately blocked. // FIXME: https://github.com/google/sanitizers/issues/1816 @@ -212,9 +209,10 @@ void BlockSignals(__sanitizer_sigset_t *oldset) { KeepUnblocked(newset, currentset, SIGABRT); KeepUnblocked(newset, currentset, SIGFPE); KeepUnblocked(newset, currentset, SIGPIPE); -# endif +# endif //! SANITIZER_ANDROID SetSigProcMask(&newset, oldset); +# endif // SANITIZER_LINUX } ScopedBlockSignals::ScopedBlockSignals(__sanitizer_sigset_t *copy) { From a6988aa91a590703927be74dfd9b8c977b4f0233 Mon Sep 17 00:00:00 2001 From: Thurston Dang Date: Thu, 14 Nov 2024 00:31:33 +0000 Subject: [PATCH 7/7] Fix logic error pointed out by Vitaly --- compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp b/compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp index ea496664cb1c0c..04b095dca904ab 100644 --- a/compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp +++ b/compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp @@ -177,12 +177,12 @@ static void KeepUnblocked(__sanitizer_sigset_t &newset, // Block asynchronous signals void BlockSignals(__sanitizer_sigset_t *oldset) { -# if SANITIZER_LINUX - __sanitizer_sigset_t currentset; - __sanitizer_sigset_t newset; internal_sigfillset(&newset); +# if SANITIZER_LINUX + __sanitizer_sigset_t currentset; + # if !SANITIZER_ANDROID // FIXME: https://github.com/google/sanitizers/issues/1816 SetSigProcMask(NULL, ¤tset); @@ -211,8 +211,9 @@ void BlockSignals(__sanitizer_sigset_t *oldset) { KeepUnblocked(newset, currentset, SIGPIPE); # endif //! SANITIZER_ANDROID - SetSigProcMask(&newset, oldset); # endif // SANITIZER_LINUX + + SetSigProcMask(&newset, oldset); } ScopedBlockSignals::ScopedBlockSignals(__sanitizer_sigset_t *copy) {