null-check the redirect context before using. #65910

VSadov · 2022-02-25T23:09:30Z

In rare cases (typically on x86) the redirect context can be NULL. It is supposed to be preallocated before redirecting a thread, but we could be short on memory at the time when we tried to allocate.

The condition should not be fatal. It should just cause the current redirection attempt to fail and cause a retry.
On the next attempt we may be able to allocate or stop the thread via other mechanisms.
We used to indirectly rely on EEGetThreadContext to fail when the context is NULL. Now the first use is SetXStateFeaturesMask which crashes with NRE.

We should just check pCtx for NULL before using it.

jkotas · 2022-02-26T07:25:06Z

src/coreclr/vm/threadsuspend.cpp

    }

+    // We may not have a preallocated context. Could be short on memory when we tried to preallocate.


The crash was hit in the CI by a lot of tests. It sounds very unlikely we would be running out of memory that often.

Isn't the actual problem a race condition somewhere?

My concern as well. The situation should not be fatal, but should not be common either. These tests do not look like something that should run close to OOM.
I am not sure if this was always happening and handled or it is something new.

I could not reproduce this on purpose. I've spent quite some time trying. Maybe having fewer resources on lab machines is a factor.
I think the fix makes sense one way or another, but why we need it more than I'd expect is still a question.

I was unable to reproduce this so far. Did we see this on anything other than Windows7/x86?

I think we have seen it on Win7/x86 only

We call InitializeContext with 0 for context buffer size to get the actual buffer size. The expected result is to get ERROR_INSUFFICIENT_BUFFER and to get the size required.
Rarely, for unknown reasons we get a different error. We have disabled an assert because of this.

runtime/src/coreclr/vm/threadsuspend.cpp

Line 1986 in 1857d04

// The following assert is valid, but gets triggered in some Win7 runs with no impact on functionality.

I wonder if in such case we also get invaid size (like 0) and thus the allocation fails.

The spec apparently mentions that:

If the function fails with an error other than ERROR_INSUFFICIENT_BUFFER, the contents of ContextLength are undefined.

I just do not know what can possibly go wrong with a call like InitializeContext(NULL, context, NULL, &contextSize); - that is the same use as in the example.

I think we should not even try to allocate when InitializeContext returns unexpected results.

Perhaps that is the root cause, but the reason why the call may fail on Win7/x86 is still unclear.

jkotas

LGTM

VSadov · 2022-03-01T06:51:42Z

Thanks!!

* null-check the redirect context before using. * tweak the comment * do not allocate context if InitializeContext has unexpected results.

…#66120) * Use CopyContext to restore saved context on X86 (#65490) * Use CopyContext to restore saved context on X86 * PR feedback * more PR feedback * Do not copy XState other than AVX when redirecting for GC stress (#65825) * Do not copy XState other than AVX * #if defined(TARGET_X86) || defined(TARGET_AMD64) * mask XState unconditionally * Ensure XSTATE_MASK_AVX is set before calling EEGetThreadContext * redundant supportsAVX, more clear comment * PR feedback * null-check the redirect context before using. (#65910) * null-check the redirect context before using. * tweak the comment * do not allocate context if InitializeContext has unexpected results. * EE Suspension on x86 should use RtlRestoreContext when available (#65878) * RestoreContextSimulated * probe for RtlRestoreContext * ntdll.dll * restore self-trap sequence * PR feedback * Clarify CopyContext in RedirectedHandledJITCaseExceptionFilter * simpler indentation. * restore last error on the legacy path. * Update src/coreclr/vm/threads.h Co-authored-by: Dan Moseley <[email protected]> Co-authored-by: Dan Moseley <[email protected]>

* Use CopyContext to restore saved context on X86 (#65490) * Use CopyContext to restore saved context on X86 * PR feedback * more PR feedback * Do not copy XState other than AVX when redirecting for GC stress (#65825) * Do not copy XState other than AVX * #if defined(TARGET_X86) || defined(TARGET_AMD64) * mask XState unconditionally * Ensure XSTATE_MASK_AVX is set before calling EEGetThreadContext * redundant supportsAVX, more clear comment * PR feedback * null-check the redirect context before using. (#65910) * null-check the redirect context before using. * tweak the comment * do not allocate context if InitializeContext has unexpected results. * EE Suspension on x86 should use RtlRestoreContext when available (#65878) * RestoreContextSimulated * probe for RtlRestoreContext * ntdll.dll * restore self-trap sequence * PR feedback * Clarify CopyContext in RedirectedHandledJITCaseExceptionFilter * simpler indentation. * restore last error on the legacy path. * Update src/coreclr/vm/threads.h Co-authored-by: Dan Moseley <[email protected]> Co-authored-by: Vladimir Sadov <[email protected]> Co-authored-by: Dan Moseley <[email protected]>

dotnet-issue-labeler bot added the area-VM-coreclr label Feb 25, 2022

ghost assigned VSadov Feb 25, 2022

jkotas reviewed Feb 26, 2022

View reviewed changes

VSadov added 2 commits February 28, 2022 11:44

null-check the redirect context before using.

615b4cd

tweak the comment

5afcb94

VSadov force-pushed the ctxNull branch from 9d03976 to 5afcb94 Compare February 28, 2022 19:45

VSadov mentioned this pull request Feb 28, 2022

Fatal error. Internal CLR error. (0x80131506) #65890

Closed

do not allocate context if InitializeContext has unexpected results.

473ff9f

jkotas approved these changes Feb 28, 2022

View reviewed changes

VSadov merged commit 5130645 into dotnet:main Mar 1, 2022

VSadov deleted the ctxNull branch March 1, 2022 06:51

jkotas mentioned this pull request Mar 1, 2022

Make DllImportGenerator emit a GeneratedCodeAttribute on our stubs #65868

Merged

VSadov added a commit to VSadov/runtime that referenced this pull request Mar 2, 2022

null-check the redirect context before using. (dotnet#65910)

dd8a4f2

* null-check the redirect context before using. * tweak the comment * do not allocate context if InitializeContext has unexpected results.

ghost locked as resolved and limited conversation to collaborators Mar 31, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

null-check the redirect context before using. #65910

null-check the redirect context before using. #65910

VSadov commented Feb 25, 2022 •

edited

Loading

jkotas Feb 26, 2022

VSadov Feb 26, 2022

VSadov Feb 28, 2022

jkotas Feb 28, 2022

VSadov Feb 28, 2022 •

edited

Loading

VSadov Feb 28, 2022

VSadov Feb 28, 2022

VSadov Feb 28, 2022 •

edited

Loading

VSadov Feb 28, 2022

jkotas left a comment

VSadov commented Mar 1, 2022

		}

		// We may not have a preallocated context. Could be short on memory when we tried to preallocate.

null-check the redirect context before using. #65910

null-check the redirect context before using. #65910

Conversation

VSadov commented Feb 25, 2022 • edited Loading

jkotas Feb 26, 2022

Choose a reason for hiding this comment

VSadov Feb 26, 2022

Choose a reason for hiding this comment

VSadov Feb 28, 2022

Choose a reason for hiding this comment

jkotas Feb 28, 2022

Choose a reason for hiding this comment

VSadov Feb 28, 2022 • edited Loading

Choose a reason for hiding this comment

VSadov Feb 28, 2022

Choose a reason for hiding this comment

VSadov Feb 28, 2022

Choose a reason for hiding this comment

VSadov Feb 28, 2022 • edited Loading

Choose a reason for hiding this comment

VSadov Feb 28, 2022

Choose a reason for hiding this comment

jkotas left a comment

Choose a reason for hiding this comment

VSadov commented Mar 1, 2022

VSadov commented Feb 25, 2022 •

edited

Loading

VSadov Feb 28, 2022 •

edited

Loading

VSadov Feb 28, 2022 •

edited

Loading