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

Allowed null Namespace for Framework Types #430

Merged
merged 3 commits into from
Sep 1, 2020

Conversation

oliver-chime
Copy link
Contributor

@oliver-chime oliver-chime commented Aug 20, 2020

This is a fix for a critical issue we recently experienced with a .NET Core app running in a production environment where deserilization calls started returning exceptions at seemingly random intervals.

The issue began occurring without any changes to how the serializer is handled or the structure of the objects being serialized. We were not able to reproduce this issue outside of production.

Below is an example of the exceptions we were seeing:

System.NullReferenceException: Object reference not set to an instance of an object.
   at ExtendedXmlSerializer.ContentModel.Reflection.NotHappy.IsSatisfiedBy(TypeInfo parameter)
   at System.Linq.Enumerable.WhereArrayIterator`1.MoveNext()
   at System.Linq.Lookup`2.Create(IEnumerable`1 source, Func`2 keySelector, IEqualityComparer`1 comparer)
   at ExtendedXmlSerializer.ContentModel.Reflection.IdentityPartitionedTypeCandidates.TypeNamePartition.Get(KeyValuePair`2 parameter)
   at System.Linq.Enumerable.ToDictionary[TSource,TKey,TElement](IEnumerable`1 source, Func`2 keySelector, Func`2 elementSelector, IEqualityComparer`1 comparer)
   at ExtendedXmlSerializer.ContentModel.Reflection.IdentityPartitionedTypeCandidates..ctor(ISpecification`1 specification, ITypeFormatter formatter)
   at ExtendedXmlSerializer.ContentModel.Reflection.Types..ctor(IPartitionedTypeSpecification specification, IAssemblyTypePartitions partitions, ITypeIdentities identities, ITypeFormatter formatter)
   at DynamicMethod(Object[] , Scope )
   at LightInject.PerContainerLifetime.GetInstance(Func`1 createInstance, Scope scope)
   at LightInject.ServiceContainer.EmitLifetime(ServiceRegistration serviceRegistration, Action`1 emitMethod, IEmitter emitter)
   at LightInject.ServiceContainer.<>c__DisplayClass162_0.<CreateEmitMethodWrapper>b__0(IEmitter ms)
   at LightInject.ServiceContainer.EmitConstructorDependency(IEmitter emitter, Dependency dependency)
   at LightInject.ServiceContainer.EmitConstructorDependencies(ConstructionInfo constructionInfo, IEmitter emitter, Action`1 decoratorTargetEmitter)
   at LightInject.ServiceContainer.EmitNewInstanceUsingImplementingType(IEmitter emitter, ConstructionInfo constructionInfo, Action`1 decoratorTargetEmitMethod)
   at LightInject.ServiceContainer.EmitNewInstance(ServiceRegistration serviceRegistration, IEmitter emitter)
   at LightInject.ServiceContainer.CreateDynamicMethodDelegate(Action`1 serviceEmitter)
   at LightInject.ServiceContainer.<>c__DisplayClass205_0.<EmitLifetime>b__1()
   at LightInject.PerContainerLifetime.GetInstance(Func`1 createInstance, Scope scope)
   at LightInject.ServiceContainer.EmitLifetime(ServiceRegistration serviceRegistration, Action`1 emitMethod, IEmitter emitter)
   at LightInject.ServiceContainer.<>c__DisplayClass162_0.<CreateEmitMethodWrapper>b__0(IEmitter ms)
   at LightInject.ServiceContainer.EmitConstructorDependency(IEmitter emitter, Dependency dependency)
   at LightInject.ServiceContainer.EmitConstructorDependencies(ConstructionInfo constructionInfo, IEmitter emitter, Action`1 decoratorTargetEmitter)
   at LightInject.ServiceContainer.EmitNewInstanceUsingImplementingType(IEmitter emitter, ConstructionInfo constructionInfo, Action`1 decoratorTargetEmitMethod)
   at LightInject.ServiceContainer.EmitNewInstance(ServiceRegistration serviceRegistration, IEmitter emitter)
   at LightInject.ServiceContainer.CreateDynamicMethodDelegate(Action`1 serviceEmitter)
   at LightInject.ServiceContainer.<>c__DisplayClass205_0.<EmitLifetime>b__1()
   at LightInject.PerContainerLifetime.GetInstance(Func`1 createInstance, Scope scope)
   at LightInject.ServiceContainer.EmitLifetime(ServiceRegistration serviceRegistration, Action`1 emitMethod, IEmitter emitter)
   at LightInject.ServiceContainer.<>c__DisplayClass162_0.<CreateEmitMethodWrapper>b__0(IEmitter ms)
   at LightInject.ServiceContainer.EmitConstructorDependency(IEmitter emitter, Dependency dependency)
   at LightInject.ServiceContainer.EmitConstructorDependencies(ConstructionInfo constructionInfo, IEmitter emitter, Action`1 decoratorTargetEmitter)
   at LightInject.ServiceContainer.EmitNewInstanceUsingImplementingType(IEmitter emitter, ConstructionInfo constructionInfo, Action`1 decoratorTargetEmitMethod)
   at LightInject.ServiceContainer.EmitNewInstance(ServiceRegistration serviceRegistration, IEmitter emitter)
   at LightInject.ServiceContainer.CreateDynamicMethodDelegate(Action`1 serviceEmitter)
   at LightInject.ServiceContainer.<>c__DisplayClass205_0.<EmitLifetime>b__1()
   at LightInject.PerContainerLifetime.GetInstance(Func`1 createInstance, Scope scope)
   at LightInject.ServiceContainer.EmitLifetime(ServiceRegistration serviceRegistration, Action`1 emitMethod, IEmitter emitter)
   at LightInject.ServiceContainer.<>c__DisplayClass162_0.<CreateEmitMethodWrapper>b__0(IEmitter ms)
   at LightInject.ServiceContainer.EmitConstructorDependency(IEmitter emitter, Dependency dependency)
   at LightInject.ServiceContainer.EmitConstructorDependencies(ConstructionInfo constructionInfo, IEmitter emitter, Action`1 decoratorTargetEmitter)
   at LightInject.ServiceContainer.EmitNewInstanceUsingImplementingType(IEmitter emitter, ConstructionInfo constructionInfo, Action`1 decoratorTargetEmitMethod)
   at LightInject.ServiceContainer.EmitNewInstance(ServiceRegistration serviceRegistration, IEmitter emitter)
   at LightInject.ServiceContainer.CreateDynamicMethodDelegate(Action`1 serviceEmitter)
   at LightInject.ServiceContainer.<>c__DisplayClass205_0.<EmitLifetime>b__1()
   at LightInject.PerContainerLifetime.GetInstance(Func`1 createInstance, Scope scope)
   at LightInject.ServiceContainer.EmitLifetime(ServiceRegistration serviceRegistration, Action`1 emitMethod, IEmitter emitter)
   at LightInject.ServiceContainer.<>c__DisplayClass162_0.<CreateEmitMethodWrapper>b__0(IEmitter ms)
   at LightInject.ServiceContainer.EmitConstructorDependency(IEmitter emitter, Dependency dependency)
   at LightInject.ServiceContainer.EmitConstructorDependencies(ConstructionInfo constructionInfo, IEmitter emitter, Action`1 decoratorTargetEmitter)
   at LightInject.ServiceContainer.EmitNewInstanceUsingImplementingType(IEmitter emitter, ConstructionInfo constructionInfo, Action`1 decoratorTargetEmitMethod)
   at LightInject.ServiceContainer.EmitNewInstance(ServiceRegistration serviceRegistration, IEmitter emitter)
   at LightInject.ServiceContainer.CreateDynamicMethodDelegate(Action`1 serviceEmitter)
   at LightInject.ServiceContainer.<>c__DisplayClass205_0.<EmitLifetime>b__1()
   at LightInject.PerContainerLifetime.GetInstance(Func`1 createInstance, Scope scope)
   at LightInject.ServiceContainer.EmitLifetime(ServiceRegistration serviceRegistration, Action`1 emitMethod, IEmitter emitter)
   at LightInject.ServiceContainer.<>c__DisplayClass162_0.<CreateEmitMethodWrapper>b__0(IEmitter ms)
   at LightInject.ServiceContainer.CreateDynamicMethodDelegate(Action`1 serviceEmitter)
   at LightInject.ServiceContainer.CreateDelegate(Type serviceType, String serviceName, Boolean throwError)
   at LightInject.ServiceContainer.CreateDefaultDelegate(Type serviceType, Boolean throwError)
   at LightInject.ServiceContainer.GetInstance(Type serviceType)
   at ExtendedXmlSerializer.Core.Extensions.Get[T](IServiceProvider this)
   at ExtendedXmlSerializer.Configuration.RootContext.Create()

@AppVeyorBot
Copy link

@Mike-E-angelo
Copy link
Member

Wow interesting. Thank you for reporting this, @oliver-chime and for the commit. Can you verify that the version this occurred in was prior to 3.2.5?

The check for the System.Runtime.Remoting was put in there for #248 and we have some tests around this that fail without it.

However, the bigger problem it would seem is that for some reason the provided TypeInfo is null, so should not be sent through the check to begin with. I will see if there is some point to further filter this so it doesn't produce the null reference exception.

@oliver-chime
Copy link
Contributor Author

The issue occurred with version 3.2.4

@Mike-E-angelo
Copy link
Member

Mike-E-angelo commented Aug 20, 2020

Well to add to the excitement here, I cannot seem to connect to this PR from Visual Studio. Running into this issue here:

https://developercommunity.visualstudio.com/content/problem/1157198/cannot-open-pull-request-from-github-window.html

However, I do believe the issue is with the parameter.Namespace and a null check is required on it.

I understand you are only experiencing this in production -- which is, really, the worst of all fears -- but it would be valuable to somehow see if the following also fixes it for you:

		public bool IsSatisfiedBy(TypeInfo parameter)
			=> parameter.Assembly != _assembly
			   ||
			   parameter.Namespace == null
			   ||
			   !parameter.Namespace.Contains("System.Runtime.Remoting");

Once I am able to connect to this PR I can also further contribute/commit to get this pulled into mainline.

Developer Community for Visual Studio Product family

@Mike-E-angelo Mike-E-angelo added the bug (cc: fix) Something isn't working label Aug 20, 2020
@Mike-E-angelo Mike-E-angelo self-assigned this Aug 20, 2020
@AppVeyorBot
Copy link

Build ExtendedXmlSerializer 3.2.5.1-hnmaclnq completed (commit 14c4b9d884 by @Mike-E-angelo)

Install it by running the following command in Package Manager Console:

Install-Package ExtendedXmlSerializer -Version 3.2.5.1-hnmaclnq -Source https://ci.appveyor.com/nuget/extendedxmlserializer-preview

@Mike-E-angelo
Copy link
Member

Alright @oliver-chime I was able to get that committed. Please try the above package on our preview feed and see if that treats you better. If this is not feasible (since this problem is a production-only issue), please let me know if the committed file works for you and I will accept it into mainline.

The issue occurred with version 3.2.4

Thank you for clarifying this, btw. I was worried that we somehow introduced another problem with #424 -- another null-related issue.

@oliver-chime
Copy link
Contributor Author

Thanks for the quick response @Mike-E-angelo.

Regarding your changes: Shouldn't the condition look like this:
(!(parameter.Namespace?.Contains("System.Runtime.Remoting") ?? false));
Without the parentheses the namespace null case will return false, which would be the opposite of the current solution.

@Mike-E-angelo
Copy link
Member

Indeed @oliver-chime I threw you a curveball. 😁

Somewhere between my suggested condition and my commit I realized that we do not want any type with a null namespace. My hunch was that this condition isn't even considered/covered in the serializer.

I just now created a simple project and it does seem to fail on null namespaces:

However, that is for custom assemblies and not where the condition is probing, which is in the typeof(object).Assembly. In this case, the namespace would be provided (sys:) so this might work. So you are correct, we should err to keep that inline to where it was before. I will make another commit shortly.

@Mike-E-angelo Mike-E-angelo changed the title Fix for parameter/assembly null issue PartitionedTypeSpecification Throws Null Reference Exception on null Namespace Aug 20, 2020
@AppVeyorBot
Copy link

Build ExtendedXmlSerializer 3.2.5.1-wyovnhhb completed (commit 0ee3ab6fcd by @Mike-E-angelo)

Install it by running the following command in Package Manager Console:

Install-Package ExtendedXmlSerializer -Version 3.2.5.1-wyovnhhb -Source https://ci.appveyor.com/nuget/extendedxmlserializer-preview

@oliver-chime
Copy link
Contributor Author

Thanks @Mike-E-angelo , will get back to you once I can verify the change.

@Mike-E-angelo Mike-E-angelo changed the title PartitionedTypeSpecification Throws Null Reference Exception on null Namespace Allowed null Namespace for Framework Types Sep 1, 2020
@Mike-E-angelo Mike-E-angelo merged commit ffd3a89 into ExtendedXmlSerializer:master Sep 1, 2020
@Mike-E-angelo
Copy link
Member

Going to get this deployed to NuGet. Please let me know if you encounter any further issues with it and we'll take it from there. 👍

@Mike-E-angelo
Copy link
Member

Mike-E-angelo commented Sep 1, 2020

Cool, FWIW got you in the release notes, @oliver-chime:

https://github.com/ExtendedXmlSerializer/home/releases/tag/v3.2.6

This has also been deployed to NuGet:

https://www.nuget.org/packages/ExtendedXmlSerializer/

GitHub
🐛 Bug Fixes 🔧

Allowed null Namespace for Framework Types #430 @oliver-chime
Allowed null/Empty Namespaces for Custom/Non-system Assemblies #432 @Mike-E-angelo
Removed Caching from Reference Resolu...

An extensible Xml Serializer for .NET that builds on the functionality of the classic XmlSerializer with a powerful and robust extension model.

@oliver-chime
Copy link
Contributor Author

Thanks @Mike-E-angelo. I can confirm the change solved our issue on production.

@Mike-E-angelo
Copy link
Member

Awesome, good to hear, @oliver-chime! Thanks for the update. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug (cc: fix) Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants