[sanitizer] Add cloak_sanitizer_signal_handlers runtime option #162746
Conversation
This file contains hidden or 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
If set, signal/sigaction will pretend that the sanitizers did not
preinstall any signal handlers. If a user successfully installs a signal handler, it will not be cloaked.
The flag is currently off by default, which means this patch should not
affect the behavior of any sanitizers.
This can be useful in an ecosystem where:
1) there exists a library that will install a signal handler iff it does not detect a
preinstalled signal handler (a heuristic to prevent overriding user-installed exception handlers etc.)
2) the aforementioned library is linked in to some, but not all, apps
3) user-installed signal handlers have the highest priority, followed by
the library-installed signal handler, and then the sanitizer's signal
handler
This patch also adds an API function, __sanitizer_uncloak_preinstalled_signal_handlers(),
that can be used to effectively undo the runtime option. This makes it
possible to set the cloak_sanitizer_signal_handlers option broadly, and
selectively programmatically disable it for incompatible programs (e.g.,
allow_user_segv.cpp, which wants to manually call ASan's preinstalled handler).
The flag is in sanitizer_common, though it is currently only supported
in ASan, LSan and UBSan.
|
@llvm/pr-subscribers-compiler-rt-sanitizer Author: Thurston Dang (thurstond) ChangesIf set, signal/sigaction will pretend that the sanitizers did not preinstall any signal handlers. If a user successfully installs a signal handler, it will not be cloaked. The flag is currently off by default, which means this patch should not affect the behavior of any sanitizers. This can be useful in an ecosystem where:
The flag is in sanitizer_common, though it is currently only supported in ASan, LSan and UBSan. Full diff: https://github.com/llvm/llvm-project/pull/162746.diff 8 Files Affected:
diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_common.cpp b/compiler-rt/lib/sanitizer_common/sanitizer_common.cpp
index 6cd69a53093e7..60f2834d277a7 100644
--- a/compiler-rt/lib/sanitizer_common/sanitizer_common.cpp
+++ b/compiler-rt/lib/sanitizer_common/sanitizer_common.cpp
@@ -24,6 +24,8 @@ namespace __sanitizer {
const char *SanitizerToolName = "SanitizerTool";
+bool signal_handler_is_from_sanitizer[MaxSignals] = {0};
+
atomic_uint32_t current_verbosity;
uptr PageSizeCached;
u32 NumberOfCPUsCached;
diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_common.h b/compiler-rt/lib/sanitizer_common/sanitizer_common.h
index 3e82df498572c..10bd83118e9a4 100644
--- a/compiler-rt/lib/sanitizer_common/sanitizer_common.h
+++ b/compiler-rt/lib/sanitizer_common/sanitizer_common.h
@@ -52,6 +52,9 @@ const u64 kExternalPCBit = 1ULL << 60;
extern const char *SanitizerToolName; // Can be changed by the tool.
+const int MaxSignals = 64;
+extern bool signal_handler_is_from_sanitizer[MaxSignals];
+
extern atomic_uint32_t current_verbosity;
inline void SetVerbosity(int verbosity) {
atomic_store(¤t_verbosity, verbosity, memory_order_relaxed);
diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_flags.inc b/compiler-rt/lib/sanitizer_common/sanitizer_flags.inc
index 650a4580bbcf0..5f449907f6011 100644
--- a/compiler-rt/lib/sanitizer_common/sanitizer_flags.inc
+++ b/compiler-rt/lib/sanitizer_common/sanitizer_flags.inc
@@ -113,6 +113,11 @@ COMMON_FLAG(HandleSignalMode, handle_sigfpe, kHandleSignalYes,
COMMON_FLAG(bool, allow_user_segv_handler, true,
"Deprecated. True has no effect, use handle_sigbus=1. If false, "
"handle_*=1 will be upgraded to handle_*=2.")
+COMMON_FLAG(bool, cloak_sanitizer_signal_handlers, false,
+ "If set, signal/sigaction will pretend that sanitizers did not "
+ "preinstall any signal handlers. If the user subsequently installs "
+ "a signal handler, this will disable cloaking for the respective "
+ "signal.")
COMMON_FLAG(bool, use_sigaltstack, true,
"If set, uses alternate stack for signal handling.")
COMMON_FLAG(bool, detect_deadlocks, true,
diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_posix_libcdep.cpp b/compiler-rt/lib/sanitizer_common/sanitizer_posix_libcdep.cpp
index b1eb2009cf157..b06f6b1028e7b 100644
--- a/compiler-rt/lib/sanitizer_common/sanitizer_posix_libcdep.cpp
+++ b/compiler-rt/lib/sanitizer_common/sanitizer_posix_libcdep.cpp
@@ -223,6 +223,9 @@ static void MaybeInstallSigaction(int signum,
if (common_flags()->use_sigaltstack) sigact.sa_flags |= SA_ONSTACK;
CHECK_EQ(0, internal_sigaction(signum, &sigact, nullptr));
VReport(1, "Installed the sigaction for signal %d\n", signum);
+
+ if (common_flags()->cloak_sanitizer_signal_handlers)
+ signal_handler_is_from_sanitizer[signum] = true;
}
void InstallDeadlySignalHandlers(SignalHandlerType handler) {
@@ -230,6 +233,7 @@ void InstallDeadlySignalHandlers(SignalHandlerType handler) {
// This will cause SetAlternateSignalStack to be called twice, but the stack
// will be actually set only once.
if (common_flags()->use_sigaltstack) SetAlternateSignalStack();
+
MaybeInstallSigaction(SIGSEGV, handler);
MaybeInstallSigaction(SIGBUS, handler);
MaybeInstallSigaction(SIGABRT, handler);
diff --git a/compiler-rt/lib/sanitizer_common/sanitizer_signal_interceptors.inc b/compiler-rt/lib/sanitizer_common/sanitizer_signal_interceptors.inc
index 94e4e2954a3b9..b26276792d7b3 100644
--- a/compiler-rt/lib/sanitizer_common/sanitizer_signal_interceptors.inc
+++ b/compiler-rt/lib/sanitizer_common/sanitizer_signal_interceptors.inc
@@ -24,8 +24,10 @@ using namespace __sanitizer;
#endif
#ifndef SIGNAL_INTERCEPTOR_SIGNAL_IMPL
-#define SIGNAL_INTERCEPTOR_SIGNAL_IMPL(func, signum, handler) \
- { return REAL(func)(signum, handler); }
+# define SIGNAL_INTERCEPTOR_SIGNAL_IMPL(func, signum, handler) \
+ { \
+ ret = REAL(func)(signum, handler); \
+ }
#endif
#ifndef SIGNAL_INTERCEPTOR_SIGACTION_IMPL
@@ -35,9 +37,10 @@ using namespace __sanitizer;
Printf( \
"Warning: REAL(sigaction_symname) == nullptr. This may happen " \
"if you link with ubsan statically. Sigaction will not work.\n"); \
- return -1; \
+ ret = -1; \
+ } else { \
+ ret = REAL(sigaction_symname)(signum, act, oldact); \
} \
- return REAL(sigaction_symname)(signum, act, oldact); \
}
#endif
@@ -45,7 +48,10 @@ using namespace __sanitizer;
INTERCEPTOR(uptr, bsd_signal, int signum, uptr handler) {
SIGNAL_INTERCEPTOR_ENTER();
if (GetHandleSignalMode(signum) == kHandleSignalExclusive) return 0;
+
+ int ret;
SIGNAL_INTERCEPTOR_SIGNAL_IMPL(bsd_signal, signum, handler);
+ return ret;
}
#define INIT_BSD_SIGNAL COMMON_INTERCEPT_FUNCTION(bsd_signal)
#else // SANITIZER_INTERCEPT_BSD_SIGNAL
@@ -56,19 +62,50 @@ INTERCEPTOR(uptr, bsd_signal, int signum, uptr handler) {
INTERCEPTOR(uptr, signal, int signum, uptr handler) {
SIGNAL_INTERCEPTOR_ENTER();
if (GetHandleSignalMode(signum) == kHandleSignalExclusive)
+ // A side-effect is that a user can never uncloak the sanitizer's
+ // preinstalled signal handler.
return (uptr) nullptr;
+ uptr ret;
SIGNAL_INTERCEPTOR_SIGNAL_IMPL(signal, signum, handler);
+
+ if (signum >= 0 && signum < MaxSignals &&
+ signal_handler_is_from_sanitizer[signum] && ret != sig_err) {
+ // If the user sets a signal handler, it is never cloaked, even if they
+ // reuse a sanitizer's signal handler.
+ signal_handler_is_from_sanitizer[signum] = false;
+
+ ret = sig_dfl;
+ }
+
+ return ret;
}
#define INIT_SIGNAL COMMON_INTERCEPT_FUNCTION(signal)
INTERCEPTOR(int, sigaction_symname, int signum,
const __sanitizer_sigaction *act, __sanitizer_sigaction *oldact) {
SIGNAL_INTERCEPTOR_ENTER();
+
if (GetHandleSignalMode(signum) == kHandleSignalExclusive) {
if (!oldact) return 0;
act = nullptr;
+ // A side-effect is that a user can never uncloak the sanitizer's
+ // preinstalled signal handler.
}
+ int ret;
SIGNAL_INTERCEPTOR_SIGACTION_IMPL(signum, act, oldact);
+
+ if (signum >= 0 && signum < MaxSignals &&
+ signal_handler_is_from_sanitizer[signum] && ret == 0) {
+ if (act)
+ // If the user sets a signal handler, it is never cloaked, even if they
+ // reuse a sanitizer's signal handler.
+ signal_handler_is_from_sanitizer[signum] = false;
+
+ if (oldact)
+ oldact->handler = reinterpret_cast<__sanitizer_sighandler_ptr>(sig_dfl);
+ }
+
+ return ret;
}
#define INIT_SIGACTION COMMON_INTERCEPT_FUNCTION(sigaction_symname)
diff --git a/compiler-rt/test/sanitizer_common/TestCases/Linux/allow_user_segv.cpp b/compiler-rt/test/sanitizer_common/TestCases/Linux/allow_user_segv.cpp
index 1c740153a81d7..0c5a922ecfb83 100644
--- a/compiler-rt/test/sanitizer_common/TestCases/Linux/allow_user_segv.cpp
+++ b/compiler-rt/test/sanitizer_common/TestCases/Linux/allow_user_segv.cpp
@@ -23,6 +23,10 @@
// Flaky errors in debuggerd with "waitpid returned unexpected pid (0)" in logcat.
// UNSUPPORTED: android && i386-target-arch
+// Note: this test case is unusual because it retrieves the original
+// (ASan-installed) signal handler; thus, it is incompatible with the
+// cloak_sanitizer_signal_handlers runtime option.
+
#include <signal.h>
#include <stdio.h>
#include <stdlib.h>
diff --git a/compiler-rt/test/sanitizer_common/TestCases/Linux/cloak_sigaction.cpp b/compiler-rt/test/sanitizer_common/TestCases/Linux/cloak_sigaction.cpp
new file mode 100644
index 0000000000000..3610a3f4e8cf0
--- /dev/null
+++ b/compiler-rt/test/sanitizer_common/TestCases/Linux/cloak_sigaction.cpp
@@ -0,0 +1,41 @@
+// XFAIL: msan
+// XFAIL: tsan
+
+// UNSUPPORTED: android
+// UNSUPPORTED: hwasan
+
+// RUN: %clangxx -O0 %s -o %t
+
+// RUN: %env_tool_opts=handle_segv=1:cloak_sanitizer_signal_handlers=false not %run %t 2>&1 | FileCheck %s --check-prefix=UNCLOAKED
+// RUN: %env_tool_opts=handle_segv=1:cloak_sanitizer_signal_handlers=true not %run %t 2>&1 | FileCheck %s --check-prefix=CLOAKED
+
+#include <sanitizer/common_interface_defs.h>
+#include <signal.h>
+#include <stdio.h>
+#include <stdlib.h>
+
+void handler(int signum, siginfo_t *info, void *context) {
+ printf("Custom signal handler\n");
+ exit(1);
+}
+
+int main(int argc, char *argv[]) {
+ struct sigaction sa = {0};
+ struct sigaction old = {0};
+ sa.sa_flags = SA_SIGINFO;
+ sa.sa_sigaction = &handler;
+ sigaction(SIGSEGV, &sa, &old);
+
+ if (reinterpret_cast<void *>(old.sa_sigaction) == SIG_DFL)
+ printf("Old handler: default\n");
+ // CLOAKED: Old handler: default
+ else
+ printf("Old handler: non-default\n");
+ // UNCLOAKED: Old handler: non-default
+
+ char *c = (char *)0x123;
+ printf("%d\n", *c);
+ // UNCLOAKED,CLOAKED:Custom signal handler
+
+ return 0;
+}
diff --git a/compiler-rt/test/sanitizer_common/TestCases/Linux/cloak_signal.cpp b/compiler-rt/test/sanitizer_common/TestCases/Linux/cloak_signal.cpp
new file mode 100644
index 0000000000000..bf35df469d862
--- /dev/null
+++ b/compiler-rt/test/sanitizer_common/TestCases/Linux/cloak_signal.cpp
@@ -0,0 +1,36 @@
+// XFAIL: msan
+// XFAIL: tsan
+
+// UNSUPPORTED: android
+// UNSUPPORTED: hwasan
+
+// RUN: %clangxx -O0 %s -o %t
+
+// RUN: %env_tool_opts=handle_segv=1:cloak_sanitizer_signal_handlers=false not %run %t 2>&1 | FileCheck %s --check-prefix=UNCLOAKED
+// RUN: %env_tool_opts=handle_segv=1:cloak_sanitizer_signal_handlers=true not %run %t 2>&1 | FileCheck %s --check-prefix=CLOAKED
+
+#include <sanitizer/common_interface_defs.h>
+#include <signal.h>
+#include <stdio.h>
+#include <stdlib.h>
+
+void my_signal_sighandler(int signum) {
+ printf("Custom signal handler\n");
+ exit(1);
+}
+
+int main(int argc, char *argv[]) {
+ __sighandler_t old = signal(SIGSEGV, &my_signal_sighandler);
+ if (old == SIG_DFL)
+ printf("Old handler: default\n");
+ // CLOAKED: Old handler: default
+ else
+ printf("Old handler: non-default\n");
+ // UNCLOAKED: Old handler: non-default
+
+ char *c = (char *)0x123;
+ printf("%d\n", *c);
+ // UNCLOAKED,CLOAKED:Custom signal handler
+
+ return 0;
+}
|
This uncovered a quirk of the existing signal interceptor: cloaking is effectively on when handle_segv=2.
vitalybuka
reviewed
Oct 10, 2025
|
|
||
| if (signum >= 0 && signum < MaxSignals && | ||
| signal_handler_is_from_sanitizer[signum] && ret == 0) { | ||
| if (act) |
There was a problem hiding this comment.
can you make it a function somewher with signal_handler_is_from_sanitizer
and hide MaxSignals and signal_handler_is_from_sanitizer?
vitalybuka
reviewed
Oct 10, 2025
| return -1; \ | ||
| ret = -1; \ | ||
| } else { \ | ||
| ret = REAL(sigaction_symname)(signum, act, oldact); \ |
There was a problem hiding this comment.
It is necessary: the interceptor calls this macro and then we want to do more stuff (e.g., change the return value); if the macro returns immediately, we can't do the followup steps.
I can split it into a separate NFC though?
There was a problem hiding this comment.
I can split it into a separate NFC though?
That's what I meant.
There was a problem hiding this comment.
Refactored to use lambdas per your suggestion in #162916 (comment)
A nice side effect is MSan/TSan is now supported as well.
vitalybuka
reviewed
Oct 10, 2025
|
|
||
| const char *SanitizerToolName = "SanitizerTool"; | ||
|
|
||
| const int MaxSignals = 64; |
There was a problem hiding this comment.
constexpr
and static
Also you don't realy need MaxSignals
ARRAY_SIZE() is more trustworthy for users
There was a problem hiding this comment.
Replaced MaxSignals with ARRAY_SIZE()
vitalybuka
reviewed
Oct 10, 2025
| const char *SanitizerToolName = "SanitizerTool"; | ||
|
|
||
| const int MaxSignals = 64; | ||
| bool signal_handler_is_from_sanitizer[MaxSignals] = {0}; |
There was a problem hiding this comment.
I don't see any locking, so should be atomic
SetSignalHandlerFromSanitizer/IsSignalHandlerFromSanitizer
should be just atomic SetSignalHandlerFromSanitizer which returns previos value
There was a problem hiding this comment.
I will add atomicity.
I don't think it's feasible to combine SetSignalHandlerFromSanitizer/IsSignalHandlerFromSanitizer, because of this code in the sigaction interceptor:
if (IsSignalHandlerFromSanitizer(signum) && ret == 0) {
if (act)
// If the user sets a signal handler, it is never cloaked, even if they
// reuse a sanitizer's signal handler.
SetSignalHandlerFromSanitizer(signum, false);
...
}
There was a problem hiding this comment.
you still need "Is" but you can set atomically
if (act) {
if (ret == 0 && SetSignalHandlerFromSanitizer(signum, false)) {
...
}
} else if (ret == 0 && IsSignalHandlerFromSanitizer) {
...
}
Note: check local ret before more expensive SetSignalHandlerFromSanitizer
vitalybuka
reviewed
Oct 10, 2025
|
|
||
| void SetSignalHandlerFromSanitizer(int signum, bool new_state) { | ||
| if (signum >= 0 && signum < MaxSignals) | ||
| signal_handler_is_from_sanitizer[signum] = new_state; |
There was a problem hiding this comment.
Should n't this be in the same file as MaybeInstallSigaction?
There was a problem hiding this comment.
This function is used in the interceptors, which are cross-platform. If I moved SetSignalHandlerFromSanitizer into sanitizer_posix_libcdep.cpp (the same file as MaybeInstallSigaction), I would need to add a lot of #if SANITIZER_POSIX into the interceptors.
There was a problem hiding this comment.
Signals are sanitizer_posix_libcdep specific
declare empty for other platforms
There was a problem hiding this comment.
#define SANITIZER_INTERCEPT_SIGNAL_AND_SIGACTION (!SI_WINDOWS && SI_NOT_FUCHSIA)
thurstond
added a commit
to thurstond/llvm-project
that referenced
this pull request
Oct 10, 2025
…mmediately return This enables follow-up work (llvm#162746), which will inspect the return value and do additional work before returning.
vitalybuka
reviewed
Oct 11, 2025
|
|
||
| const char *SanitizerToolName = "SanitizerTool"; | ||
|
|
||
| const int MaxSignals = 64; |
There was a problem hiding this comment.
Why 64?
it needs to be max signal which sanitizers install itself
There was a problem hiding this comment.
max(
MaybeInstallSigaction(SIGSEGV, handler);
MaybeInstallSigaction(SIGBUS, handler);
MaybeInstallSigaction(SIGABRT, handler);
MaybeInstallSigaction(SIGFPE, handler);
MaybeInstallSigaction(SIGILL, handler);
MaybeInstallSigaction(SIGTRAP, handler);
) + 1
There was a problem hiding this comment.
The number of signals is technically dynamic, but 64 is commonly the max (https://www.chromium.org/chromium-os/developer-library/reference/linux-constants/signals/).
The constants SIGFPE, SIGBUS, SIGTRAP, SIGILL and SIGSEGV vary between platforms (https://venam.net/blog/unix/2016/10/21/unix-signals.html).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
If set, signal/sigaction will pretend that the sanitizers did not preinstall any signal handlers. If a user successfully installs a signal handler, it will not be cloaked.
The flag is currently off by default, which means this patch should not affect the behavior of any sanitizers.
This can be useful in an ecosystem where:
The flag is in sanitizer_common, though it is currently only supported in ASan, LSan, MSan, TSan and UBSan.