Skip to content
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

Fix FailFast message formatting race #56388

Merged
merged 2 commits into from
Jul 27, 2021
Merged

Fix FailFast message formatting race #56388

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0865d76
Fix FailFast message formatting race
janvorli Jul 27, 2021
132f2c6
Fix the OOM case
janvorli Jul 27, 2021
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
17 changes: 12 additions & 5 deletions src/coreclr/classlibnative/bcltype/system.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,8 @@ INT32 QCALLTYPE SystemNative::GetProcessorCount()
// managed string object buffer. This buffer is not always used, see comments in
// the method below.
WCHAR g_szFailFastBuffer[256];
WCHAR *g_pFailFastBuffer = g_szFailFastBuffer;

#define FAIL_FAST_STATIC_BUFFER_LENGTH (sizeof(g_szFailFastBuffer) / sizeof(WCHAR))

// This is the common code for FailFast processing that is wrapped by the two
Expand Down Expand Up @@ -262,9 +264,11 @@ void SystemNative::GenericFailFast(STRINGREF refMesgString, EXCEPTIONREF refExce

if (cchMessage < FAIL_FAST_STATIC_BUFFER_LENGTH)
{
pszMessage = g_szFailFastBuffer;
// The static buffer can be used only once to avoid race condition with other threads
pszMessage = (WCHAR *)InterlockedExchangeT(&g_pFailFastBuffer, NULL);
}
else

if (pszMessage == NULL)
{
// We can fail here, but we can handle the fault.
CONTRACT_VIOLATION(FaultViolation);
Expand All @@ -273,19 +277,22 @@ void SystemNative::GenericFailFast(STRINGREF refMesgString, EXCEPTIONREF refExce
{
// Truncate the message to what will fit in the static buffer.
cchMessage = FAIL_FAST_STATIC_BUFFER_LENGTH - 1;
pszMessage = g_szFailFastBuffer;
pszMessage = InterlockedExchangeT(&g_pFailFastBuffer, NULL);
}
}

if (cchMessage > 0)
memcpyNoGCRefs(pszMessage, gc.refMesgString->GetBuffer(), cchMessage * sizeof(WCHAR));
pszMessage[cchMessage] = W('\0');

if (cchMessage == 0) {
if (pszMessage == NULL) {
WszOutputDebugString(W("CLR: Managed code called FailFast, but there is not enough memory to print the supplied message.\r\n"));
}
else if (cchMessage == 0) {
WszOutputDebugString(W("CLR: Managed code called FailFast without specifying a reason.\r\n"));
}
else {
WszOutputDebugString(W("CLR: Managed code called FailFast, saying \""));
Copy link
Member

Choose a reason for hiding this comment

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

Not your change, but to be consistent with the other CLR termination messages, this would be ...called FailFast. "));. They all suffix the message after a period.

Copy link
Member

Choose a reason for hiding this comment

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

I suggest to keep it on one line (ie, space instead of \r\n). That is the existing pattern and it works well with logs and concurrent console output where the two lines can otherwise get separated.

pszMessage[cchMessage] = W('\0');
WszOutputDebugString(pszMessage);
WszOutputDebugString(W("\"\r\n"));
}
Expand Down