Fix x86 EH for UnmanagedCallersOnly methods #58796
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There is a bug in the exception handling on x86 when exception is propagated from a method marked with UnmanagedCallersOnly attribute to an immediate caller that's also managed (and calls the method via a native delegate). In this case, there is no native frame in between and the stack walker accidentally skips the InlinedCallFrame processing during first pass of exception handling. The InlinedCallFrame is expected to be processed before the related PInvoke IL stub and it doesn't happen in this case, as a native frame is required to make the stack frame iterator move to the explicit InlinedCallFrame. The fact that it is not processed results in ignoring the upper limit on the stack walk and walking the stack further than expected. This change fixes it by detecting the direct transition from UnmanagedCallersOnly to managed caller and forces the stack frame iterator to process the InlinedCallFrame.
|
I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label. |
jkotas
reviewed
Sep 8, 2021
janvorli
force-pushed
the
fix-x86-eh-unmanagedcallers
branch
from
62f65f1 to
36861ee
Compare
jkotas
reviewed
Sep 8, 2021
| @@ -3135,6 +3137,12 @@ void StackFrameIterator::PostProcessingForManagedFrames(void) | |||
| m_exInfoWalk.WalkToPosition(GetRegdisplaySP(m_crawl.pRD), (m_flags & POPFRAMES)); | |||
| #endif // ELIMINATE_FEF | |||
|
|
|||
| #ifdef TARGET_X86 | |||
| hdrInfo gcHdrInfo; | |||
| DecodeGCHdrInfo(m_crawl.codeInfo.GetGCInfoToken(), 0, &gcHdrInfo); | |||
There was a problem hiding this comment.
All other places that decode GCInfo in this file go through m_crawl.GetCodeManager(). Is it worth following the pattern?
There was a problem hiding this comment.
Good idea, but let me do that as a separate cleanup so that this change is just a fix for the bug. I will need to add a new method to the EECodeManager and then I'd use it in the exceptionhandling.cpp too where we currently also have this raw decoding.
AaronRobinsonMSFT
approved these changes
Sep 8, 2021
ghost
locked as resolved and limited conversation to collaborators
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There is a bug in the exception handling on x86 when exception is
propagated from a method marked with UnmanagedCallersOnly attribute to
an immediate caller that's also managed (and calls the method via a
native delegate). In this case, there is no native frame in between and
the stack walker accidentally skips the InlinedCallFrame processing
during first pass of exception handling. The InlinedCallFrame is expected
to be processed before the related PInvoke IL stub and it doesn't happen
in this case, as a native frame is required to make the stack frame iterator
move to the explicit InlinedCallFrame. The fact that it is not processed
results in ignoring the upper limit on the stack walk and walking the stack
further than expected.
This change fixes it by detecting the direct transition from
UnmanagedCallersOnly to managed caller and forces the stack frame
iterator to process the InlinedCallFrame.
Fixes #57915