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.

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

Use RtlUnwind instead of RtlUnwindEx. #34188

Merged
merged 1 commit into from
Mar 27, 2020
Merged

Use RtlUnwind instead of RtlUnwindEx. #34188

merged 1 commit into from
Mar 27, 2020

Conversation

jaykrell
Copy link
Contributor

Because the formerly uninitialized scratch context is now an input.

@Dotnet-GitSync-Bot
Copy link
Collaborator

I couldn't figure out the best area label to add to this PR. Please help me learn by adding exactly one area label.

Because the formerly uninitialized scratch context is now an input.
@jkotas
Copy link
Member

jkotas commented Mar 27, 2020

Is this fixing any observable bug?

@janvorli
Copy link
Member

This change doesn't seem necessary, RtlUnwindEx fills in the passed in context with the current context using RtlCaptureContext. RtlUnwind until very recent Windows 10 was implemented in the OS literally as we do in this function.

@jaykrell
Copy link
Contributor Author

jaykrell commented Mar 27, 2020

This code used to be correct, but is no longer.
In the current Windows 10 implementation, the context parameter to RtlUnwindEx is an input, and not merely uninitialized scratch.
So this is reading uninitialized data.
longjmp for example has received this same change.

@jkotas
Copy link
Member

jkotas commented Mar 27, 2020

Ok, what does not work with the code that we have there today?

Note that this code is present in all .NET Framework and .NET Core runtimes shipped in last 15+ years.

@jaykrell
Copy link
Contributor Author

jaykrell commented Mar 27, 2020

It is not clearly deterministic.
It uses uninitialized stack.
Longjmp received the same change for the same reason.

@jaykrell
Copy link
Contributor Author

jaykrell commented Mar 27, 2020

It is also a little smaller but that isn’t the point.
It never needed to use RtlUnwindEx.
There was no advantage to that on older systems, given no already allocated context or history table.
It might as well use RtlUnwind.

Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

Ok, this looks like an improvement. Thanks

@jkotas jkotas merged commit 6c709ca into dotnet:master Mar 27, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants