From 0865d7667e8623f03cc16b426cc8ee6d385aa14a Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Tue, 27 Jul 2021 15:49:37 +0200 Subject: [PATCH 1/2] Fix FailFast message formatting race SystemNative::GenericFailFast uses a global buffer for messages shorter than certain size. When multiple threads call FailFast at the same time, they all use the same buffer, overwriting each other's message. That leads to a problem on Unix when the message is converted to UTF-8 using two calls to WideCharToMultiByte and another thread changes the message to a longer one between those two calls. So the buffer size the first call determines is no longer sufficient. --- src/coreclr/classlibnative/bcltype/system.cpp | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/src/coreclr/classlibnative/bcltype/system.cpp b/src/coreclr/classlibnative/bcltype/system.cpp index eece2220c63a91..69bdc27e2cbc27 100644 --- a/src/coreclr/classlibnative/bcltype/system.cpp +++ b/src/coreclr/classlibnative/bcltype/system.cpp @@ -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 @@ -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); @@ -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 \"")); + pszMessage[cchMessage] = W('\0'); WszOutputDebugString(pszMessage); WszOutputDebugString(W("\"\r\n")); } From 132f2c68b6a317261775e4c0a1bd5dcbb086b611 Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Tue, 27 Jul 2021 16:51:04 +0200 Subject: [PATCH 2/2] Fix the OOM case --- src/coreclr/classlibnative/bcltype/system.cpp | 37 +++++++++++-------- 1 file changed, 22 insertions(+), 15 deletions(-) diff --git a/src/coreclr/classlibnative/bcltype/system.cpp b/src/coreclr/classlibnative/bcltype/system.cpp index 69bdc27e2cbc27..bf5cd0fc75309a 100644 --- a/src/coreclr/classlibnative/bcltype/system.cpp +++ b/src/coreclr/classlibnative/bcltype/system.cpp @@ -245,7 +245,7 @@ void SystemNative::GenericFailFast(STRINGREF refMesgString, EXCEPTIONREF refExce // Another option would seem to be to implement a new frame type that // protects object references as pinned, but that seems like overkill for // just this problem. - WCHAR *pszMessage = NULL; + WCHAR *pszMessageBuffer = NULL; DWORD cchMessage = (gc.refMesgString == NULL) ? 0 : gc.refMesgString->GetStringLength(); WCHAR * errorSourceString = NULL; @@ -265,36 +265,43 @@ void SystemNative::GenericFailFast(STRINGREF refMesgString, EXCEPTIONREF refExce if (cchMessage < FAIL_FAST_STATIC_BUFFER_LENGTH) { // The static buffer can be used only once to avoid race condition with other threads - pszMessage = (WCHAR *)InterlockedExchangeT(&g_pFailFastBuffer, NULL); + pszMessageBuffer = InterlockedExchangeT(&g_pFailFastBuffer, NULL); } - if (pszMessage == NULL) + if (pszMessageBuffer == NULL) { // We can fail here, but we can handle the fault. CONTRACT_VIOLATION(FaultViolation); - pszMessage = new (nothrow) WCHAR[cchMessage + 1]; - if (pszMessage == NULL) + pszMessageBuffer = new (nothrow) WCHAR[cchMessage + 1]; + if (pszMessageBuffer == NULL) { // Truncate the message to what will fit in the static buffer. cchMessage = FAIL_FAST_STATIC_BUFFER_LENGTH - 1; - pszMessage = InterlockedExchangeT(&g_pFailFastBuffer, NULL); + pszMessageBuffer = InterlockedExchangeT(&g_pFailFastBuffer, NULL); } } - if (cchMessage > 0) - memcpyNoGCRefs(pszMessage, gc.refMesgString->GetBuffer(), cchMessage * sizeof(WCHAR)); - - if (pszMessage == NULL) { - WszOutputDebugString(W("CLR: Managed code called FailFast, but there is not enough memory to print the supplied message.\r\n")); + const WCHAR *pszMessage; + if (pszMessageBuffer != NULL) + { + if (cchMessage > 0) + memcpyNoGCRefs(pszMessageBuffer, gc.refMesgString->GetBuffer(), cchMessage * sizeof(WCHAR)); + pszMessageBuffer[cchMessage] = W('\0'); + pszMessage = pszMessageBuffer; } - else if (cchMessage == 0) { + else + { + pszMessage = W("There is not enough memory to print the supplied FailFast message."); + cchMessage = (DWORD)wcslen(pszMessage); + } + + 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 \"")); - pszMessage[cchMessage] = W('\0'); + WszOutputDebugString(W("CLR: Managed code called FailFast.\r\n")); WszOutputDebugString(pszMessage); - WszOutputDebugString(W("\"\r\n")); + WszOutputDebugString(W("\r\n")); } LPCWSTR argExceptionString = NULL;