From c36db173b1b424944e026fd7ea4a65897ba1499b Mon Sep 17 00:00:00 2001 From: Dominique Schuppli Date: Tue, 5 Jan 2021 23:48:30 +0100 Subject: [PATCH 1/6] `GetObjectData` can cause verification failure An overridable `GetObjectData` method inside a `[Serializable]` class that does not implement `ISerializable` can be handled in two ways: 1. It can be overridden as part of the `ISerializable` implementation that DynamicProxy provides for `[Serializable]` classes. 2. It can be proxied regularly, like any other overridable method. Unfortunately, at this time, DynamicProxy does both, i.e. it produces two `GetObjectData` methods with identical signature in the same proxy type, which causes verification failure. The correct behavior is (1): The `ISerializable` implementation should take precedence over any method proxying, even though proxied methods are emitted first. (Thus the need for "the trick with methodsToSkip"!) --- .../SerializableClassTestCase.cs | 13 ++++++++ .../Serialization/BadSerializable.cs | 31 +++++++++++++++++++ 2 files changed, 44 insertions(+) create mode 100644 src/Castle.Core.Tests/DynamicProxy.Tests/Serialization/BadSerializable.cs 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 From 8fa581f03809661ff8aa706e2dd8835b8d774daa Mon Sep 17 00:00:00 2001 From: Dominique Schuppli Date: Wed, 6 Jan 2021 00:43:30 +0100 Subject: [PATCH 2/6] Convert check for `[Serializable]` to an assertion --- .../ClassProxySerializableContributor.cs | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/src/Castle.Core/DynamicProxy/Contributors/ClassProxySerializableContributor.cs b/src/Castle.Core/DynamicProxy/Contributors/ClassProxySerializableContributor.cs index 891c0eed5f..cb9eb0919f 100644 --- a/src/Castle.Core/DynamicProxy/Contributors/ClassProxySerializableContributor.cs +++ b/src/Castle.Core/DynamicProxy/Contributors/ClassProxySerializableContributor.cs @@ -18,6 +18,7 @@ namespace Castle.DynamicProxy.Contributors { using System; using System.Collections.Generic; + using System.Diagnostics; using System.Reflection; using System.Runtime.Serialization; @@ -30,7 +31,6 @@ namespace Castle.DynamicProxy.Contributors internal class ClassProxySerializableContributor : SerializableContributor { private readonly bool delegateToBaseGetObjectData; - private readonly bool implementISerializable; private ConstructorInfo serializationConstructor; private readonly IList serializedFields = new List(); @@ -38,20 +38,15 @@ internal class ClassProxySerializableContributor : SerializableContributor string typeId) : base(targetType, interfaces, typeId) { - if (targetType.IsSerializable) - { - implementISerializable = true; - delegateToBaseGetObjectData = VerifyIfBaseImplementsGetObjectData(targetType, methodsToSkip); - } + Debug.Assert(targetType.IsSerializable, "This contributor is intended for serializable types only."); + + delegateToBaseGetObjectData = VerifyIfBaseImplementsGetObjectData(targetType, methodsToSkip); } public override void Generate(ClassEmitter @class) { - if (implementISerializable) - { - ImplementGetObjectData(@class); - Constructor(@class); - } + ImplementGetObjectData(@class); + Constructor(@class); } protected override void AddAddValueInvocation(ArgumentReference serializationInfo, MethodEmitter getObjectData, From 371f7b94c817b252ff57b98c55ad0fb311449ae9 Mon Sep 17 00:00:00 2001 From: Dominique Schuppli Date: Wed, 6 Jan 2021 01:16:05 +0100 Subject: [PATCH 3/6] Replace `methodsToSkip` with `MetaMethod.Ignore` --- .../ClassProxySerializableContributor.cs | 20 +++++++++++++------ .../ClassProxyTargetContributor.cs | 6 ++---- .../ClassProxyWithTargetTargetContributor.cs | 7 ++----- .../Contributors/SerializableContributor.cs | 2 +- .../Generators/BaseClassProxyGenerator.cs | 9 ++++----- .../Generators/ClassProxyGenerator.cs | 8 ++++---- .../ClassProxyWithTargetGenerator.cs | 8 ++++---- .../DynamicProxy/Generators/MetaMethod.cs | 2 ++ 8 files changed, 33 insertions(+), 29 deletions(-) diff --git a/src/Castle.Core/DynamicProxy/Contributors/ClassProxySerializableContributor.cs b/src/Castle.Core/DynamicProxy/Contributors/ClassProxySerializableContributor.cs index cb9eb0919f..8e90a1ea8f 100644 --- a/src/Castle.Core/DynamicProxy/Contributors/ClassProxySerializableContributor.cs +++ b/src/Castle.Core/DynamicProxy/Contributors/ClassProxySerializableContributor.cs @@ -19,9 +19,11 @@ 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; @@ -30,17 +32,19 @@ namespace Castle.DynamicProxy.Contributors internal class ClassProxySerializableContributor : SerializableContributor { - private readonly bool delegateToBaseGetObjectData; + 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) { Debug.Assert(targetType.IsSerializable, "This contributor is intended for serializable types only."); + } - delegateToBaseGetObjectData = VerifyIfBaseImplementsGetObjectData(targetType, methodsToSkip); + public override void CollectElementsToProxy(IProxyGenerationHook hook, MetaType model) + { + delegateToBaseGetObjectData = VerifyIfBaseImplementsGetObjectData(targetType, model); } public override void Generate(ClassEmitter @class) @@ -157,7 +161,7 @@ private void GenerateSerializationConstructor(ClassEmitter emitter) ctor.CodeBuilder.AddStatement(new ReturnStatement()); } - private bool VerifyIfBaseImplementsGetObjectData(Type baseType, IList methodsToSkip) + private bool VerifyIfBaseImplementsGetObjectData(Type baseType, MetaType model) { if (!typeof(ISerializable).IsAssignableFrom(baseType)) { @@ -187,7 +191,11 @@ private bool VerifyIfBaseImplementsGetObjectData(Type baseType, IList m.Method == getObjectDataMethod); + if (getObjectData != null && getObjectData.Proxyable) + { + getObjectData.Ignore = true; + } serializationConstructor = baseType.GetConstructor( BindingFlags.Instance | BindingFlags.Public | diff --git a/src/Castle.Core/DynamicProxy/Contributors/ClassProxyTargetContributor.cs b/src/Castle.Core/DynamicProxy/Contributors/ClassProxyTargetContributor.cs index 38f03d52a0..ea5f3f6f78 100644 --- a/src/Castle.Core/DynamicProxy/Contributors/ClassProxyTargetContributor.cs +++ b/src/Castle.Core/DynamicProxy/Contributors/ClassProxyTargetContributor.cs @@ -29,14 +29,12 @@ namespace Castle.DynamicProxy.Contributors internal class ClassProxyTargetContributor : CompositeTypeContributor { - private readonly 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; } From 5775957cb3fc6fda8f441491bb7cb5bcc401759d Mon Sep 17 00:00:00 2001 From: Dominique Schuppli Date: Wed, 6 Jan 2021 01:42:33 +0100 Subject: [PATCH 4/6] Guard against double `GetObjectData` impl better --- .../ClassProxySerializableContributor.cs | 42 +++++++++++++++---- 1 file changed, 35 insertions(+), 7 deletions(-) diff --git a/src/Castle.Core/DynamicProxy/Contributors/ClassProxySerializableContributor.cs b/src/Castle.Core/DynamicProxy/Contributors/ClassProxySerializableContributor.cs index 8e90a1ea8f..7ca3ff718a 100644 --- a/src/Castle.Core/DynamicProxy/Contributors/ClassProxySerializableContributor.cs +++ b/src/Castle.Core/DynamicProxy/Contributors/ClassProxySerializableContributor.cs @@ -44,7 +44,37 @@ public ClassProxySerializableContributor(Type targetType, Type[] interfaces, str public override void CollectElementsToProxy(IProxyGenerationHook hook, MetaType model) { - delegateToBaseGetObjectData = VerifyIfBaseImplementsGetObjectData(targetType, 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) + { + // `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.Methods.FirstOrDefault(m => m.Method == getObjectDataMethod); + } + } + + if (getObjectData != null && getObjectData.Proxyable) + { + getObjectData.Ignore = true; + } } public override void Generate(ClassEmitter @class) @@ -161,8 +191,10 @@ private void GenerateSerializationConstructor(ClassEmitter emitter) ctor.CodeBuilder.AddStatement(new ReturnStatement()); } - private bool VerifyIfBaseImplementsGetObjectData(Type baseType, MetaType model) + private bool VerifyIfBaseImplementsGetObjectData(Type baseType, MetaType model, out MetaMethod getObjectData) { + getObjectData = null; + if (!typeof(ISerializable).IsAssignableFrom(baseType)) { return false; @@ -191,11 +223,7 @@ private bool VerifyIfBaseImplementsGetObjectData(Type baseType, MetaType model) throw new ArgumentException(message); } - var getObjectData = model.Methods.FirstOrDefault(m => m.Method == getObjectDataMethod); - if (getObjectData != null && getObjectData.Proxyable) - { - getObjectData.Ignore = true; - } + getObjectData = model.Methods.FirstOrDefault(m => m.Method == getObjectDataMethod); serializationConstructor = baseType.GetConstructor( BindingFlags.Instance | BindingFlags.Public | From d7b39302469c56e5ecb184a7c40f54a98c2ac31f Mon Sep 17 00:00:00 2001 From: Dominique Schuppli Date: Tue, 19 Jan 2021 19:37:22 +0100 Subject: [PATCH 5/6] Add method index to `MetaType` for faster lookup --- .../Contributors/ClassProxySerializableContributor.cs | 4 ++-- src/Castle.Core/DynamicProxy/Generators/MetaType.cs | 8 ++++++++ 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/src/Castle.Core/DynamicProxy/Contributors/ClassProxySerializableContributor.cs b/src/Castle.Core/DynamicProxy/Contributors/ClassProxySerializableContributor.cs index 7ca3ff718a..d41bd25806 100644 --- a/src/Castle.Core/DynamicProxy/Contributors/ClassProxySerializableContributor.cs +++ b/src/Castle.Core/DynamicProxy/Contributors/ClassProxySerializableContributor.cs @@ -67,7 +67,7 @@ public override void CollectElementsToProxy(IProxyGenerationHook hook, MetaType if (getObjectDataMethod != null) { - getObjectData = model.Methods.FirstOrDefault(m => m.Method == getObjectDataMethod); + getObjectData = model.FindMethod(getObjectDataMethod); } } @@ -223,7 +223,7 @@ private bool VerifyIfBaseImplementsGetObjectData(Type baseType, MetaType model, throw new ArgumentException(message); } - getObjectData = model.Methods.FirstOrDefault(m => m.Method == getObjectDataMethod); + getObjectData = model.FindMethod(getObjectDataMethod); serializationConstructor = baseType.GetConstructor( BindingFlags.Instance | BindingFlags.Public | 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 From 87b6e58c2aa663e2d59bdd0b0acaa8cd3365421d Mon Sep 17 00:00:00 2001 From: Dominique Schuppli Date: Wed, 6 Jan 2021 01:53:41 +0100 Subject: [PATCH 6/6] Update changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) 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)