Merge pull request #554 from stakx/bad-serializables

Prevent double `GetObjectData` implementation for badly implemented serializable classes

CHANGELOG.md

@@ -6,6 +6,7 @@ Enhancements:
 - .NET Standard 2.0 and 2.1 support (@lg2de, #485)
 
 Bugfixes:
+- Proxying certain `[Serializable]` classes produces proxy types that fail PEVerify test (@stakx, #367)
 - `private protected` methods are not intercepted (@CrispyDrone, #535)
 - `System.UIntPtr` unsupported (@stakx, #546)
 

src/Castle.Core.Tests/DynamicProxy.Tests/SerializableClassTestCase.cs

@@ -591,6 +591,19 @@ public void SimpleProxySerialization()
 			Assert.AreEqual(current, otherProxy.Current);
 		}
 
+		[Test]
+		public void BadSerializable()
+		{
+			const int expectedValue = 13;
+
+			var proxy = generator.CreateClassProxy<BadSerializable>();
+			proxy.Value = expectedValue;
+
+			var otherProxy = SerializeAndDeserialize(proxy);
+
+			Assert.AreEqual(expectedValue, otherProxy.Value);
+		}
+
 		public override void TearDown()
 		{
 			base.TearDown();

src/Castle.Core.Tests/DynamicProxy.Tests/Serialization/BadSerializable.cs

@@ -0,0 +1,31 @@
+// Copyright 2004-2021 Castle Project - http://www.castleproject.org/
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#if FEATURE_SERIALIZATION
+
+namespace Castle.DynamicProxy.Tests.Serialization
+{
+	using System;
+	using System.Runtime.Serialization;
+
+	[Serializable]
+	public abstract class BadSerializable
+	{
+		public int Value { get; set; }
+
+		public abstract void GetObjectData(SerializationInfo info, StreamingContext context);
+	}
+}
+
+#endif

src/Castle.Core/DynamicProxy/Contributors/ClassProxySerializableContributor.cs

@@ -18,9 +18,12 @@ namespace Castle.DynamicProxy.Contributors
 {
 	using System;
 	using System.Collections.Generic;
+	using System.Diagnostics;
+	using System.Linq;
 	using System.Reflection;
 	using System.Runtime.Serialization;
 
+	using Castle.DynamicProxy.Generators;
 	using Castle.DynamicProxy.Generators.Emitters;
 	using Castle.DynamicProxy.Generators.Emitters.CodeBuilders;
 	using Castle.DynamicProxy.Generators.Emitters.SimpleAST;
@@ -29,29 +32,55 @@ namespace Castle.DynamicProxy.Contributors
 
 	internal class ClassProxySerializableContributor : SerializableContributor
 	{
-		private readonly bool delegateToBaseGetObjectData;
-		private readonly bool implementISerializable;
+		private bool delegateToBaseGetObjectData;
 		private ConstructorInfo serializationConstructor;
 		private readonly IList<FieldReference> serializedFields = new List<FieldReference>();
 
-		public ClassProxySerializableContributor(Type targetType, IList<MethodInfo> methodsToSkip, Type[] interfaces,
-		                                     string typeId)
+		public ClassProxySerializableContributor(Type targetType, Type[] interfaces, string typeId)
 			: base(targetType, interfaces, typeId)
 		{
-			if (targetType.IsSerializable)
+			Debug.Assert(targetType.IsSerializable, "This contributor is intended for serializable types only.");
+		}
+
+		public override void CollectElementsToProxy(IProxyGenerationHook hook, MetaType model)
+		{
+			delegateToBaseGetObjectData = VerifyIfBaseImplementsGetObjectData(targetType, model, out var getObjectData);
+
+			// This contributor is going to add a `GetObjectData` method to the proxy type.
+			// If a method with the same name and signature exists in the proxied class type,
+			// and another contributor has decided to proxy it, we need to tell it not to.
+			// Otherwise, we'll end up with two implementations!
+
+			if (getObjectData == null)
 			{
-				implementISerializable = true;
-				delegateToBaseGetObjectData = VerifyIfBaseImplementsGetObjectData(targetType, methodsToSkip);
+				// `VerifyIfBaseImplementsGetObjectData` only searches for `GetObjectData`
+				// in the implementation map for `ISerializable`. In the best case, it was
+				// already found there. If not, we need to look again, since *any* method
+				// with the same signature is a problem.
+
+				var getObjectDataMethod = targetType.GetMethod(
+					"GetObjectData",
+					BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Instance,
+					null,
+					new[] { typeof(SerializationInfo), typeof(StreamingContext) },
+					null);
+
+				if (getObjectDataMethod != null)
+				{
+					getObjectData = model.FindMethod(getObjectDataMethod);
+				}
+			}
+
+			if (getObjectData != null && getObjectData.Proxyable)
+			{
+				getObjectData.Ignore = true;
 			}
 		}
 
 		public override void Generate(ClassEmitter @class)
 		{
-			if (implementISerializable)
-			{
-				ImplementGetObjectData(@class);
-				Constructor(@class);
-			}
+			ImplementGetObjectData(@class);
+			Constructor(@class);
 		}
 
 		protected override void AddAddValueInvocation(ArgumentReference serializationInfo, MethodEmitter getObjectData,
@@ -162,8 +191,10 @@ private void GenerateSerializationConstructor(ClassEmitter emitter)
 			ctor.CodeBuilder.AddStatement(new ReturnStatement());
 		}
 
-		private bool VerifyIfBaseImplementsGetObjectData(Type baseType, IList<MethodInfo> methodsToSkip)
+		private bool VerifyIfBaseImplementsGetObjectData(Type baseType, MetaType model, out MetaMethod getObjectData)
 		{
+			getObjectData = null;
+
 			if (!typeof(ISerializable).IsAssignableFrom(baseType))
 			{
 				return false;
@@ -192,7 +223,7 @@ private bool VerifyIfBaseImplementsGetObjectData(Type baseType, IList<MethodInfo
 				throw new ArgumentException(message);
 			}
 
-			methodsToSkip.Add(getObjectDataMethod);
+			getObjectData = model.FindMethod(getObjectDataMethod);
 
 			serializationConstructor = baseType.GetConstructor(
 				BindingFlags.Instance | BindingFlags.Public |

src/Castle.Core/DynamicProxy/Contributors/ClassProxyTargetContributor.cs

@@ -29,14 +29,12 @@ namespace Castle.DynamicProxy.Contributors
 
 	internal class ClassProxyTargetContributor : CompositeTypeContributor
 	{
-		private readonly IList<MethodInfo> methodsToSkip;
 		private readonly Type targetType;
 
-		public ClassProxyTargetContributor(Type targetType, IList<MethodInfo> methodsToSkip, INamingScope namingScope)
+		public ClassProxyTargetContributor(Type targetType, INamingScope namingScope)
 			: base(namingScope)
 		{
 			this.targetType = targetType;
-			this.methodsToSkip = methodsToSkip;
 		}
 
 		protected override IEnumerable<MembersCollector> CollectElementsToProxyInternal(IProxyGenerationHook hook)
@@ -59,7 +57,7 @@ protected override IEnumerable<MembersCollector> CollectElementsToProxyInternal(
 		protected override MethodGenerator GetMethodGenerator(MetaMethod method, ClassEmitter @class,
 		                                                      OverrideMethodDelegate overrideMethod)
 		{
-			if (methodsToSkip.Contains(method.Method))
+			if (method.Ignore)
 			{
 				return null;
 			}

src/Castle.Core/DynamicProxy/Contributors/ClassProxyWithTargetTargetContributor.cs

@@ -27,15 +27,12 @@ namespace Castle.DynamicProxy.Contributors
 
 	internal class ClassProxyWithTargetTargetContributor : CompositeTypeContributor
 	{
-		private readonly IList<MethodInfo> methodsToSkip;
 		private readonly Type targetType;
 
-		public ClassProxyWithTargetTargetContributor(Type targetType, IList<MethodInfo> methodsToSkip,
-		                                             INamingScope namingScope)
+		public ClassProxyWithTargetTargetContributor(Type targetType, INamingScope namingScope)
 			: base(namingScope)
 		{
 			this.targetType = targetType;
-			this.methodsToSkip = methodsToSkip;
 		}
 
 		protected override IEnumerable<MembersCollector> CollectElementsToProxyInternal(IProxyGenerationHook hook)
@@ -58,7 +55,7 @@ protected override IEnumerable<MembersCollector> CollectElementsToProxyInternal(
 		protected override MethodGenerator GetMethodGenerator(MetaMethod method, ClassEmitter @class,
 		                                                      OverrideMethodDelegate overrideMethod)
 		{
-			if (methodsToSkip.Contains(method.Method))
+			if (method.Ignore)
 			{
 				return null;
 			}

src/Castle.Core/DynamicProxy/Contributors/SerializableContributor.cs

@@ -150,7 +150,7 @@ protected void ImplementGetObjectData(ClassEmitter emitter)
 		protected abstract void CustomizeGetObjectData(AbstractCodeBuilder builder, ArgumentReference serializationInfo,
 		                                               ArgumentReference streamingContext, ClassEmitter emitter);
 
-		public void CollectElementsToProxy(IProxyGenerationHook hook, MetaType model)
+		public virtual void CollectElementsToProxy(IProxyGenerationHook hook, MetaType model)
 		{
 		}
 	}

src/Castle.Core/DynamicProxy/Generators/BaseClassProxyGenerator.cs

@@ -34,10 +34,10 @@ protected BaseClassProxyGenerator(ModuleScope scope, Type targetType, Type[] int
 		protected abstract FieldReference TargetField { get; }
 
 #if FEATURE_SERIALIZATION
-		protected abstract SerializableContributor GetSerializableContributor(List<MethodInfo> methodsToSkip);
+		protected abstract SerializableContributor GetSerializableContributor();
 #endif
 
-		protected abstract CompositeTypeContributor GetProxyTargetContributor(List<MethodInfo> methodsToSkip, INamingScope namingScope);
+		protected abstract CompositeTypeContributor GetProxyTargetContributor(INamingScope namingScope);
 
 		protected abstract ProxyTargetAccessorContributor GetProxyTargetAccessorContributor();
 
@@ -109,15 +109,14 @@ protected sealed override Type GenerateType(string name, INamingScope namingScop
 		private IEnumerable<Type> GetTypeImplementerMapping(out IEnumerable<ITypeContributor> contributors, INamingScope namingScope)
 		{
 			var contributorsList = new List<ITypeContributor>(capacity: 5);
-			var methodsToSkip = new List<MethodInfo>();  // TODO: the trick with methodsToSkip is not very nice...
 			var targetInterfaces = targetType.GetAllInterfaces();
 			var typeImplementerMapping = new Dictionary<Type, ITypeContributor>();
 
 			// Order of interface precedence:
 
 			// 1. first target
 			// target is not an interface so we do nothing
-			var targetContributor = GetProxyTargetContributor(methodsToSkip, namingScope);
+			var targetContributor = GetProxyTargetContributor(namingScope);
 			contributorsList.Add(targetContributor);
 
 			// 2. then mixins
@@ -183,7 +182,7 @@ private IEnumerable<Type> GetTypeImplementerMapping(out IEnumerable<ITypeContrib
 #if FEATURE_SERIALIZATION
 			if (targetType.IsSerializable)
 			{
-				var serializableContributor = GetSerializableContributor(methodsToSkip);
+				var serializableContributor = GetSerializableContributor();
 				contributorsList.Add(serializableContributor);
 				AddMappingForISerializable(typeImplementerMapping, serializableContributor);
 			}

src/Castle.Core/DynamicProxy/Generators/ClassProxyGenerator.cs

@@ -37,15 +37,15 @@ protected override CacheKey GetCacheKey()
 		}
 
 #if FEATURE_SERIALIZATION
-		protected override SerializableContributor GetSerializableContributor(List<MethodInfo> methodsToSkip)
+		protected override SerializableContributor GetSerializableContributor()
 		{
-			return new ClassProxySerializableContributor(targetType, methodsToSkip, interfaces, ProxyTypeConstants.Class);
+			return new ClassProxySerializableContributor(targetType, interfaces, ProxyTypeConstants.Class);
 		}
 #endif
 
-		protected override CompositeTypeContributor GetProxyTargetContributor(List<MethodInfo> methodsToSkip, INamingScope namingScope)
+		protected override CompositeTypeContributor GetProxyTargetContributor(INamingScope namingScope)
 		{
-			return new ClassProxyTargetContributor(targetType, methodsToSkip, namingScope) { Logger = Logger };
+			return new ClassProxyTargetContributor(targetType, namingScope) { Logger = Logger };
 		}
 
 		protected override ProxyTargetAccessorContributor GetProxyTargetAccessorContributor()

src/Castle.Core/DynamicProxy/Generators/ClassProxyWithTargetGenerator.cs

@@ -51,15 +51,15 @@ protected override void CreateFields(ClassEmitter emitter)
 		}
 
 #if FEATURE_SERIALIZATION
-		protected override SerializableContributor GetSerializableContributor(List<MethodInfo> methodsToSkip)
+		protected override SerializableContributor GetSerializableContributor()
 		{
-			return new ClassProxySerializableContributor(targetType, methodsToSkip, interfaces, ProxyTypeConstants.ClassWithTarget);
+			return new ClassProxySerializableContributor(targetType, interfaces, ProxyTypeConstants.ClassWithTarget);
 		}
 #endif
 
-		protected override CompositeTypeContributor GetProxyTargetContributor(List<MethodInfo> methodsToSkip, INamingScope namingScope)
+		protected override CompositeTypeContributor GetProxyTargetContributor(INamingScope namingScope)
 		{
-			return new ClassProxyWithTargetTargetContributor(targetType, methodsToSkip, namingScope) { Logger = Logger };
+			return new ClassProxyWithTargetTargetContributor(targetType, namingScope) { Logger = Logger };
 		}
 
 		protected override ProxyTargetAccessorContributor GetProxyTargetAccessorContributor()

src/Castle.Core/DynamicProxy/Generators/MetaMethod.cs

@@ -52,6 +52,8 @@ public string Name
 			get { return name; }
 		}
 
+		public bool Ignore { get; internal set; }
+
 		public bool Proxyable { get; private set; }
 
 		public bool Standalone { get; private set; }

src/Castle.Core/DynamicProxy/Generators/MetaType.cs

@@ -15,11 +15,13 @@
 namespace Castle.DynamicProxy.Generators
 {
 	using System.Collections.Generic;
+	using System.Reflection;
 
 	internal class MetaType
 	{
 		private readonly ICollection<MetaEvent> events = new TypeElementCollection<MetaEvent>();
 		private readonly ICollection<MetaMethod> methods = new TypeElementCollection<MetaMethod>();
+		private readonly Dictionary<MethodInfo, MetaMethod> methodsIndex = new Dictionary<MethodInfo, MetaMethod>();
 		private readonly ICollection<MetaProperty> properties = new TypeElementCollection<MetaProperty>();
 
 		public IEnumerable<MetaEvent> Events
@@ -46,11 +48,17 @@ public void AddEvent(MetaEvent @event)
 		public void AddMethod(MetaMethod method)
 		{
 			methods.Add(method);
+			methodsIndex.Add(method.Method, method);  // shouldn't get added twice
 		}
 
 		public void AddProperty(MetaProperty property)
 		{
 			properties.Add(property);
 		}
+
+		public MetaMethod FindMethod(MethodInfo method)
+		{
+			return methodsIndex.TryGetValue(method, out var metaMethod) ? metaMethod : null;
+		}
 	}
 }