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

Jump to bottom

Fix FailFast message formatting race #56388

Merged
merged 2 commits into from
Jul 27, 2021
Merged

Fix FailFast message formatting race #56388

merged 2 commits into from
Jul 27, 2021

Conversation

janvorli
Copy link
Member

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.

Close #47096

@janvorli
Fix FailFast message formatting race
0865d76 
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.
@janvorli janvorli added this to the 6.0.0 milestone Jul 27, 2021
@janvorli janvorli requested a review from jkotas July 27, 2021 14:00
@janvorli janvorli self-assigned this Jul 27, 2021
danmoseley
danmoseley reviewed Jul 27, 2021
View reviewed changes
src/coreclr/classlibnative/bcltype/system.cpp
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.

@janvorli
Copy link
Member Author

I actually made a mistake in handling the OOM case, fixing it now.

@janvorli
Copy link
Member Author

I've fixed the issue and also made the message in the OOM case to propagate to the EEPolicy::HandleFatalError so that it is actually printed to the user. With the previous state, the message would be just printed in the debugger and the message a user would see would be empty.

@danmoseley
Copy link
Member

Just curious, in what circumstances do we end up with this "CLR: ... " fail fast message, as opposed to what I get from Environment.FailFast(...), eg.,

Process terminated. Some message here.
   at System.Environment.FailFast(System.String)
   at <Program>$.<Main>$(System.String[])

@janvorli
Copy link
Member Author

Just curious, in what circumstances do we end up with this "CLR: ... " fail fast message

This is just OutputDebugString message in the debugger. It is not printed to the console. The fixed function calls EEPolicy::HandleFatalError at the end which does the regular console printing.

@janvorli janvorli merged commit 2e10087 into dotnet:main Jul 27, 2021
thaystg added a commit to thaystg/runtime that referenced this pull request Jul 28, 2021
@thaystg
Merge remote-tracking branch 'origin/main' into improve_hot_reload_de…
1930315 
…bug_tests

* origin/main: (274 commits)
  Disable test ConnectWithCertificateForDifferentName_Throws (dotnet#56456)
  Update dependencies from https://github.com/mono/linker build 20210726.2 (dotnet#56374)
  Cleanup disabled test conditions (dotnet#56381)
  [mono] Add GC unsafe transition to mono_unhandled_exception  (dotnet#56380)
  don't fail the file extraction when we can't set last write time (dotnet#56370)
  Properly rebuild optimization data when it changes (dotnet#56397)
  Make open function calls in coreclr EINTR resilient on macOS (dotnet#56403)
  Fix dependency from EventLog to TraceSource ref (dotnet#56417)
  Fix comments in asm with JitDiffableDasm=1 (dotnet#56416)
  Catch TcpClient ctor exceptions in FtpWebRequest.CreateConnectionAsync (dotnet#56379)
  Add interop between serializer and DOMs (dotnet#56112)
  Fix type loader not recognizing overridden method (dotnet#56337)
  Prevent Segfault in EventPipe on disable (dotnet#56104)
  Update runtimeconfig.json and deps.json paths when these break past the MAX_PATH threshold  (dotnet#56224)
  Use native allocator instead of pinning when decompressing embedded PDB (dotnet#56336)
  Specify win-x64 as a valid platform in the microsoft-net-runtime-* workloads for iOS/tvOS/MacCatalyst (dotnet#56311)
  Fix FailFast message formatting race (dotnet#56388)
  Try to fix finalizer-based async tests (dotnet#56384)
  Fix MetricsEventSource tests (dotnet#56382)
  Remove invalid Castle.DynamicProxy.Internal.AbstractInvocation from ILLink descriptor files (dotnet#56392)
  ...
@janvorli janvorli mentioned this pull request Jul 28, 2021
Closed
thaystg added a commit to thaystg/runtime that referenced this pull request Jul 28, 2021
@thaystg
Merge remote-tracking branch 'origin/main' into thays_fix_js_tests
3f83a08 
* origin/main: (95 commits)
  Disable test ConnectWithCertificateForDifferentName_Throws (dotnet#56456)
  Update dependencies from https://github.com/mono/linker build 20210726.2 (dotnet#56374)
  Cleanup disabled test conditions (dotnet#56381)
  [mono] Add GC unsafe transition to mono_unhandled_exception  (dotnet#56380)
  don't fail the file extraction when we can't set last write time (dotnet#56370)
  Properly rebuild optimization data when it changes (dotnet#56397)
  Make open function calls in coreclr EINTR resilient on macOS (dotnet#56403)
  Fix dependency from EventLog to TraceSource ref (dotnet#56417)
  Fix comments in asm with JitDiffableDasm=1 (dotnet#56416)
  Catch TcpClient ctor exceptions in FtpWebRequest.CreateConnectionAsync (dotnet#56379)
  Add interop between serializer and DOMs (dotnet#56112)
  Fix type loader not recognizing overridden method (dotnet#56337)
  Prevent Segfault in EventPipe on disable (dotnet#56104)
  Update runtimeconfig.json and deps.json paths when these break past the MAX_PATH threshold  (dotnet#56224)
  Use native allocator instead of pinning when decompressing embedded PDB (dotnet#56336)
  Specify win-x64 as a valid platform in the microsoft-net-runtime-* workloads for iOS/tvOS/MacCatalyst (dotnet#56311)
  Fix FailFast message formatting race (dotnet#56388)
  Try to fix finalizer-based async tests (dotnet#56384)
  Fix MetricsEventSource tests (dotnet#56382)
  Remove invalid Castle.DynamicProxy.Internal.AbstractInvocation from ILLink descriptor files (dotnet#56392)
  ...
@ghost ghost locked as resolved and limited conversation to collaborators Aug 26, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@danmoseley danmoseley danmoseley left review comments

@stephentoub stephentoub stephentoub approved these changes

@jkotas jkotas Awaiting requested review from jkotas

Assignees

@janvorli janvorli

Labels
area-VM-coreclr
Projects
None yet
Milestone
6.0.0
Development

Successfully merging this pull request may close these issues.

Test failed: baseservices/exceptions/simple/ParallelCrashWorkerThreads/ParallelCrashWorkerThreads.sh
3 participants
@janvorli @danmoseley @stephentoub