diff --git a/CHANGELOG.md b/CHANGELOG.md index 4e1ef47762..6f32091ed3 100644 --- a/CHANGELOG.md +++ b/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) diff --git a/src/Castle.Core.Tests/DynamicProxy.Tests/SerializableClassTestCase.cs b/src/Castle.Core.Tests/DynamicProxy.Tests/SerializableClassTestCase.cs index 58a02e5657..c869686271 100644 --- a/src/Castle.Core.Tests/DynamicProxy.Tests/SerializableClassTestCase.cs +++ b/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(); + proxy.Value = expectedValue; + + var otherProxy = SerializeAndDeserialize(proxy); + + Assert.AreEqual(expectedValue, otherProxy.Value); + } + public override void TearDown() { base.TearDown(); diff --git a/src/Castle.Core.Tests/DynamicProxy.Tests/Serialization/BadSerializable.cs b/src/Castle.Core.Tests/DynamicProxy.Tests/Serialization/BadSerializable.cs new file mode 100644 index 0000000000..55c1b3b332 --- /dev/null +++ b/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 diff --git a/src/Castle.Core/DynamicProxy/Contributors/ClassProxySerializableContributor.cs b/src/Castle.Core/DynamicProxy/Contributors/ClassProxySerializableContributor.cs index 891c0eed5f..d41bd25806 100644 --- a/src/Castle.Core/DynamicProxy/Contributors/ClassProxySerializableContributor.cs +++ b/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 serializedFields = new List(); - public ClassProxySerializableContributor(Type targetType, IList 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 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 methodsToSkip; private readonly Type targetType; - public ClassProxyTargetContributor(Type targetType, IList methodsToSkip, INamingScope namingScope) + public ClassProxyTargetContributor(Type targetType, INamingScope namingScope) : base(namingScope) { this.targetType = targetType; - this.methodsToSkip = methodsToSkip; } protected override IEnumerable CollectElementsToProxyInternal(IProxyGenerationHook hook) @@ -59,7 +57,7 @@ protected override IEnumerable CollectElementsToProxyInternal( protected override MethodGenerator GetMethodGenerator(MetaMethod method, ClassEmitter @class, OverrideMethodDelegate overrideMethod) { - if (methodsToSkip.Contains(method.Method)) + if (method.Ignore) { return null; } diff --git a/src/Castle.Core/DynamicProxy/Contributors/ClassProxyWithTargetTargetContributor.cs b/src/Castle.Core/DynamicProxy/Contributors/ClassProxyWithTargetTargetContributor.cs index 911f0fc571..f27afd2cf1 100644 --- a/src/Castle.Core/DynamicProxy/Contributors/ClassProxyWithTargetTargetContributor.cs +++ b/src/Castle.Core/DynamicProxy/Contributors/ClassProxyWithTargetTargetContributor.cs @@ -27,15 +27,12 @@ namespace Castle.DynamicProxy.Contributors internal class ClassProxyWithTargetTargetContributor : CompositeTypeContributor { - private readonly IList methodsToSkip; private readonly Type targetType; - public ClassProxyWithTargetTargetContributor(Type targetType, IList methodsToSkip, - INamingScope namingScope) + public ClassProxyWithTargetTargetContributor(Type targetType, INamingScope namingScope) : base(namingScope) { this.targetType = targetType; - this.methodsToSkip = methodsToSkip; } protected override IEnumerable CollectElementsToProxyInternal(IProxyGenerationHook hook) @@ -58,7 +55,7 @@ protected override IEnumerable CollectElementsToProxyInternal( protected override MethodGenerator GetMethodGenerator(MetaMethod method, ClassEmitter @class, OverrideMethodDelegate overrideMethod) { - if (methodsToSkip.Contains(method.Method)) + if (method.Ignore) { return null; } diff --git a/src/Castle.Core/DynamicProxy/Contributors/SerializableContributor.cs b/src/Castle.Core/DynamicProxy/Contributors/SerializableContributor.cs index 4b1f2b222a..48f213a51c 100644 --- a/src/Castle.Core/DynamicProxy/Contributors/SerializableContributor.cs +++ b/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) { } } diff --git a/src/Castle.Core/DynamicProxy/Generators/BaseClassProxyGenerator.cs b/src/Castle.Core/DynamicProxy/Generators/BaseClassProxyGenerator.cs index b3f3e17ade..02f6012380 100644 --- a/src/Castle.Core/DynamicProxy/Generators/BaseClassProxyGenerator.cs +++ b/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 methodsToSkip); + protected abstract SerializableContributor GetSerializableContributor(); #endif - protected abstract CompositeTypeContributor GetProxyTargetContributor(List methodsToSkip, INamingScope namingScope); + protected abstract CompositeTypeContributor GetProxyTargetContributor(INamingScope namingScope); protected abstract ProxyTargetAccessorContributor GetProxyTargetAccessorContributor(); @@ -109,7 +109,6 @@ protected sealed override Type GenerateType(string name, INamingScope namingScop private IEnumerable GetTypeImplementerMapping(out IEnumerable contributors, INamingScope namingScope) { var contributorsList = new List(capacity: 5); - var methodsToSkip = new List(); // TODO: the trick with methodsToSkip is not very nice... var targetInterfaces = targetType.GetAllInterfaces(); var typeImplementerMapping = new Dictionary(); @@ -117,7 +116,7 @@ private IEnumerable GetTypeImplementerMapping(out IEnumerable GetTypeImplementerMapping(out IEnumerable 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 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() diff --git a/src/Castle.Core/DynamicProxy/Generators/ClassProxyWithTargetGenerator.cs b/src/Castle.Core/DynamicProxy/Generators/ClassProxyWithTargetGenerator.cs index 647eaee207..c7f6fe6ad5 100644 --- a/src/Castle.Core/DynamicProxy/Generators/ClassProxyWithTargetGenerator.cs +++ b/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 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 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() diff --git a/src/Castle.Core/DynamicProxy/Generators/MetaMethod.cs b/src/Castle.Core/DynamicProxy/Generators/MetaMethod.cs index cd69018202..097008b6b9 100644 --- a/src/Castle.Core/DynamicProxy/Generators/MetaMethod.cs +++ b/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; } diff --git a/src/Castle.Core/DynamicProxy/Generators/MetaType.cs b/src/Castle.Core/DynamicProxy/Generators/MetaType.cs index 91cf51150c..c0fced51de 100644 --- a/src/Castle.Core/DynamicProxy/Generators/MetaType.cs +++ b/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 events = new TypeElementCollection(); private readonly ICollection methods = new TypeElementCollection(); + private readonly Dictionary methodsIndex = new Dictionary(); private readonly ICollection properties = new TypeElementCollection(); public IEnumerable 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; + } } } \ No newline at end of file