When invoking class constructor ensure class is initialized - #40293

Merged
davidwrighton merged 4 commits into
dotnet:masterfrom
davidwrighton:RunCctorInReflection
Aug 5, 2020
Merged

When invoking class constructor ensure class is initialized#40293
davidwrighton merged 4 commits into
dotnet:masterfrom
davidwrighton:RunCctorInReflection

Conversation

@davidwrighton

@davidwrightondavidwrighton commented Aug 3, 2020

Copy link
Copy Markdown
Member

When invoking the class constructor method via reflection invoke, ensure that the class constructor is run via the standard run class constructor pathway before manually invoking the class constructor

This ensure that all of the various data structures associated with the class constructor are initialized such as valuetype statics.

Fixes#1748

@Dotnet-GitSync-Bot

Dotnet-GitSync-Bot commented Aug 3, 2020

Copy link
Copy Markdown
Collaborator

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.

danmoseley
danmoseley reviewed Aug 3, 2020
View reviewed changes
Comment threadsrc/libraries/System.Reflection/tests/ConstructorInfoTests.cs
[Fact]
public void Invoke_StaticConstructorMultipleTimes()
{
ConstructorInfo[] constructors = GetConstructors(typeof(ClassWithStaticConstructorThatIsCalledMultipleTimesViaReflection));

@danmoseleydanmoseleyAug 3, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

BTW this test library doesn't run against .NET Framework, so you'd have to paste this into a test program if you wanted to be sure NETFX behavior was the same.

@jkotas

jkotas commented Aug 3, 2020

Copy link
Copy Markdown
Member

Does this create easy path to mutate read-only statics (by accident)?

We have disallowed setting of read-only fields via reflection in .NET Core. This is a more subtle variant of the same. I think it may be better to fix this by disallowing static cctor invocation via reflection.

@jkotas

jkotas commented Aug 3, 2020

Copy link
Copy Markdown
Member

I think it may be better to fix this by disallowing static cctor invocation via reflection.

Or to allow it just once.

@davidwrighton

davidwrighton commented Aug 3, 2020

Copy link
Copy Markdown
MemberAuthor

This creates all sorts of questionable paths. But they aren't new. @danmosemsft I've verified that this is the behavior of Netfx 4.8.

I'd be pleased to convert an attempt invoke the static ctor via this mechanism into a simple call to RunClassConstructor instead of actually, you know, repeatedly invoking the class constructor directly. The current checked in scheme is clearly not safe, as it doesn't behave reliably, and can skip critical aspects of cctor invocation.

@jkotas

jkotas commented Aug 3, 2020

Copy link
Copy Markdown
Member

I'd be pleased to convert an attempt invoke the static ctor via this mechanism into a simple call to RunClassConstructor

+1

jkotas
jkotas reviewed Aug 4, 2020
View reviewed changes
Comment threadsrc/coreclr/src/System.Private.CoreLib/src/System/Reflection/RuntimeConstructorInfo.cs Outdated
{
// Run the class constructor through the class constructor mechanism instead of the Invoke path.
// This avoids allowing mutation of readonly static fields, and initializes the type correctly.
RuntimeHelpers.RunClassConstructor(DeclaringType!.TypeHandle);

@jkotasjkotasAug 4, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can we get here for module constructors? I believe DeclaringType is going to be null for them.

@davidwrightondavidwrightonAug 4, 2020

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Sigh, we can. I'll update the code to handle them by running the module constructor api.

jkotas
jkotas approved these changes Aug 4, 2020
View reviewed changes

@jkotasjkotas left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM!

@jkotasjkotas changed the title When invoking class constructor ensure class is intializedWhen invoking class constructor ensure class is initializedAug 4, 2020
@davidwrighton
davidwrighton merged commit dd05d47 into dotnet:masterAug 5, 2020
Jacksondr5 pushed a commit to Jacksondr5/runtime that referenced this pull request Aug 10, 2020
@davidwrighton@Jacksondr5
When invoking class constructor ensure class is initialized (dotnet#4…
293c9a0
…0293)
When invoking the class constructor method via reflection invoke, ensure that the class constructor is run via the standard run class constructor pathway instead of running it explicitly. Do the same for module constructors.
@karelzkarelz added this to the 5.0.0 milestone Aug 18, 2020
@ghostghost locked as resolved and limited conversation to collaborators Dec 7, 2020
@davidwrighton
davidwrighton deleted the RunCctorInReflection branch April 20, 2021 17:43
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@jkotasjkotasjkotas approved these changes
+1 more reviewer
@danmoseleydanmoseleydanmoseley left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area-System.Reflection

Projects

None yet

Milestone

5.0.0

Development

Successfully merging this pull request may close these issues.

static constructors throw NRE if having struct fields and invoked via TypeInitializer

5 participants

@davidwrighton@Dotnet-GitSync-Bot@jkotas@danmoseley@karelz
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all 
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '

Skip to content
When invoking class constructor ensure class is initialized - #40293
Merged
davidwrighton merged 4 commits into
dotnet:masterfrom 
davidwrighton:RunCctorInReflection
Aug 5, 2020
Merged
When invoking class constructor ensure class is initialized#40293
davidwrighton merged 4 commits into
dotnet:masterfrom 
davidwrighton:RunCctorInReflection
Conversation
@davidwrighton
@davidwrightondavidwrighton
commented
Aug 3, 2020

Copy link


Copy Markdown

Member
When invoking the class constructor method via reflection invoke, ensure that the class constructor is run via the standard run class constructor pathway before manually invoking the class constructor
This ensure that all of the various data structures associated with the class constructor are initialized such as valuetype statics.
Fixes#1748
@Dotnet-GitSync-Bot
Dotnet-GitSync-Bot
commented
Aug 3, 2020

Copy link


Copy Markdown

Collaborator
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.
danmoseley
danmoseley
reviewed
Aug 3, 2020
View reviewed changes
Comment threadsrc/libraries/System.Reflection/tests/ConstructorInfoTests.cs
 [Fact]
 public void Invoke_StaticConstructorMultipleTimes()
 {
 ConstructorInfo[] constructors = GetConstructors(typeof(ClassWithStaticConstructorThatIsCalledMultipleTimesViaReflection));
@danmoseleydanmoseleyAug 3, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

BTW this test library doesn't run against .NET Framework, so you'd have to paste this into a test program if you wanted to be sure NETFX behavior was the same.
@jkotas
jkotas
commented
Aug 3, 2020

Copy link


Copy Markdown

Member
Does this create easy path to mutate read-only statics (by accident)?
We have disallowed setting of read-only fields via reflection in .NET Core. This is a more subtle variant of the same. I think it may be better to fix this by disallowing static cctor invocation via reflection.
@jkotas
jkotas
commented
Aug 3, 2020

Copy link


Copy Markdown

Member
I think it may be better to fix this by disallowing static cctor invocation via reflection.
Or to allow it just once.
@davidwrighton
davidwrighton
commented
Aug 3, 2020

Copy link


Copy Markdown

MemberAuthor
This creates all sorts of questionable paths. But they aren't new. @danmosemsft I've verified that this is the behavior of Netfx 4.8.
I'd be pleased to convert an attempt invoke the static ctor via this mechanism into a simple call to RunClassConstructor instead of actually, you know, repeatedly invoking the class constructor directly. The current checked in scheme is clearly not safe, as it doesn't behave reliably, and can skip critical aspects of cctor invocation.
@jkotas
jkotas
commented
Aug 3, 2020

Copy link


Copy Markdown

Member
I'd be pleased to convert an attempt invoke the static ctor via this mechanism into a simple call to RunClassConstructor
+1
jkotas
jkotas
reviewed
Aug 4, 2020
View reviewed changes
Comment threadsrc/coreclr/src/System.Private.CoreLib/src/System/Reflection/RuntimeConstructorInfo.cs
Outdated

 {
 // Run the class constructor through the class constructor mechanism instead of the Invoke path.
 // This avoids allowing mutation of readonly static fields, and initializes the type correctly.
 RuntimeHelpers.RunClassConstructor(DeclaringType!.TypeHandle);
@jkotasjkotasAug 4, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Can we get here for module constructors? I believe DeclaringType is going to be null for them.
@davidwrightondavidwrightonAug 4, 2020

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Sigh, we can. I'll update the code to handle them by running the module constructor api.
jkotas
jkotas
approved these changes
Aug 4, 2020
View reviewed changes
@jkotasjkotas
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

LGTM!
@jkotasjkotas
changed the title
When invoking class constructor ensure class is intializedWhen invoking class constructor ensure class is initializedAug 4, 2020
@davidwrighton
davidwrighton
merged commit dd05d47
into
dotnet:masterAug 5, 2020
Jacksondr5 pushed a commit
to Jacksondr5/runtime
that referenced
this pull request
Aug 10, 2020
@davidwrighton@Jacksondr5
When invoking class constructor ensure class is initialized (dotnet#4…
293c9a0
…0293)
When invoking the class constructor method via reflection invoke, ensure that the class constructor is run via the standard run class constructor pathway instead of running it explicitly. Do the same for module constructors.
@karelzkarelz
added this to the 5.0.0 milestone
Aug 18, 2020
@ghostghost
locked as resolved and limited conversation to collaborators
Dec 7, 2020
@davidwrighton
davidwrighton
deleted the

RunCctorInReflection

branch
April 20, 2021 17:43
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@jkotasjkotasjkotas approved these changes

+1 more reviewer

@danmoseleydanmoseleydanmoseley left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

area-System.Reflection

Projects


None yet


Milestone

5.0.0

Development

Successfully merging this pull request may close these issues.

static constructors throw NRE if having struct fields and invoked via TypeInitializer


5 participants

@davidwrighton@Dotnet-GitSync-Bot@jkotas@danmoseley@karelz

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix");
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
When invoking class constructor ensure class is initialized - #40293
Merged
davidwrighton merged 4 commits into
dotnet:masterfrom 
davidwrighton:RunCctorInReflection
Aug 5, 2020
Merged
When invoking class constructor ensure class is initialized#40293
davidwrighton merged 4 commits into
dotnet:masterfrom 
davidwrighton:RunCctorInReflection
Conversation
@davidwrighton
@davidwrightondavidwrighton
commented
Aug 3, 2020

Copy link


Copy Markdown

Member
When invoking the class constructor method via reflection invoke, ensure that the class constructor is run via the standard run class constructor pathway before manually invoking the class constructor
This ensure that all of the various data structures associated with the class constructor are initialized such as valuetype statics.
Fixes#1748
@Dotnet-GitSync-Bot
Dotnet-GitSync-Bot
commented
Aug 3, 2020

Copy link


Copy Markdown

Collaborator
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.
danmoseley
danmoseley
reviewed
Aug 3, 2020
View reviewed changes
Comment threadsrc/libraries/System.Reflection/tests/ConstructorInfoTests.cs
 [Fact]
 public void Invoke_StaticConstructorMultipleTimes()
 {
 ConstructorInfo[] constructors = GetConstructors(typeof(ClassWithStaticConstructorThatIsCalledMultipleTimesViaReflection));
@danmoseleydanmoseleyAug 3, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

BTW this test library doesn't run against .NET Framework, so you'd have to paste this into a test program if you wanted to be sure NETFX behavior was the same.
@jkotas
jkotas
commented
Aug 3, 2020

Copy link


Copy Markdown

Member
Does this create easy path to mutate read-only statics (by accident)?
We have disallowed setting of read-only fields via reflection in .NET Core. This is a more subtle variant of the same. I think it may be better to fix this by disallowing static cctor invocation via reflection.
@jkotas
jkotas
commented
Aug 3, 2020

Copy link


Copy Markdown

Member
I think it may be better to fix this by disallowing static cctor invocation via reflection.
Or to allow it just once.
@davidwrighton
davidwrighton
commented
Aug 3, 2020

Copy link


Copy Markdown

MemberAuthor
This creates all sorts of questionable paths. But they aren't new. @danmosemsft I've verified that this is the behavior of Netfx 4.8.
I'd be pleased to convert an attempt invoke the static ctor via this mechanism into a simple call to RunClassConstructor instead of actually, you know, repeatedly invoking the class constructor directly. The current checked in scheme is clearly not safe, as it doesn't behave reliably, and can skip critical aspects of cctor invocation.
@jkotas
jkotas
commented
Aug 3, 2020

Copy link


Copy Markdown

Member
I'd be pleased to convert an attempt invoke the static ctor via this mechanism into a simple call to RunClassConstructor
+1
jkotas
jkotas
reviewed
Aug 4, 2020
View reviewed changes
Comment threadsrc/coreclr/src/System.Private.CoreLib/src/System/Reflection/RuntimeConstructorInfo.cs
Outdated

 {
 // Run the class constructor through the class constructor mechanism instead of the Invoke path.
 // This avoids allowing mutation of readonly static fields, and initializes the type correctly.
 RuntimeHelpers.RunClassConstructor(DeclaringType!.TypeHandle);
@jkotasjkotasAug 4, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Can we get here for module constructors? I believe DeclaringType is going to be null for them.
@davidwrightondavidwrightonAug 4, 2020

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Sigh, we can. I'll update the code to handle them by running the module constructor api.
jkotas
jkotas
approved these changes
Aug 4, 2020
View reviewed changes
@jkotasjkotas
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

LGTM!
@jkotasjkotas
changed the title
When invoking class constructor ensure class is intializedWhen invoking class constructor ensure class is initializedAug 4, 2020
@davidwrighton
davidwrighton
merged commit dd05d47
into
dotnet:masterAug 5, 2020
Jacksondr5 pushed a commit
to Jacksondr5/runtime
that referenced
this pull request
Aug 10, 2020
@davidwrighton@Jacksondr5
When invoking class constructor ensure class is initialized (dotnet#4…
293c9a0
…0293)
When invoking the class constructor method via reflection invoke, ensure that the class constructor is run via the standard run class constructor pathway instead of running it explicitly. Do the same for module constructors.
@karelzkarelz
added this to the 5.0.0 milestone
Aug 18, 2020
@ghostghost
locked as resolved and limited conversation to collaborators
Dec 7, 2020
@davidwrighton
davidwrighton
deleted the

RunCctorInReflection

branch
April 20, 2021 17:43
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@jkotasjkotasjkotas approved these changes

+1 more reviewer

@danmoseleydanmoseleydanmoseley left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

area-System.Reflection

Projects


None yet


Milestone

5.0.0

Development

Successfully merging this pull request may close these issues.

static constructors throw NRE if having struct fields and invoked via TypeInitializer


5 participants

@davidwrighton@Dotnet-GitSync-Bot@jkotas@danmoseley@karelz

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms");
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
When invoking class constructor ensure class is initialized - #40293
Merged
davidwrighton merged 4 commits into
dotnet:masterfrom 
davidwrighton:RunCctorInReflection
Aug 5, 2020
Merged
When invoking class constructor ensure class is initialized#40293
davidwrighton merged 4 commits into
dotnet:masterfrom 
davidwrighton:RunCctorInReflection
Conversation
@davidwrighton
@davidwrightondavidwrighton
commented
Aug 3, 2020

Copy link


Copy Markdown

Member
When invoking the class constructor method via reflection invoke, ensure that the class constructor is run via the standard run class constructor pathway before manually invoking the class constructor
This ensure that all of the various data structures associated with the class constructor are initialized such as valuetype statics.
Fixes#1748
@Dotnet-GitSync-Bot
Dotnet-GitSync-Bot
commented
Aug 3, 2020

Copy link


Copy Markdown

Collaborator
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.
danmoseley
danmoseley
reviewed
Aug 3, 2020
View reviewed changes
Comment threadsrc/libraries/System.Reflection/tests/ConstructorInfoTests.cs
 [Fact]
 public void Invoke_StaticConstructorMultipleTimes()
 {
 ConstructorInfo[] constructors = GetConstructors(typeof(ClassWithStaticConstructorThatIsCalledMultipleTimesViaReflection));
@danmoseleydanmoseleyAug 3, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

BTW this test library doesn't run against .NET Framework, so you'd have to paste this into a test program if you wanted to be sure NETFX behavior was the same.
@jkotas
jkotas
commented
Aug 3, 2020

Copy link


Copy Markdown

Member
Does this create easy path to mutate read-only statics (by accident)?
We have disallowed setting of read-only fields via reflection in .NET Core. This is a more subtle variant of the same. I think it may be better to fix this by disallowing static cctor invocation via reflection.
@jkotas
jkotas
commented
Aug 3, 2020

Copy link


Copy Markdown

Member
I think it may be better to fix this by disallowing static cctor invocation via reflection.
Or to allow it just once.
@davidwrighton
davidwrighton
commented
Aug 3, 2020

Copy link


Copy Markdown

MemberAuthor
This creates all sorts of questionable paths. But they aren't new. @danmosemsft I've verified that this is the behavior of Netfx 4.8.
I'd be pleased to convert an attempt invoke the static ctor via this mechanism into a simple call to RunClassConstructor instead of actually, you know, repeatedly invoking the class constructor directly. The current checked in scheme is clearly not safe, as it doesn't behave reliably, and can skip critical aspects of cctor invocation.
@jkotas
jkotas
commented
Aug 3, 2020

Copy link


Copy Markdown

Member
I'd be pleased to convert an attempt invoke the static ctor via this mechanism into a simple call to RunClassConstructor
+1
jkotas
jkotas
reviewed
Aug 4, 2020
View reviewed changes
Comment threadsrc/coreclr/src/System.Private.CoreLib/src/System/Reflection/RuntimeConstructorInfo.cs
Outdated

 {
 // Run the class constructor through the class constructor mechanism instead of the Invoke path.
 // This avoids allowing mutation of readonly static fields, and initializes the type correctly.
 RuntimeHelpers.RunClassConstructor(DeclaringType!.TypeHandle);
@jkotasjkotasAug 4, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Can we get here for module constructors? I believe DeclaringType is going to be null for them.
@davidwrightondavidwrightonAug 4, 2020

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Sigh, we can. I'll update the code to handle them by running the module constructor api.
jkotas
jkotas
approved these changes
Aug 4, 2020
View reviewed changes
@jkotasjkotas
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

LGTM!
@jkotasjkotas
changed the title
When invoking class constructor ensure class is intializedWhen invoking class constructor ensure class is initializedAug 4, 2020
@davidwrighton
davidwrighton
merged commit dd05d47
into
dotnet:masterAug 5, 2020
Jacksondr5 pushed a commit
to Jacksondr5/runtime
that referenced
this pull request
Aug 10, 2020
@davidwrighton@Jacksondr5
When invoking class constructor ensure class is initialized (dotnet#4…
293c9a0
…0293)
When invoking the class constructor method via reflection invoke, ensure that the class constructor is run via the standard run class constructor pathway instead of running it explicitly. Do the same for module constructors.
@karelzkarelz
added this to the 5.0.0 milestone
Aug 18, 2020
@ghostghost
locked as resolved and limited conversation to collaborators
Dec 7, 2020
@davidwrighton
davidwrighton
deleted the

RunCctorInReflection

branch
April 20, 2021 17:43
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@jkotasjkotasjkotas approved these changes

+1 more reviewer

@danmoseleydanmoseleydanmoseley left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

area-System.Reflection

Projects


None yet


Milestone

5.0.0

Development

Successfully merging this pull request may close these issues.

static constructors throw NRE if having struct fields and invoked via TypeInitializer


5 participants

@davidwrighton@Dotnet-GitSync-Bot@jkotas@danmoseley@karelz

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links");
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '

Skip to content
When invoking class constructor ensure class is initialized - #40293
Merged
davidwrighton merged 4 commits into
dotnet:masterfrom 
davidwrighton:RunCctorInReflection
Aug 5, 2020
Merged
When invoking class constructor ensure class is initialized#40293
davidwrighton merged 4 commits into
dotnet:masterfrom 
davidwrighton:RunCctorInReflection
Conversation
@davidwrighton
@davidwrightondavidwrighton
commented
Aug 3, 2020

Copy link


Copy Markdown

Member
When invoking the class constructor method via reflection invoke, ensure that the class constructor is run via the standard run class constructor pathway before manually invoking the class constructor
This ensure that all of the various data structures associated with the class constructor are initialized such as valuetype statics.
Fixes#1748
@Dotnet-GitSync-Bot
Dotnet-GitSync-Bot
commented
Aug 3, 2020

Copy link


Copy Markdown

Collaborator
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.
danmoseley
danmoseley
reviewed
Aug 3, 2020
View reviewed changes
Comment threadsrc/libraries/System.Reflection/tests/ConstructorInfoTests.cs
 [Fact]
 public void Invoke_StaticConstructorMultipleTimes()
 {
 ConstructorInfo[] constructors = GetConstructors(typeof(ClassWithStaticConstructorThatIsCalledMultipleTimesViaReflection));
@danmoseleydanmoseleyAug 3, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

BTW this test library doesn't run against .NET Framework, so you'd have to paste this into a test program if you wanted to be sure NETFX behavior was the same.
@jkotas
jkotas
commented
Aug 3, 2020

Copy link


Copy Markdown

Member
Does this create easy path to mutate read-only statics (by accident)?
We have disallowed setting of read-only fields via reflection in .NET Core. This is a more subtle variant of the same. I think it may be better to fix this by disallowing static cctor invocation via reflection.
@jkotas
jkotas
commented
Aug 3, 2020

Copy link


Copy Markdown

Member
I think it may be better to fix this by disallowing static cctor invocation via reflection.
Or to allow it just once.
@davidwrighton
davidwrighton
commented
Aug 3, 2020

Copy link


Copy Markdown

MemberAuthor
This creates all sorts of questionable paths. But they aren't new. @danmosemsft I've verified that this is the behavior of Netfx 4.8.
I'd be pleased to convert an attempt invoke the static ctor via this mechanism into a simple call to RunClassConstructor instead of actually, you know, repeatedly invoking the class constructor directly. The current checked in scheme is clearly not safe, as it doesn't behave reliably, and can skip critical aspects of cctor invocation.
@jkotas
jkotas
commented
Aug 3, 2020

Copy link


Copy Markdown

Member
I'd be pleased to convert an attempt invoke the static ctor via this mechanism into a simple call to RunClassConstructor
+1
jkotas
jkotas
reviewed
Aug 4, 2020
View reviewed changes
Comment threadsrc/coreclr/src/System.Private.CoreLib/src/System/Reflection/RuntimeConstructorInfo.cs
Outdated

 {
 // Run the class constructor through the class constructor mechanism instead of the Invoke path.
 // This avoids allowing mutation of readonly static fields, and initializes the type correctly.
 RuntimeHelpers.RunClassConstructor(DeclaringType!.TypeHandle);
@jkotasjkotasAug 4, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Can we get here for module constructors? I believe DeclaringType is going to be null for them.
@davidwrightondavidwrightonAug 4, 2020

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Sigh, we can. I'll update the code to handle them by running the module constructor api.
jkotas
jkotas
approved these changes
Aug 4, 2020
View reviewed changes
@jkotasjkotas
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

LGTM!
@jkotasjkotas
changed the title
When invoking class constructor ensure class is intializedWhen invoking class constructor ensure class is initializedAug 4, 2020
@davidwrighton
davidwrighton
merged commit dd05d47
into
dotnet:masterAug 5, 2020
Jacksondr5 pushed a commit
to Jacksondr5/runtime
that referenced
this pull request
Aug 10, 2020
@davidwrighton@Jacksondr5
When invoking class constructor ensure class is initialized (dotnet#4…
293c9a0
…0293)
When invoking the class constructor method via reflection invoke, ensure that the class constructor is run via the standard run class constructor pathway instead of running it explicitly. Do the same for module constructors.
@karelzkarelz
added this to the 5.0.0 milestone
Aug 18, 2020
@ghostghost
locked as resolved and limited conversation to collaborators
Dec 7, 2020
@davidwrighton
davidwrighton
deleted the

RunCctorInReflection

branch
April 20, 2021 17:43
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@jkotasjkotasjkotas approved these changes

+1 more reviewer

@danmoseleydanmoseleydanmoseley left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

area-System.Reflection

Projects


None yet


Milestone

5.0.0

Development

Successfully merging this pull request may close these issues.

static constructors throw NRE if having struct fields and invoked via TypeInitializer


5 participants

@davidwrighton@Dotnet-GitSync-Bot@jkotas@danmoseley@karelz

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default");
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
When invoking class constructor ensure class is initialized - #40293
Merged
davidwrighton merged 4 commits into
dotnet:masterfrom 
davidwrighton:RunCctorInReflection
Aug 5, 2020
Merged
When invoking class constructor ensure class is initialized#40293
davidwrighton merged 4 commits into
dotnet:masterfrom 
davidwrighton:RunCctorInReflection
Conversation
@davidwrighton
@davidwrightondavidwrighton
commented
Aug 3, 2020

Copy link


Copy Markdown

Member
When invoking the class constructor method via reflection invoke, ensure that the class constructor is run via the standard run class constructor pathway before manually invoking the class constructor
This ensure that all of the various data structures associated with the class constructor are initialized such as valuetype statics.
Fixes#1748
@Dotnet-GitSync-Bot
Dotnet-GitSync-Bot
commented
Aug 3, 2020

Copy link


Copy Markdown

Collaborator
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.
danmoseley
danmoseley
reviewed
Aug 3, 2020
View reviewed changes
Comment threadsrc/libraries/System.Reflection/tests/ConstructorInfoTests.cs
 [Fact]
 public void Invoke_StaticConstructorMultipleTimes()
 {
 ConstructorInfo[] constructors = GetConstructors(typeof(ClassWithStaticConstructorThatIsCalledMultipleTimesViaReflection));
@danmoseleydanmoseleyAug 3, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

BTW this test library doesn't run against .NET Framework, so you'd have to paste this into a test program if you wanted to be sure NETFX behavior was the same.
@jkotas
jkotas
commented
Aug 3, 2020

Copy link


Copy Markdown

Member
Does this create easy path to mutate read-only statics (by accident)?
We have disallowed setting of read-only fields via reflection in .NET Core. This is a more subtle variant of the same. I think it may be better to fix this by disallowing static cctor invocation via reflection.
@jkotas
jkotas
commented
Aug 3, 2020

Copy link


Copy Markdown

Member
I think it may be better to fix this by disallowing static cctor invocation via reflection.
Or to allow it just once.
@davidwrighton
davidwrighton
commented
Aug 3, 2020

Copy link


Copy Markdown

MemberAuthor
This creates all sorts of questionable paths. But they aren't new. @danmosemsft I've verified that this is the behavior of Netfx 4.8.
I'd be pleased to convert an attempt invoke the static ctor via this mechanism into a simple call to RunClassConstructor instead of actually, you know, repeatedly invoking the class constructor directly. The current checked in scheme is clearly not safe, as it doesn't behave reliably, and can skip critical aspects of cctor invocation.
@jkotas
jkotas
commented
Aug 3, 2020

Copy link


Copy Markdown

Member
I'd be pleased to convert an attempt invoke the static ctor via this mechanism into a simple call to RunClassConstructor
+1
jkotas
jkotas
reviewed
Aug 4, 2020
View reviewed changes
Comment threadsrc/coreclr/src/System.Private.CoreLib/src/System/Reflection/RuntimeConstructorInfo.cs
Outdated

 {
 // Run the class constructor through the class constructor mechanism instead of the Invoke path.
 // This avoids allowing mutation of readonly static fields, and initializes the type correctly.
 RuntimeHelpers.RunClassConstructor(DeclaringType!.TypeHandle);
@jkotasjkotasAug 4, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Can we get here for module constructors? I believe DeclaringType is going to be null for them.
@davidwrightondavidwrightonAug 4, 2020

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Sigh, we can. I'll update the code to handle them by running the module constructor api.
jkotas
jkotas
approved these changes
Aug 4, 2020
View reviewed changes
@jkotasjkotas
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

LGTM!
@jkotasjkotas
changed the title
When invoking class constructor ensure class is intializedWhen invoking class constructor ensure class is initializedAug 4, 2020
@davidwrighton
davidwrighton
merged commit dd05d47
into
dotnet:masterAug 5, 2020
Jacksondr5 pushed a commit
to Jacksondr5/runtime
that referenced
this pull request
Aug 10, 2020
@davidwrighton@Jacksondr5
When invoking class constructor ensure class is initialized (dotnet#4…
293c9a0
…0293)
When invoking the class constructor method via reflection invoke, ensure that the class constructor is run via the standard run class constructor pathway instead of running it explicitly. Do the same for module constructors.
@karelzkarelz
added this to the 5.0.0 milestone
Aug 18, 2020
@ghostghost
locked as resolved and limited conversation to collaborators
Dec 7, 2020
@davidwrighton
davidwrighton
deleted the

RunCctorInReflection

branch
April 20, 2021 17:43
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@jkotasjkotasjkotas approved these changes

+1 more reviewer

@danmoseleydanmoseleydanmoseley left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

area-System.Reflection

Projects


None yet


Milestone

5.0.0

Development

Successfully merging this pull request may close these issues.

static constructors throw NRE if having struct fields and invoked via TypeInitializer


5 participants

@davidwrighton@Dotnet-GitSync-Bot@jkotas@danmoseley@karelz

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers");
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
When invoking class constructor ensure class is initialized - #40293
Merged
davidwrighton merged 4 commits into
dotnet:masterfrom 
davidwrighton:RunCctorInReflection
Aug 5, 2020
Merged
When invoking class constructor ensure class is initialized#40293
davidwrighton merged 4 commits into
dotnet:masterfrom 
davidwrighton:RunCctorInReflection
Conversation
@davidwrighton
@davidwrightondavidwrighton
commented
Aug 3, 2020

Copy link


Copy Markdown

Member
When invoking the class constructor method via reflection invoke, ensure that the class constructor is run via the standard run class constructor pathway before manually invoking the class constructor
This ensure that all of the various data structures associated with the class constructor are initialized such as valuetype statics.
Fixes#1748
@Dotnet-GitSync-Bot
Dotnet-GitSync-Bot
commented
Aug 3, 2020

Copy link


Copy Markdown

Collaborator
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.
danmoseley
danmoseley
reviewed
Aug 3, 2020
View reviewed changes
Comment threadsrc/libraries/System.Reflection/tests/ConstructorInfoTests.cs
 [Fact]
 public void Invoke_StaticConstructorMultipleTimes()
 {
 ConstructorInfo[] constructors = GetConstructors(typeof(ClassWithStaticConstructorThatIsCalledMultipleTimesViaReflection));
@danmoseleydanmoseleyAug 3, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

BTW this test library doesn't run against .NET Framework, so you'd have to paste this into a test program if you wanted to be sure NETFX behavior was the same.
@jkotas
jkotas
commented
Aug 3, 2020

Copy link


Copy Markdown

Member
Does this create easy path to mutate read-only statics (by accident)?
We have disallowed setting of read-only fields via reflection in .NET Core. This is a more subtle variant of the same. I think it may be better to fix this by disallowing static cctor invocation via reflection.
@jkotas
jkotas
commented
Aug 3, 2020

Copy link


Copy Markdown

Member
I think it may be better to fix this by disallowing static cctor invocation via reflection.
Or to allow it just once.
@davidwrighton
davidwrighton
commented
Aug 3, 2020

Copy link


Copy Markdown

MemberAuthor
This creates all sorts of questionable paths. But they aren't new. @danmosemsft I've verified that this is the behavior of Netfx 4.8.
I'd be pleased to convert an attempt invoke the static ctor via this mechanism into a simple call to RunClassConstructor instead of actually, you know, repeatedly invoking the class constructor directly. The current checked in scheme is clearly not safe, as it doesn't behave reliably, and can skip critical aspects of cctor invocation.
@jkotas
jkotas
commented
Aug 3, 2020

Copy link


Copy Markdown

Member
I'd be pleased to convert an attempt invoke the static ctor via this mechanism into a simple call to RunClassConstructor
+1
jkotas
jkotas
reviewed
Aug 4, 2020
View reviewed changes
Comment threadsrc/coreclr/src/System.Private.CoreLib/src/System/Reflection/RuntimeConstructorInfo.cs
Outdated

 {
 // Run the class constructor through the class constructor mechanism instead of the Invoke path.
 // This avoids allowing mutation of readonly static fields, and initializes the type correctly.
 RuntimeHelpers.RunClassConstructor(DeclaringType!.TypeHandle);
@jkotasjkotasAug 4, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Can we get here for module constructors? I believe DeclaringType is going to be null for them.
@davidwrightondavidwrightonAug 4, 2020

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Sigh, we can. I'll update the code to handle them by running the module constructor api.
jkotas
jkotas
approved these changes
Aug 4, 2020
View reviewed changes
@jkotasjkotas
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

LGTM!
@jkotasjkotas
changed the title
When invoking class constructor ensure class is intializedWhen invoking class constructor ensure class is initializedAug 4, 2020
@davidwrighton
davidwrighton
merged commit dd05d47
into
dotnet:masterAug 5, 2020
Jacksondr5 pushed a commit
to Jacksondr5/runtime
that referenced
this pull request
Aug 10, 2020
@davidwrighton@Jacksondr5
When invoking class constructor ensure class is initialized (dotnet#4…
293c9a0
…0293)
When invoking the class constructor method via reflection invoke, ensure that the class constructor is run via the standard run class constructor pathway instead of running it explicitly. Do the same for module constructors.
@karelzkarelz
added this to the 5.0.0 milestone
Aug 18, 2020
@ghostghost
locked as resolved and limited conversation to collaborators
Dec 7, 2020
@davidwrighton
davidwrighton
deleted the

RunCctorInReflection

branch
April 20, 2021 17:43
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@jkotasjkotasjkotas approved these changes

+1 more reviewer

@danmoseleydanmoseleydanmoseley left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

area-System.Reflection

Projects


None yet


Milestone

5.0.0

Development

Successfully merging this pull request may close these issues.

static constructors throw NRE if having struct fields and invoked via TypeInitializer


5 participants

@davidwrighton@Dotnet-GitSync-Bot@jkotas@danmoseley@karelz

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode");
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();

Skip to content
When invoking class constructor ensure class is initialized - #40293
Merged
davidwrighton merged 4 commits into
dotnet:masterfrom 
davidwrighton:RunCctorInReflection
Aug 5, 2020
Merged
When invoking class constructor ensure class is initialized#40293
davidwrighton merged 4 commits into
dotnet:masterfrom 
davidwrighton:RunCctorInReflection
Conversation
@davidwrighton
@davidwrightondavidwrighton
commented
Aug 3, 2020

Copy link


Copy Markdown

Member
When invoking the class constructor method via reflection invoke, ensure that the class constructor is run via the standard run class constructor pathway before manually invoking the class constructor
This ensure that all of the various data structures associated with the class constructor are initialized such as valuetype statics.
Fixes#1748
@Dotnet-GitSync-Bot
Dotnet-GitSync-Bot
commented
Aug 3, 2020

Copy link


Copy Markdown

Collaborator
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.
danmoseley
danmoseley
reviewed
Aug 3, 2020
View reviewed changes
Comment threadsrc/libraries/System.Reflection/tests/ConstructorInfoTests.cs
 [Fact]
 public void Invoke_StaticConstructorMultipleTimes()
 {
 ConstructorInfo[] constructors = GetConstructors(typeof(ClassWithStaticConstructorThatIsCalledMultipleTimesViaReflection));
@danmoseleydanmoseleyAug 3, 2020

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

BTW this test library doesn't run against .NET Framework, so you'd have to paste this into a test program if you wanted to be sure NETFX behavior was the same.
@jkotas
jkotas
commented
Aug 3, 2020

Copy link


Copy Markdown

Member
Does this create easy path to mutate read-only statics (by accident)?
We have disallowed setting of read-only fields via reflection in .NET Core. This is a more subtle variant of the same. I think it may be better to fix this by disallowing static cctor invocation via reflection.
@jkotas
jkotas
commented
Aug 3, 2020

Copy link


Copy Markdown

Member
I think it may be better to fix this by disallowing static cctor invocation via reflection.
Or to allow it just once.
@davidwrighton
davidwrighton
commented
Aug 3, 2020

Copy link


Copy Markdown

MemberAuthor
This creates all sorts of questionable paths. But they aren't new. @danmosemsft I've verified that this is the behavior of Netfx 4.8.
I'd be pleased to convert an attempt invoke the static ctor via this mechanism into a simple call to RunClassConstructor instead of actually, you know, repeatedly invoking the class constructor directly. The current checked in scheme is clearly not safe, as it doesn't behave reliably, and can skip critical aspects of cctor invocation.
@jkotas
jkotas
commented
Aug 3, 2020

Copy link


Copy Markdown

Member
I'd be pleased to convert an attempt invoke the static ctor via this mechanism into a simple call to RunClassConstructor
+1
jkotas
jkotas
reviewed
Aug 4, 2020
View reviewed changes
Comment threadsrc/coreclr/src/System.Private.CoreLib/src/System/Reflection/RuntimeConstructorInfo.cs
Outdated

 {
 // Run the class constructor through the class constructor mechanism instead of the Invoke path.
 // This avoids allowing mutation of readonly static fields, and initializes the type correctly.
 RuntimeHelpers.RunClassConstructor(DeclaringType!.TypeHandle);
@jkotasjkotasAug 4, 2020

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

Can we get here for module constructors? I believe DeclaringType is going to be null for them.
@davidwrightondavidwrightonAug 4, 2020

Copy link


Copy Markdown

MemberAuthor
Choose a reason for hiding this comment

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

Sigh, we can. I'll update the code to handle them by running the module constructor api.
jkotas
jkotas
approved these changes
Aug 4, 2020
View reviewed changes
@jkotasjkotas
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

LGTM!
@jkotasjkotas
changed the title
When invoking class constructor ensure class is intializedWhen invoking class constructor ensure class is initializedAug 4, 2020
@davidwrighton
davidwrighton
merged commit dd05d47
into
dotnet:masterAug 5, 2020
Jacksondr5 pushed a commit
to Jacksondr5/runtime
that referenced
this pull request
Aug 10, 2020
@davidwrighton@Jacksondr5
When invoking class constructor ensure class is initialized (dotnet#4…
293c9a0
…0293)
When invoking the class constructor method via reflection invoke, ensure that the class constructor is run via the standard run class constructor pathway instead of running it explicitly. Do the same for module constructors.
@karelzkarelz
added this to the 5.0.0 milestone
Aug 18, 2020
@ghostghost
locked as resolved and limited conversation to collaborators
Dec 7, 2020
@davidwrighton
davidwrighton
deleted the

RunCctorInReflection

branch
April 20, 2021 17:43
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@jkotasjkotasjkotas approved these changes

+1 more reviewer

@danmoseleydanmoseleydanmoseley left review comments
Reviewers whose approvals may not affect merge requirements

Assignees


No one assigned


Labels

area-System.Reflection

Projects


None yet


Milestone

5.0.0

Development

Successfully merging this pull request may close these issues.

static constructors throw NRE if having struct fields and invoked via TypeInitializer


5 participants

@davidwrighton@Dotnet-GitSync-Bot@jkotas@danmoseley@karelz