Skip to content

[Diagnostics] Serialize concurrent reporters in InProc Crash Reporter #130436

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
mdh1418 wants to merge 1 commit into
base: main
Choose a base branch
from
+19 −4
Open

[Diagnostics] Serialize concurrent reporters in InProc Crash Reporter #130436

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 19 additions & 4 deletions src/coreclr/debug/crashreport/inproccrashreporter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@

#include <fcntl.h>
#include <errno.h>
#include <poll.h>
#include <new>
#include <unistd.h>
#include <string.h>
Expand Down Expand Up @@ -151,7 +152,7 @@ static void CacheSysctlString(const char* sysctlName, char* buffer, size_t buffe
// just less compact for that frame.
//
// Single-instance because CreateReport is one-shot per process (guarded by
// the ``s_generating`` InterlockedCompareExchange in CreateReport).
// the ``s_generatingThreadId`` InterlockedCompareExchange64 in CreateReport).

static constexpr int MAX_MODULES_IN_TABLE = 256;

Expand Down Expand Up @@ -603,10 +604,24 @@ InProcCrashReporter::CreateReport(
int signal,
void* context)
{
static LONG s_generating = 0;
if (InterlockedCompareExchange(&s_generating, 1, 0) != 0)
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;

Comment on lines +607 to +611

@lateralusX lateralusX Jul 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@lateralusX lateralusX Jul 10, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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);
}
Comment on lines +609 to +624
}

m_reportFilePath[0] = '\0';
Expand Down