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.

Sign up for GitHub

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

Jump to bottom

Defend against cyclical Type Forwarders? #706

Open
KirillOsenkov opened this issue Nov 28, 2020 · 5 comments · May be fixed by #806
Open

Defend against cyclical Type Forwarders? #706

KirillOsenkov opened this issue Nov 28, 2020 · 5 comments · May be fixed by #806

Comments

@KirillOsenkov
Copy link

KirillOsenkov commented Nov 28, 2020 •
edited

This is not necessarily a bug in Cecil per se, but something to think about to protect consumers against cyclical type forwarders.

I'm filing this primarily so that there's a record of a lengthy investigation where the symptom was a StackOverflow in Cecil with this repeating stack:

Mono.Cecil.ExportedType.Resolve <--
Mono.Cecil.MetadataResolver.GetType
Mono.Cecil.MetadataResolver.Resolve
Mono.Cecil.ModuleDefinition.Resolve
Mono.Cecil.ExportedType.Resolve  <--
Mono.Cecil.MetadataResolver.GetType
Mono.Cecil.MetadataResolver.Resolve
Mono.Cecil.ModuleDefinition.Resolve
Mono.Cecil.TypeReference.Resolve
Mono.Cecil.TypeReference.ResolveDefinition
Mono.Cecil.MemberReference.Resolve

I'm attaching two .dlls where the type System.Lazy``2 is type forwarded to the other .dll.

CecilRepro.zip

Of course it's a bug where the custom assembly resolver I have resolved this combination of .dlls - normally there shouldn't be a cycle.

But I thought of how to defend against a StackOverflow here and there doesn't seem to be a way to defend against reentrancy on the consumer side.

Ideally I'd place a circuit breaker here:

cecil/Mono.Cecil/ModuleDefinition.cs

Line 741 in 6edabe0

return MetadataResolver.Resolve (type);

Remember the type I'm about to start resolving by adding it to the list, and remove it at the end of the method in a finally block. At the beginning of the method, check if the type is already in the list, and if so, throw an InvalidOperationException("recursive type references") or something.

I know it's kind of an obscure edge case but something to think about.
@KirillOsenkov
Copy link
Author

If you think this is worth pursuing I can send a PR.

@KirillOsenkov KirillOsenkov mentioned this issue Nov 28, 2020
Open
KirillOsenkov added a commit to KirillOsenkov/cecil that referenced this issue Oct 29, 2021
@KirillOsenkov
Protect against circular type forwarders
655d831 
If we have two assemblies with type forwarders that point to each other, we enter an infinite loop and a stack overflow.

This breaks the cycle by detecting reentrancy.

Fixes jbevain#706
@KirillOsenkov KirillOsenkov linked a pull request Oct 29, 2021 that will close this issue
Open
@KirillOsenkov
Copy link
Author

@jbevain here's an attempt to break the circularity of type forwarders:
#806

@brettsam brettsam mentioned this issue Sep 21, 2022
Merged
@KirillOsenkov
Copy link
Author

@jbevain we need to fix this, I ran into this again :( Every time I investigate this I forget I already saw this and it takes me an hour or so.

This time a loop for System.Security.Cryptography.ProtectedData:
System.Security.dll -> {System.Security.Cryptography.ProtectedData, Version=0.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a}
{System.Security.Cryptography.ProtectedData, Version=4.0.5.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a}
-> {System.Security, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a}

@KirillOsenkov
Copy link
Author

Another cycle between System.Configuration.dll and System.Configuration.ConfigurationManager.dll

KirillOsenkov added a commit to KirillOsenkov/cecil that referenced this issue Oct 15, 2023
@KirillOsenkov
Protect against circular type forwarders
5ad5672 
If we have two assemblies with type forwarders that point to each other, we enter an infinite loop and a stack overflow.

This breaks the cycle by detecting reentrancy.

Fixes jbevain#706
@KirillOsenkov
Copy link
Author

I revived #806 and added a test. Should be ready to go!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Protect against circular type forwarders KirillOsenkov/cecil
1 participant
@KirillOsenkov