[CoreCLR][Signal] Bump shutdown notif and crashdump before prev handler #123735
Conversation
mdh1418
requested review from
Copilot,
janvorli,
jkotas,
lateralusX,
noahfalk and
tmds
There was a problem hiding this comment.
Pull request overview
Adjusts CoreCLR’s signal chaining so shutdown notification and crash dump creation happen before invoking a previously-registered signal handler, ensuring these steps still run when the prior handler doesn’t return (notably on Android).
Changes:
- Reorders
PROCNotifyProcessShutdownandPROCCreateCrashDumpIfEnabledto run before chaining to the prior sigaction handler. - Adds an assertion to document/enforce that the “custom handler” path isn’t reached for
SIG_DFL/SIG_IGN.
|
|
||
| _ASSERTE(!IsSigDfl(action) && !IsSigIgn(action)); | ||
|
|
||
| PROCNotifyProcessShutdown(IsRunningOnAlternateStack(context)); |
There was a problem hiding this comment.
I do not think this works well when there are multiple runtimes loaded in the process that all want to handle segfaults. It can be multiple .NET runtimes (e.g. CoreCLR + NAOT, or multiple NAOT), or it can be .NET and some other runtime (e.g. Java runtime).
The expected behavior in these situations is that the given runtime will check whether the signal happened in the code that it cares about. If yes, it will handle the signal. If no, it will forward the signal to the next runtime, and so on.
With this change, I think we will shutdown our runtime instance and generate crashdump if there is segfault gracefully handled by some other runtime.
There was a problem hiding this comment.
Gracefully handled by another runtime, as in sa_sigaction/sa_handler? Wouldn't those still hit PROCNotifyProcessShutdown/PROCCreateCrashDumpIfEnabled in the original implementation? Or do they somehow return from invoke_previous_action
There was a problem hiding this comment.
If the signal is handled in some other runtime, the handler registered by that runtime would not return.
There was a problem hiding this comment.
I see, so given that we cannot tell how the other runtime will handle the signal, would it make sense then to pivot to an opt-in config switch that allows triggering a shutdown/create dump, even if the other runtime handled it gracefully?
I am not sure yet if there is a way for Android CoreCLR to not have a previously registered signal handler, so in those cases we wouldn't ever create crash dumps for signals.
There was a problem hiding this comment.
Do we know what the handlers that are registered on Android before us do?
There was a problem hiding this comment.
would it make sense then to pivot to an opt-in config switch that allows triggering a shutdown/create dump, even if the other runtime handled it gracefully?
I wonder if for such opt-in, we should just create a dump and instead of shutting down proceed to the other registered handler to be a good citizen of the Android ecosystem?
There was a problem hiding this comment.
I wonder if for such opt-in, we should just create a dump and instead of shutting down proceed to the other registered handler to be a good citizen of the Android ecosystem?
That can lead to things slowing down to crawl if the other runtime sends and handles multiple instances of the signal over a short period of time. There is no clear winner here.
I agree that this should be an opt-in.
That way AndroidSDK/AndroidAppBuilder may opt-in at build-time.
We may want to enumerate the scenarios for collecting a crash dump. For example, is collecting a crash dump of a appstore-installed app on retail device something we want to enable?
There was a problem hiding this comment.
On Mono we have this as opt-in feature and it will create the native crash report before chaining any signal handlers, but its off by default and apps opt in to enable the feature through the embedding API's (as described in the PR description). dotnet Android SDK currently opt-in to this feature, meaning that it will dump the native crash report before chaining signal handlers.
The "crash dump" that we collect on Android is mainly crash details going into logcat in text format, since that is the only way to get crash data out of retail devices in cases when using default error reporting, logcat output will be one of few artifacts uploaded from installed app on retail devices, unless app is running 3'rd party crash services that could upload additional data into their own services.
The Android signal handler is most likely invoking the Android crash daemon generating the Android crash report and tombstone and terminate the app, so it won't return, meaning there is no way for us to report additional information in a SIGSEGV scenario unless we do it before that handler gets executed.
There is AFAIK no reliable way to detect that its the Andorid crash report handler that has been registered, maybe we could identify the handler as being part of Android libraries and act based on that information, but that will probably break sooner or later, so probably not a good path forward.
Let say we have this as an opt-in as we do on Mono, meaning we can generate a crash report before chaining signals, should we still call PROCNotifyProcessShutdown or is it enough to just call PROCCreateCrashDumpIfEnabled triggering the Android specific implementation of that function?
There was a problem hiding this comment.
Maybe we could do something along these lines instead (default config value is false):
if (IsSigIgn(action))
{
...
}
else if (IsSigDfl(action))
{
....
}
else
{
bool doCrashChaining = Configuration::GetKnobBooleanValue(....);
if (doCrashChaining)
{
//Should we ignore doing PROCNotifyProcessShutdown in this scenario?
//PROCNotifyProcessShutdown(IsRunningOnAlternateStack(context));
PROCCreateCrashDumpIfEnabled(code, siginfo, context, true);
}
if (IsSaSigInfo(action))
{
_ASSERTE(action->sa_sigaction != NULL);
action->sa_sigaction(code, siginfo, context);
}
else
{
_ASSERTE(action->sa_handler != NULL);
action->sa_handler(code);
}
if (!doCrashChaining)
{
PROCNotifyProcessShutdown(IsRunningOnAlternateStack(context));
PROCCreateCrashDumpIfEnabled(code, siginfo, context, true);
}
}
There was a problem hiding this comment.
I'm seeing that PROCNotifyProcessShutdown doesn't actually handle/signal shutdown. It currently only applies to Unix and just cleans up the debugger transport and diagnostic server.
For non-Android, PROCCreateCrashDumpIfEnabled will trigger PROCCreateCrashDump which will likely terminate the process with exit().
On Android CoreCLR, there is no CreateDump (hopefully we will have one in the future), so its PROCCreateCrashDumpIfEnabled is just
runtime/src/coreclr/pal/src/thread/process.cpp
Lines 2743 to 2751 in cf05e72
is collecting a crash dump of a appstore-installed app on retail device something we want to enable?
Do other CoreCLR platforms not do so if crash dumps are enabled?
I think having parity with other platforms makes sense, but has any customer hit a crash on desktop CoreCLR and not produced a dump with DOTNET_DbgEnableMiniDump=1 because of signal handler registered out of their control?
Unless we coordinate with Android's crash reporter resolve our symbols, it feels like we should atleast allow an opt-in to generate a crash report without terminating the process, and then pass the signal along to the previously registered handler.
|
@mdh1418 it doesn't really matter what handlers Android installs, as long as you chain up to any you've captured. |
5 participants
The previously registered signal action/handler aren't guaranteed to return, so we lose out on notifying shutdown and creating a dump in those cases. Specifically, PROCCreateCrashDumpIfEnabled would be the last chance to provide the managed context for the thread that crashed.
e.g. On Android CoreCLR, it seems that, by default, signal handlers are already registered by Android's runtime (/apex/com.android.runtime/bin/linker64 + /system/lib64/libandroid_runtime.so). Whenever an unhandled synchronous fault occurs, the previously registered handler will not return back to invoke_previous_action and aborts the thread itself, so PROCCreateCrashDumpIfEnabled will not be hit.
Sigsegv behavior Android CoreCLR vs other platforms
Android CoreCLR
When intentionally writing to NULL (sigsegv) on Android CoreCLR, the previously registered signal handler goes down this path
runtime/src/coreclr/pal/src/exception/signal.cpp
Line 454 in 40e8c73
MacOS/Linux/NativeAOT(linux)
On MacOS, Linux, NativeAOT (Only checked linux at time of writing), the same intentional SIGSEGV will hit
runtime/src/coreclr/pal/src/exception/signal.cpp
Lines 431 to 448 in 40e8c73
History investigation
From a github history dive, I didn't spot anything in particular requiring the previous signal handler to be invoked before PROCNotifyProcessShutdown + PROCCreateCrashDumpIfEnabled.
PROCNotifyProcessShutdown was first introduced in 1433c3f. It doesn't seem to state a particular reason for invoking it after the previous signal handler.
PROCCreateCrashDumpIfEnabled was added to signal.cpp in 7f9bd2c because the PROCNotifyProcessShutdown didn't create a crash dump. It doesn't state any particular reason for being invoked after the previously registered signal handler, and was probably just placed next to PROCNotifyProcessShutdown.
invoke_previous_actionwas introduced in a740f65 and was refactoring while maintaining the order.Android CoreCLR behavior after swapping order
Locally, I have POC changes to emit managed callstacks in Android's PROCCreateCrashDumpIfEnabled.
Now theres a window to log managed callstacks before the original signal handler aborts and triggers a tombstone.
Android Mono behavior
Mono provides two embeddings APIs to configure signal and crash chaining
runtime/src/mono/mono/mini/driver.c
Lines 2864 to 2894 in 61d3943
runtime/src/mono/mono/mini/mini-runtime.c
Lines 3892 to 3903 in 61d3943
runtime/src/mono/mono/mini/mini-posix.c
Lines 193 to 210 in 61d3943
runtime/src/mono/mono/mini/mini-exceptions.c
Lines 2992 to 3012 in 61d3943
Alternatives
If there is any particular reason to preserve the order of sa_sigaction/sa_handler with respect to PROCNotifyProcessShutdown and PROCCreateCrashDumpIfEnabled for CoreCLR, a config knob can be added to allow Android CoreCLR to opt into the swapped ordering behavior. This may be in the form of config property key/values
runtime/src/coreclr/dlls/mscoree/exports.cpp
Lines 237 to 238 in 54ca569
clrconfigvalues. That way AndroidSDK/AndroidAppBuilder may opt-in at build-time.Given that the history of the ordering didn't reveal any problems with swapping the order, we can fallback to this behavior if the order swap causes problems down the line.
The other way around is more restrictive. Should we first introduce all the overhead to enable an opt-in/opt-out config knob, and later discover that no platforms need to invoke their previous handlers before PROCNotifyProcessShutdown/PROCCreateCrashDumpIfEnabled, it seems harder to justify removing the knob.