Reapply "[sanitizer_common] AND signals in BlockSignals instead of deleting (#113443)" for non-Android Linux only #115790
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…leting (llvm#113443)" for non-Android Linux only The original patch (25fd366) was reverted in 083a5cd 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 llvm#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 llvm#113385
|
@llvm/pr-subscribers-compiler-rt-sanitizer Author: Thurston Dang (thurstond) ChangesThe original patch (25fd366) was reverted in 083a5cd because it broke some buildbots. This revised patch makes two changes:
Original commit message: Full diff: https://github.com/llvm/llvm-project/pull/115790.diff 3 Files Affected:
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 <signal.h>
+#include <stdio.h>
+
+#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
|
vitalybuka
reviewed
Nov 12, 2024
fmayer
reviewed
Nov 13, 2024
| // 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 |
There was a problem hiding this comment.
can we create some reference? a bug in llvm or google/sanitizers repo that has the logs of the failing buildbot run?
There was a problem hiding this comment.
Filed google/sanitizers#1816 and updated the FIXMEs
vitalybuka
approved these changes
Nov 13, 2024
| // Block asynchronous signals | ||
| void BlockSignals(__sanitizer_sigset_t *oldset) { | ||
| __sanitizer_sigset_t set; | ||
| internal_sigfillset(&set); | ||
| __sanitizer_sigset_t currentset; |
There was a problem hiding this comment.
unused current set warning is possible on non-linux.
There was a problem hiding this comment.
we still need
__sanitizer_sigset_t newset;
internal_sigfillset(&newset);
# if SANITIZER_LINUX
...
#endif
SetSigProcMask(&newset, oldset);
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
vitalybuka
approved these changes
Nov 13, 2024
| # endif // SANITIZER_ANDROID | ||
| # endif // SANITIZER_LINUX | ||
|
|
||
| __sanitizer_sigset_t newset; |
There was a problem hiding this comment.
Move this into the begining of the function,
__sanitizer_sigset_t newset;
internal_sigfillset(&newset);
and we can have single if SANITIZER_LINUX for the whole function
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/24/builds/2677 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/64/builds/1466 Here is the relevant piece of the build log for the reference |
akshayrdeodhar
pushed a commit
to akshayrdeodhar/llvm-project
that referenced
this pull request
Nov 18, 2024
…leting (llvm#113443)" for non-Android Linux only (llvm#115790) The original patch (25fd366) was reverted in 083a5cd because it broke some buildbots. This revised patch makes two changes: - Reverts to *pre-llvm#98200* behavior for Android. This avoids a build breakage on Android. - Only define KeepUnblocked if SANITIZER_LINUX: this avoids a build breakage on solaris, which does not support internal_sigdelset. N.B. Other buildbot failures were non-sanitizer tests and are therefore unrelated. Original commit message: My earlier patch llvm#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 llvm#113385
|
For reference: this will be backported to LLVM 19.x (#116670) |
tru
pushed a commit
that referenced
this pull request
Nov 19, 2024
…leting (#113443)" for non-Android Linux only (#115790) The original patch (25fd366) was reverted in 083a5cd because it broke some buildbots. This revised patch makes two changes: - Reverts to *pre-#98200* behavior for Android. This avoids a build breakage on Android. - Only define KeepUnblocked if SANITIZER_LINUX: this avoids a build breakage on solaris, which does not support internal_sigdelset. N.B. Other buildbot failures were non-sanitizer tests and are therefore unrelated. Original commit message: My earlier patch #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 (cherry picked from commit 531acf9)
nikic
pushed a commit
to rust-lang/llvm-project
that referenced
this pull request
Nov 20, 2024
…leting (llvm#113443)" for non-Android Linux only (llvm#115790) The original patch (25fd366) was reverted in 083a5cd because it broke some buildbots. This revised patch makes two changes: - Reverts to *pre-llvm#98200* behavior for Android. This avoids a build breakage on Android. - Only define KeepUnblocked if SANITIZER_LINUX: this avoids a build breakage on solaris, which does not support internal_sigdelset. N.B. Other buildbot failures were non-sanitizer tests and are therefore unrelated. Original commit message: My earlier patch llvm#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 llvm#113385 (cherry picked from commit 531acf9)
adrian-prantl
pushed a commit
to adrian-prantl/llvm-project
that referenced
this pull request
Dec 4, 2024
…leting (llvm#113443)" for non-Android Linux only (llvm#115790) The original patch (25fd366) was reverted in 083a5cd because it broke some buildbots. This revised patch makes two changes: - Reverts to *pre-llvm#98200* behavior for Android. This avoids a build breakage on Android. - Only define KeepUnblocked if SANITIZER_LINUX: this avoids a build breakage on solaris, which does not support internal_sigdelset. N.B. Other buildbot failures were non-sanitizer tests and are therefore unrelated. Original commit message: My earlier patch llvm#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 llvm#113385 (cherry picked from commit 531acf9) (cherry picked from commit 6925f3c)
adrian-prantl
pushed a commit
to adrian-prantl/llvm-project
that referenced
this pull request
Dec 5, 2024
…leting (llvm#113443)" for non-Android Linux only (llvm#115790) The original patch (25fd366) was reverted in 083a5cd because it broke some buildbots. This revised patch makes two changes: - Reverts to *pre-llvm#98200* behavior for Android. This avoids a build breakage on Android. - Only define KeepUnblocked if SANITIZER_LINUX: this avoids a build breakage on solaris, which does not support internal_sigdelset. N.B. Other buildbot failures were non-sanitizer tests and are therefore unrelated. Original commit message: My earlier patch llvm#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 llvm#113385 (cherry picked from commit 531acf9) (cherry picked from commit 6925f3c)
adrian-prantl
pushed a commit
to adrian-prantl/llvm-project
that referenced
this pull request
Dec 5, 2024
…leting (llvm#113443)" for non-Android Linux only (llvm#115790) The original patch (25fd366) was reverted in 083a5cd because it broke some buildbots. This revised patch makes two changes: - Reverts to *pre-llvm#98200* behavior for Android. This avoids a build breakage on Android. - Only define KeepUnblocked if SANITIZER_LINUX: this avoids a build breakage on solaris, which does not support internal_sigdelset. N.B. Other buildbot failures were non-sanitizer tests and are therefore unrelated. Original commit message: My earlier patch llvm#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 llvm#113385 (cherry picked from commit 531acf9) (cherry picked from commit 6925f3c)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The original patch (25fd366) was reverted in 083a5cd because it broke some buildbots.
This revised patch makes two changes:
N.B. Other buildbot failures were non-sanitizer tests and are therefore unrelated.
Original commit message:
My earlier patch #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