From 0672505f025c23c22f1aea75c69b557216d2c9a2 Mon Sep 17 00:00:00 2001 From: Thurston Dang Date: Wed, 23 Oct 2024 11:04:27 +0000 Subject: [PATCH 1/7] [sanitizer_common] AND signals in BlockSignals instead of deleting 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. Fixes #113385 --- .../lib/sanitizer_common/sanitizer_linux.cpp | 36 ++++++++++++------- 1 file changed, 24 insertions(+), 12 deletions(-) diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp b/compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp index 33107eb0b4299..1f74abfb39b31 100644 --- a/compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp +++ b/compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp @@ -164,33 +164,45 @@ void SetSigProcMask(__sanitizer_sigset_t *set, __sanitizer_sigset_t *oldset) { CHECK_EQ(0, internal_sigprocmask(SIG_SETMASK, set, oldset)); } +// Deletes the specified signal from newset, if it is not present in oldset +// Equivalently: newset[signum] = newset[signum] & oldset[signum] +void internal_sigandset_individual(__sanitizer_sigset_t *newset, + __sanitizer_sigset_t *oldset, int signum) { + if (!internal_sigismember(oldset, signum)) + internal_sigdelset(newset, signum); +} + // Block asynchronous signals void BlockSignals(__sanitizer_sigset_t *oldset) { - __sanitizer_sigset_t set; - internal_sigfillset(&set); + __sanitizer_sigset_t currentset; + SetSigProcMask(NULL, ¤tset); + + __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); + internal_sigdelset(&newset, 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); + internal_sigdelset(&newset, 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. + internal_sigandset_individual(&newset, ¤tset, SIGSEGV); + internal_sigandset_individual(&newset, ¤tset, SIGBUS); + internal_sigandset_individual(&newset, ¤tset, SIGILL); + internal_sigandset_individual(&newset, ¤tset, SIGTRAP); + internal_sigandset_individual(&newset, ¤tset, SIGABRT); + internal_sigandset_individual(&newset, ¤tset, SIGFPE); + internal_sigandset_individual(&newset, ¤tset, SIGPIPE); # endif - SetSigProcMask(&set, oldset); + SetSigProcMask(&newset, oldset); } ScopedBlockSignals::ScopedBlockSignals(__sanitizer_sigset_t *copy) { From d510d43f8b8c416c6ed851d930ce83c121252457 Mon Sep 17 00:00:00 2001 From: Vitaly Buka Date: Fri, 25 Oct 2024 13:13:52 -0700 Subject: [PATCH 2/7] Rename and use for all signals --- .../lib/sanitizer_common/sanitizer_linux.cpp | 22 +++++++++---------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp b/compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp index 1f74abfb39b31..111591d6f63c6 100644 --- a/compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp +++ b/compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp @@ -166,8 +166,8 @@ void SetSigProcMask(__sanitizer_sigset_t *set, __sanitizer_sigset_t *oldset) { // Deletes the specified signal from newset, if it is not present in oldset // Equivalently: newset[signum] = newset[signum] & oldset[signum] -void internal_sigandset_individual(__sanitizer_sigset_t *newset, - __sanitizer_sigset_t *oldset, int signum) { +static void KeepUnblocked(__sanitizer_sigset_t &newset, + __sanitizer_sigset_t &oldset, int signum) { if (!internal_sigismember(oldset, signum)) internal_sigdelset(newset, signum); } @@ -183,23 +183,23 @@ void BlockSignals(__sanitizer_sigset_t *oldset) { // 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(&newset, 33); + KeepUnblocked(&newset, ¤tset, 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(&newset, 31); + KeepUnblocked(&newset, ¤tset, 31); // Don't block synchronous signals // but also don't unblock signals that the user had deliberately blocked. - internal_sigandset_individual(&newset, ¤tset, SIGSEGV); - internal_sigandset_individual(&newset, ¤tset, SIGBUS); - internal_sigandset_individual(&newset, ¤tset, SIGILL); - internal_sigandset_individual(&newset, ¤tset, SIGTRAP); - internal_sigandset_individual(&newset, ¤tset, SIGABRT); - internal_sigandset_individual(&newset, ¤tset, SIGFPE); - internal_sigandset_individual(&newset, ¤tset, SIGPIPE); + KeepUnblocked(&newset, ¤tset, SIGSEGV); + KeepUnblocked(&newset, ¤tset, SIGBUS); + KeepUnblocked(&newset, ¤tset, SIGILL); + KeepUnblocked(&newset, ¤tset, SIGTRAP); + KeepUnblocked(&newset, ¤tset, SIGABRT); + KeepUnblocked(&newset, ¤tset, SIGFPE); + KeepUnblocked(&newset, ¤tset, SIGPIPE); # endif SetSigProcMask(&newset, oldset); From e8f090e5eb49fb6eb7e0c47d24ead4567d29e48e Mon Sep 17 00:00:00 2001 From: Thurston Dang Date: Tue, 29 Oct 2024 21:13:29 +0000 Subject: [PATCH 3/7] Fix up & --- .../lib/sanitizer_common/sanitizer_linux.cpp | 22 +++++++++---------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp b/compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp index 111591d6f63c6..82bc21d341a8b 100644 --- a/compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp +++ b/compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp @@ -168,8 +168,8 @@ 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) { - if (!internal_sigismember(oldset, signum)) - internal_sigdelset(newset, signum); + if (!internal_sigismember(&oldset, signum)) + internal_sigdelset(&newset, signum); } // Block asynchronous signals @@ -183,23 +183,23 @@ void BlockSignals(__sanitizer_sigset_t *oldset) { // 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, ¤tset, 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. - KeepUnblocked(&newset, ¤tset, 31); + KeepUnblocked(newset, currentset, 31); // Don't block synchronous signals // but also don't unblock signals that the user had deliberately blocked. - KeepUnblocked(&newset, ¤tset, SIGSEGV); - KeepUnblocked(&newset, ¤tset, SIGBUS); - KeepUnblocked(&newset, ¤tset, SIGILL); - KeepUnblocked(&newset, ¤tset, SIGTRAP); - KeepUnblocked(&newset, ¤tset, SIGABRT); - KeepUnblocked(&newset, ¤tset, SIGFPE); - KeepUnblocked(&newset, ¤tset, SIGPIPE); + 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(&newset, oldset); From d46231d6936ccfcd8a5fd61affdd414c60941361 Mon Sep 17 00:00:00 2001 From: Thurston Dang Date: Tue, 29 Oct 2024 21:14:34 +0000 Subject: [PATCH 4/7] Add tests for BlockSignals --- .../lib/sanitizer_common/tests/CMakeLists.txt | 1 + .../tests/sanitizer_block_signals.cpp | 87 +++++++++++++++++++ 2 files changed, 88 insertions(+) create mode 100644 compiler-rt/lib/sanitizer_common/tests/sanitizer_block_signals.cpp diff --git a/compiler-rt/lib/sanitizer_common/tests/CMakeLists.txt b/compiler-rt/lib/sanitizer_common/tests/CMakeLists.txt index 2b4c15125263a..fef8bb772e0e0 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 0000000000000..391b122ba4973 --- /dev/null +++ b/compiler-rt/lib/sanitizer_common/tests/sanitizer_block_signals.cpp @@ -0,0 +1,87 @@ +//===-- 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 "gtest/gtest.h" +#include "sanitizer_common/sanitizer_linux.h" +#include +#include + +namespace __sanitizer { + +#if SANITIZER_LINUX +volatile int received_sig = -1; + +void signal_handler (int signum) { + received_sig = signum; +} + +TEST(SanitizerCommon, BlockSignals) { + // No signals blocked + { + signal (SIGUSR1, signal_handler); + raise (SIGUSR1); + while (received_sig == -1) + sleep(1); + EXPECT_EQ(received_sig, SIGUSR1); + + received_sig = -1; + signal (SIGPIPE, signal_handler); + raise (SIGPIPE); + while (received_sig == -1) + sleep(1); + EXPECT_EQ(received_sig, SIGPIPE); + } + + // ScopedBlockSignals; SIGUSR1 should be blocked but not SIGPIPE + { + __sanitizer_sigset_t sigset = {}; + ScopedBlockSignals block(&sigset); + + received_sig = -1; + signal (SIGUSR1, signal_handler); + raise (SIGUSR1); + sleep(1); + EXPECT_EQ(received_sig, -1); + + received_sig = -1; + signal (SIGPIPE, signal_handler); + raise (SIGPIPE); + while (received_sig == -1) + sleep(1); + EXPECT_EQ(received_sig, SIGPIPE); + } + while (received_sig == -1) + sleep(1); + EXPECT_EQ(received_sig, SIGUSR1); + + // 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); + sleep(1); + EXPECT_EQ(received_sig, -1); + } + sigprocmask(SIG_UNBLOCK, &block_sigset, NULL); + while (received_sig == -1) + sleep(1); + EXPECT_EQ(received_sig, SIGPIPE); +} +#endif // SANITIZER_LINUX + +} // namespace __sanitizer From 9ad1184442cfe6258d726f9024e6bac4612a58a1 Mon Sep 17 00:00:00 2001 From: Thurston Dang Date: Tue, 29 Oct 2024 21:32:25 +0000 Subject: [PATCH 5/7] clang-format --- .../tests/sanitizer_block_signals.cpp | 108 +++++++++--------- 1 file changed, 51 insertions(+), 57 deletions(-) diff --git a/compiler-rt/lib/sanitizer_common/tests/sanitizer_block_signals.cpp b/compiler-rt/lib/sanitizer_common/tests/sanitizer_block_signals.cpp index 391b122ba4973..e5b98fc3d73fc 100644 --- a/compiler-rt/lib/sanitizer_common/tests/sanitizer_block_signals.cpp +++ b/compiler-rt/lib/sanitizer_common/tests/sanitizer_block_signals.cpp @@ -9,79 +9,73 @@ // This file is a part of sanitizer_common unit tests. // //===----------------------------------------------------------------------===// -#include "gtest/gtest.h" -#include "sanitizer_common/sanitizer_linux.h" #include #include +#include "gtest/gtest.h" +#include "sanitizer_common/sanitizer_linux.h" + namespace __sanitizer { #if SANITIZER_LINUX volatile int received_sig = -1; -void signal_handler (int signum) { - received_sig = signum; -} +void signal_handler(int signum) { received_sig = signum; } TEST(SanitizerCommon, BlockSignals) { - // No signals blocked - { - signal (SIGUSR1, signal_handler); - raise (SIGUSR1); - while (received_sig == -1) - sleep(1); - EXPECT_EQ(received_sig, SIGUSR1); + // No signals blocked + { + signal(SIGUSR1, signal_handler); + raise(SIGUSR1); + while (received_sig == -1) sleep(1); + EXPECT_EQ(received_sig, SIGUSR1); - received_sig = -1; - signal (SIGPIPE, signal_handler); - raise (SIGPIPE); - while (received_sig == -1) - sleep(1); - EXPECT_EQ(received_sig, SIGPIPE); - } + received_sig = -1; + signal(SIGPIPE, signal_handler); + raise(SIGPIPE); + while (received_sig == -1) sleep(1); + EXPECT_EQ(received_sig, SIGPIPE); + } - // ScopedBlockSignals; SIGUSR1 should be blocked but not SIGPIPE - { - __sanitizer_sigset_t sigset = {}; - ScopedBlockSignals block(&sigset); + // ScopedBlockSignals; SIGUSR1 should be blocked but not SIGPIPE + { + __sanitizer_sigset_t sigset = {}; + ScopedBlockSignals block(&sigset); - received_sig = -1; - signal (SIGUSR1, signal_handler); - raise (SIGUSR1); - sleep(1); - EXPECT_EQ(received_sig, -1); + received_sig = -1; + signal(SIGUSR1, signal_handler); + raise(SIGUSR1); + sleep(1); + EXPECT_EQ(received_sig, -1); - received_sig = -1; - signal (SIGPIPE, signal_handler); - raise (SIGPIPE); - while (received_sig == -1) - sleep(1); - EXPECT_EQ(received_sig, SIGPIPE); - } - while (received_sig == -1) - sleep(1); - EXPECT_EQ(received_sig, SIGUSR1); + received_sig = -1; + signal(SIGPIPE, signal_handler); + raise(SIGPIPE); + while (received_sig == -1) sleep(1); + EXPECT_EQ(received_sig, SIGPIPE); + } + while (received_sig == -1) sleep(1); + EXPECT_EQ(received_sig, SIGUSR1); - // 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); + // 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); - sleep(1); - EXPECT_EQ(received_sig, -1); - } - sigprocmask(SIG_UNBLOCK, &block_sigset, NULL); - while (received_sig == -1) - sleep(1); - EXPECT_EQ(received_sig, SIGPIPE); + received_sig = -1; + signal(SIGPIPE, signal_handler); + raise(SIGPIPE); + sleep(1); + EXPECT_EQ(received_sig, -1); + } + sigprocmask(SIG_UNBLOCK, &block_sigset, NULL); + while (received_sig == -1) sleep(1); + EXPECT_EQ(received_sig, SIGPIPE); } -#endif // SANITIZER_LINUX +#endif // SANITIZER_LINUX } // namespace __sanitizer From fa0a6f7af06bddc979ba59bbb5616858915bf8d3 Mon Sep 17 00:00:00 2001 From: Thurston Dang Date: Thu, 31 Oct 2024 16:35:27 +0000 Subject: [PATCH 6/7] No more sleep Split into separate test cases --- .../tests/sanitizer_block_signals.cpp | 30 ++++++++----------- 1 file changed, 13 insertions(+), 17 deletions(-) diff --git a/compiler-rt/lib/sanitizer_common/tests/sanitizer_block_signals.cpp b/compiler-rt/lib/sanitizer_common/tests/sanitizer_block_signals.cpp index e5b98fc3d73fc..bb9079c13d5fc 100644 --- a/compiler-rt/lib/sanitizer_common/tests/sanitizer_block_signals.cpp +++ b/compiler-rt/lib/sanitizer_common/tests/sanitizer_block_signals.cpp @@ -22,20 +22,19 @@ volatile int received_sig = -1; void signal_handler(int signum) { received_sig = signum; } -TEST(SanitizerCommon, BlockSignals) { +TEST(SanitizerCommon, NoBlockSignals) { // No signals blocked - { - signal(SIGUSR1, signal_handler); - raise(SIGUSR1); - while (received_sig == -1) sleep(1); - EXPECT_EQ(received_sig, SIGUSR1); + signal(SIGUSR1, signal_handler); + raise(SIGUSR1); + EXPECT_EQ(received_sig, SIGUSR1); - received_sig = -1; - signal(SIGPIPE, signal_handler); - raise(SIGPIPE); - while (received_sig == -1) sleep(1); - EXPECT_EQ(received_sig, SIGPIPE); - } + 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 { @@ -45,18 +44,17 @@ TEST(SanitizerCommon, BlockSignals) { received_sig = -1; signal(SIGUSR1, signal_handler); raise(SIGUSR1); - sleep(1); EXPECT_EQ(received_sig, -1); received_sig = -1; signal(SIGPIPE, signal_handler); raise(SIGPIPE); - while (received_sig == -1) sleep(1); EXPECT_EQ(received_sig, SIGPIPE); } - while (received_sig == -1) sleep(1); EXPECT_EQ(received_sig, SIGUSR1); +} +TEST(SanitizerCommon, BlockSignalsExceptPipe) { // Manually block SIGPIPE; ScopedBlockSignals should not unblock this sigset_t block_sigset; sigemptyset(&block_sigset); @@ -69,11 +67,9 @@ TEST(SanitizerCommon, BlockSignals) { received_sig = -1; signal(SIGPIPE, signal_handler); raise(SIGPIPE); - sleep(1); EXPECT_EQ(received_sig, -1); } sigprocmask(SIG_UNBLOCK, &block_sigset, NULL); - while (received_sig == -1) sleep(1); EXPECT_EQ(received_sig, SIGPIPE); } #endif // SANITIZER_LINUX From a0b2a1ee6925c258d92eaf3f72a67ec8a67bdf91 Mon Sep 17 00:00:00 2001 From: Thurston Dang Date: Thu, 31 Oct 2024 16:46:11 +0000 Subject: [PATCH 7/7] clang-format --- .../lib/sanitizer_common/tests/sanitizer_block_signals.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/compiler-rt/lib/sanitizer_common/tests/sanitizer_block_signals.cpp b/compiler-rt/lib/sanitizer_common/tests/sanitizer_block_signals.cpp index bb9079c13d5fc..17791c1494ca7 100644 --- a/compiler-rt/lib/sanitizer_common/tests/sanitizer_block_signals.cpp +++ b/compiler-rt/lib/sanitizer_common/tests/sanitizer_block_signals.cpp @@ -35,7 +35,6 @@ TEST(SanitizerCommon, NoBlockSignals) { } TEST(SanitizerCommon, BlockSignalsPlain) { - // ScopedBlockSignals; SIGUSR1 should be blocked but not SIGPIPE { __sanitizer_sigset_t sigset = {};