Skip to content

Fix detection of static readonly field re-initialization via reflection #37849

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

Merged
merged 3 commits into from
Jun 15, 2020

Conversation

jkotas
Copy link
Member

@jkotas jkotas commented Jun 13, 2020

Move the check for static readonly field re-initialization after the static constructor is triggered.

Fixes #37796

@marek-safar
Copy link
Contributor

/cc @vargaz @lambdageek this will need similar fix on Mono side

Move the check for static readonly field re-initialization after the static constructor is triggered.

Fixes dotnet#37796
Copy link
Member

@janvorli janvorli left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@lambdageek
Copy link
Member

/cc @vargaz @lambdageek this will need similar fix on Mono side

I created an issue #37903 Looks like mono doesn't have a check in SetValue for static field reinit at all

Co-authored-by: Jan Vorlicek <janvorli@microsoft.com>
@jkotas jkotas merged commit d629976 into dotnet:master Jun 15, 2020
@jkotas jkotas deleted the initonly branch June 15, 2020 22:27
sandreenko pushed a commit to sandreenko/runtime that referenced this pull request Jun 17, 2020
erozenfeld added a commit to erozenfeld/jitutils that referenced this pull request Jun 19, 2020
I added a workaround for disassembly non-determinism in dotnet#255.
See dotnet#255 for details of what causes the non-determinism.
It turns it relied on a reflection hole that allowed
init-only static fields to be modified after static constructor has
been called. That hole was fixed in dotnet/runtime#37849
so the workaround is no longer valid and causes an exception from pmi.

I don't see a way to work around the non-determinism without changing
framework code so for now I'm just reverting the workaround. Unfortunately, that
means that we can get non-deterministic disassembly for any method that inlines
System.Threading.Thread.GetCurrentProcessorId.

Fixes dotnet#271.
erozenfeld added a commit to dotnet/jitutils that referenced this pull request Jun 19, 2020
I added a workaround for disassembly non-determinism in #255.
See #255 for details of what causes the non-determinism.
It turns it relied on a reflection hole that allowed
init-only static fields to be modified after static constructor has
been called. That hole was fixed in dotnet/runtime#37849
so the workaround is no longer valid and causes an exception from pmi.

I don't see a way to work around the non-determinism without changing
framework code so for now I'm just reverting the workaround. Unfortunately, that
means that we can get non-deterministic disassembly for any method that inlines
System.Threading.Thread.GetCurrentProcessorId.

Fixes #271.
@ghost ghost locked as resolved and limited conversation to collaborators Dec 8, 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.

Reflection is able to set initonly static fields in some situation
6 participants