search

jbevain / cecil

Cecil is a library to inspect, modify and create .NET programs and libraries.
MIT License
2.72k stars 620 forks source link

Defend against cyclical Type Forwarders? #706

Open KirillOsenkov opened 3 years ago

KirillOsenkov commented 3 years ago

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: https://github.com/jbevain/cecil/blob/6edabe0525a17bda3613c2f4e5b28db568cb2c21/Mono.Cecil/ModuleDefinition.cs#L741

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 commented 3 years ago

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

KirillOsenkov commented 2 years ago

@jbevain here's an attempt to break the circularity of type forwarders: https://github.com/jbevain/cecil/pull/806

KirillOsenkov commented 11 months ago

@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 commented 11 months ago

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

KirillOsenkov commented 11 months ago

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