[sanitizer_common] AND signals in BlockSignals instead of deleting #113443
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
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. Fixes llvm#113385
|
@llvm/pr-subscribers-compiler-rt-sanitizer Author: Thurston Dang (thurstond) ChangesMy 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. Fixes #113385 Full diff: https://github.com/llvm/llvm-project/pull/113443.diff 1 Files Affected:
diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp b/compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp
index 33107eb0b42993..1f74abfb39b31e 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) {
|
vitalybuka
reviewed
Oct 25, 2024
vitalybuka
reviewed
Oct 25, 2024
vitalybuka
approved these changes
Oct 25, 2024
|
After the merge, we need to request cherry pick into 19 |
20 tasks
I've added a test, please review. |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
vitalybuka
reviewed
Oct 31, 2024
| void signal_handler(int signum) { received_sig = signum; } | ||
|
|
||
| TEST(SanitizerCommon, BlockSignals) { | ||
| // No signals blocked |
There was a problem hiding this comment.
Instead of large test with 3 {}
please split into 3
TEST(SanitizerCommon, BlockSignal{Name1, Name2, Name3}) {
}
vitalybuka
reviewed
Oct 31, 2024
|
|
||
| received_sig = -1; | ||
| signal(SIGPIPE, signal_handler); | ||
| raise(SIGPIPE); |
vitalybuka
approved these changes
Oct 31, 2024
Split into separate test cases
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/73/builds/7845 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/51/builds/5798 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/186/builds/3642 Here is the relevant piece of the build log for the reference |
thurstond
added a commit
that referenced
this pull request
Oct 31, 2024
…eting (#113443)" This reverts commit 25fd366. Reason: buildbot breakage (https://lab.llvm.org/buildbot/#/builders/186/builds/3642)
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/13/builds/3259 Here is the relevant piece of the build log for the reference |
smallp-o-p
pushed a commit
to smallp-o-p/llvm-project
that referenced
this pull request
Nov 3, 2024
…lvm#113443) 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 --------- Co-authored-by: Vitaly Buka <[email protected]>
smallp-o-p
pushed a commit
to smallp-o-p/llvm-project
that referenced
this pull request
Nov 3, 2024
…eting (llvm#113443)" This reverts commit 25fd366. Reason: buildbot breakage (https://lab.llvm.org/buildbot/#/builders/186/builds/3642)
smallp-o-p
pushed a commit
to smallp-o-p/llvm-project
that referenced
this pull request
Nov 3, 2024
…lvm#113443) 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 --------- Co-authored-by: Vitaly Buka <[email protected]>
smallp-o-p
pushed a commit
to smallp-o-p/llvm-project
that referenced
this pull request
Nov 3, 2024
…eting (llvm#113443)" This reverts commit 25fd366. Reason: buildbot breakage (https://lab.llvm.org/buildbot/#/builders/186/builds/3642)
NoumanAmir657
pushed a commit
to NoumanAmir657/llvm-project
that referenced
this pull request
Nov 4, 2024
…lvm#113443) 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 --------- Co-authored-by: Vitaly Buka <[email protected]>
NoumanAmir657
pushed a commit
to NoumanAmir657/llvm-project
that referenced
this pull request
Nov 4, 2024
…eting (llvm#113443)" This reverts commit 25fd366. Reason: buildbot breakage (https://lab.llvm.org/buildbot/#/builders/186/builds/3642)
thurstond
added a commit
to thurstond/llvm-project
that referenced
this pull request
Nov 11, 2024
…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
thurstond
added a commit
that referenced
this pull request
Nov 14, 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
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
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.
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.
Fixes #113385