[Diagnostics] Serialize concurrent reporters in InProc Crash Reporter#130436
[Diagnostics] Serialize concurrent reporters in InProc Crash Reporter#130436mdh1418 wants to merge 1 commit into
Conversation
…aths CreateReport() uses a CAS guard so only the first thread generates the crash report. Losing threads previously returned immediately, then continued through PROCAbort() -> abort(), tearing down the process while the winner was still mid-report and truncating it. On the common fatal-signal path this is already prevented upstream (EEPolicy::LogInfoForFatalError and the stack-overflow guard block losers before they reach the in-proc reporter). It can still be entered concurrently via direct PROCAbort paths that bypass those guards (e.g. exception-record pool exhaustion, FATAL_ASSERT, RaiseFailFastException). createdump's serialize=true path already blocks losers, but PROCCreateCrashDumpIfEnabled short-circuits into the in-proc callback and skips it, so the in-proc reporter must serialize itself. Mirror createdump: one InterlockedCompareExchange64 claims the guard and records the owner. Losers block in poll() until the winner's abort() tears the process down; a re-entrant fatal signal on the winning thread bails out instead of deadlocking. All threads are already captured in the winner's report, so there is no information loss. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: e9f8de07-6c05-4570-9f33-90bce6550391
mdh1418
force-pushed
the
inproc_crashreport_multisignal_race
branch
from
50cd39b to
a277461
Compare
|
Tagging subscribers to this area: @steveisok, @tommcdon, @dotnet/dotnet-diag |
There was a problem hiding this comment.
Pull request overview
This PR changes the CoreCLR in-proc crash reporter’s CreateReport entry guard to serialize concurrent invocations so that only one thread generates the crash report while other threads block, avoiding report truncation when multiple fatal paths race to abort the process.
Changes:
- Replace the previous one-shot
InterlockedCompareExchangeguard with a 64-bit thread-id CAS guard. - Block “losing” threads in
CreateReportinstead of returning immediately, to prevent them from continuing toPROCAbort()while the report is still being written.
| static LONGLONG s_generatingThreadId = 0; | ||
|
|
||
| // INFTIM is not defined when including pal.h; -1 is the equivalent poll() "wait forever" timeout. | ||
| const int PollWaitForever = -1; | ||
|
|
There was a problem hiding this comment.
CreateDump have a serialize flag that gets set through PROCCreateCrashDumpIfEnabled when called from a location that should serialize the threads, we should use the same flag here.
There was a problem hiding this comment.
Another thing to consider, in case we call in-proc crash reporter from invoke_previous_action and g_crash_report_before_signal_chaining has been enabled, we get called before chained signal handlers gets called. The scenario is explicitly created when you have a signal handler that is not DFL, but always tear downs the process, like calling the debugger daemon on Android, serializing the in-proc crash reporters in that case is probably OK, since no other crash handlers should appear in the chain, but if they do, then we can't serialize threads, since they need to call the chained signal handler and we might create an in-proc report for something that ends up being a handled signal. So, g_crash_report_before_signal_chaining can't be used unless the app explicitly knows that any signal handler won't handle the signal, because it will then have blocked all other threads from reaching that point and produced a "false" in-proc crash report. On Android this is normally fine, since the debugger daemon signal handler is in the list, but there is no guarantee there won't be a signal handler before that that would handle/ignore it. In scenarios like that the only reasonable way is to not using g_crash_report_before_signal_chaining and let the chained signal handler run first, but on Android that will lead to debugger daemon signal handler terminating the process before any in-proc crash reports have been generated.
| // INFTIM is not defined when including pal.h; -1 is the equivalent poll() "wait forever" timeout. | ||
| const int PollWaitForever = -1; | ||
|
|
||
| LONGLONG currentThreadId = static_cast<LONGLONG>(minipal_get_current_thread_id()); | ||
| LONGLONG previousThreadId = InterlockedCompareExchange64(&s_generatingThreadId, currentThreadId, 0); | ||
| if (previousThreadId != 0) | ||
| { | ||
| return; | ||
| if (previousThreadId == currentThreadId) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| while (true) | ||
| { | ||
| poll(nullptr, 0, PollWaitForever); | ||
| } |
CreateReport() uses a CAS guard so only the first thread generates the crash report. Losing threads previously returned immediately, then continued through PROCAbort() -> abort(), tearing down the process while the winner was still mid-report and truncating it.
On the common fatal-signal path this is already prevented upstream (EEPolicy::LogInfoForFatalError and the stack-overflow guard block losers before they reach the in-proc reporter). It can still be entered concurrently via direct PROCAbort paths that bypass those guards (e.g. exception-record pool exhaustion, FATAL_ASSERT, RaiseFailFastException). createdump's serialize=true path already blocks losers, but PROCCreateCrashDumpIfEnabled short-circuits into the in-proc callback and skips it, so the in-proc reporter must serialize itself.
Mirror createdump: one InterlockedCompareExchange64 claims the guard and records the owner. Losers block in poll() until the winner's abort() tears the process down; a re-entrant fatal signal on the winning thread bails out instead of deadlocking. All threads are already captured in the winner's report, so there is no information loss.