From 8a42f10e27e70b265bb27d8e90ecb6d944452766 Mon Sep 17 00:00:00 2001 From: Alex Povar Date: Wed, 13 Jun 2018 12:10:40 +0300 Subject: [PATCH] Ensure parameters with 'in' modifier cannot be modified --- src/NSubstitute/NSubstitute.csproj | 2 +- .../CastleDynamicProxyFactory.cs | 15 ++- .../DelegateProxy/DelegateProxyFactory.cs | 105 ++++++++++++++---- .../FieldReports/Issue378_InValueTypes.cs | 84 ++++++++++++-- 4 files changed, 173 insertions(+), 33 deletions(-) diff --git a/src/NSubstitute/NSubstitute.csproj b/src/NSubstitute/NSubstitute.csproj index 5f4111434..8b6dd3fd7 100644 --- a/src/NSubstitute/NSubstitute.csproj +++ b/src/NSubstitute/NSubstitute.csproj @@ -36,7 +36,7 @@ - + diff --git a/src/NSubstitute/Proxies/CastleDynamicProxy/CastleDynamicProxyFactory.cs b/src/NSubstitute/Proxies/CastleDynamicProxy/CastleDynamicProxyFactory.cs index b7c66a837..7dc3e3856 100644 --- a/src/NSubstitute/Proxies/CastleDynamicProxy/CastleDynamicProxyFactory.cs +++ b/src/NSubstitute/Proxies/CastleDynamicProxy/CastleDynamicProxyFactory.cs @@ -46,12 +46,19 @@ public object GenerateProxy(ICallRouter callRouter, Type typeToProxy, Type[] add } /// - /// Allows to dynamically create a type in runtime. Returns an instance of , - /// so type could be customized and built later. + /// Allows to dynamically create a type in runtime. + /// Use the callback to define and build the type. /// - public TypeBuilder DefineDynamicType(string typeName, TypeAttributes flags) + /// Callback used to construct the type. + /// The result returned by callback. + public Type DefineDynamicType(Func typeBuildCallback) { - return _proxyGenerator.ProxyBuilder.ModuleScope.DefineType(true, typeName, flags); + var moduleBuilder = _proxyGenerator.ProxyBuilder.ModuleScope.ObtainDynamicModuleWithStrongName(); + + using (_proxyGenerator.ProxyBuilder.ModuleScope.Lock.ForWriting()) + { + return typeBuildCallback.Invoke(moduleBuilder); + } } private object CreateProxyUsingCastleProxyGenerator(Type typeToProxy, Type[] additionalInterfaces, diff --git a/src/NSubstitute/Proxies/DelegateProxy/DelegateProxyFactory.cs b/src/NSubstitute/Proxies/DelegateProxy/DelegateProxyFactory.cs index b4931bfea..03ebbc9e7 100644 --- a/src/NSubstitute/Proxies/DelegateProxy/DelegateProxyFactory.cs +++ b/src/NSubstitute/Proxies/DelegateProxy/DelegateProxyFactory.cs @@ -14,6 +14,7 @@ namespace NSubstitute.Proxies.DelegateProxy public class DelegateProxyFactory : IProxyFactory { private const string MethodNameInsideProxyContainer = "Invoke"; + private const string IsReadOnlyAttributeFullTypeName = "System.Runtime.CompilerServices.IsReadOnlyAttribute"; private readonly CastleDynamicProxyFactory _castleObjectProxyFactory; private readonly ConcurrentDictionary _delegateContainerCache = new ConcurrentDictionary(); private long _typeSuffixCounter; @@ -39,7 +40,7 @@ public object GenerateProxy(ICallRouter callRouter, Type typeToProxy, Type[] add return DelegateProxy(typeToProxy, callRouter); } - private bool HasItems(T[] array) + private static bool HasItems(T[] array) { return array != null && array.Length > 0; } @@ -67,31 +68,95 @@ private Type GenerateDelegateContainerInterface(Type delegateType) delegateTypeName, typeSuffixCounter.ToString(CultureInfo.InvariantCulture)); - var typeBuilder = _castleObjectProxyFactory.DefineDynamicType( - typeName, - TypeAttributes.Abstract | TypeAttributes.Interface | TypeAttributes.Public); + return _castleObjectProxyFactory.DefineDynamicType(moduleBuilder => + { + var typeBuilder = moduleBuilder.DefineType( + typeName, + TypeAttributes.Abstract | TypeAttributes.Interface | TypeAttributes.Public); + + // Notice, we don't copy the custom modifiers here. + // That's absolutely fine, as custom modifiers are ignored when delegate is constructed. + // See the related discussion here: https://github.com/dotnet/coreclr/issues/18401 + var methodBuilder = typeBuilder + .DefineMethod( + MethodNameInsideProxyContainer, + MethodAttributes.Abstract | MethodAttributes.Virtual | MethodAttributes.Public, + CallingConventions.Standard, + delegateSignature.ReturnType, + delegateSignature.GetParameters().Select(p => p.ParameterType).ToArray()); + + // Copy original method attributes, so "out" parameters are recognized later. + for (var i = 0; i < delegateParameters.Length; i++) + { + var parameter = delegateParameters[i]; + + // Increment position by 1 to skip the implicit "this" parameter. + var paramBuilder = methodBuilder.DefineParameter(i + 1, parameter.Attributes, parameter.Name); + + // Read-only parameter ('in' keyword) is recognized by presence of the special attribute. + // If source parameter contained that attribute, ensure to copy it to the generated method. + // That helps Castle to understand that parameter is read-only and cannot be mutated. + DefineIsReadOnlyAttributeIfNeeded(parameter, paramBuilder, moduleBuilder); + } + + // Preserve the original delegate type in attribute, so it can be retrieved later in code. + methodBuilder.SetCustomAttribute( + new CustomAttributeBuilder( + typeof(ProxiedDelegateTypeAttribute).GetConstructors().Single(), + new object[] {delegateType})); + + return typeBuilder.CreateTypeInfo().AsType(); + }); + } + + private static void DefineIsReadOnlyAttributeIfNeeded( + ParameterInfo sourceParameter, ParameterBuilder paramBuilder, ModuleBuilder dynamicModuleBuilder) + { + // Read-only parameter can be by-ref only. + if (!sourceParameter.ParameterType.IsByRef) + { + return; + } + + // Lookup for the attribute using full type name. + // That's required because compiler can embed that type directly to the client's assembly + // as type identity doesn't matter - only full type attribute name is checked. + var isReadOnlyAttrType = sourceParameter.CustomAttributes + .Select(ca => ca.AttributeType) + .FirstOrDefault(t => t.FullName.Equals(IsReadOnlyAttributeFullTypeName, StringComparison.Ordinal)); - var methodBuilder = typeBuilder - .DefineMethod( - MethodNameInsideProxyContainer, - MethodAttributes.Abstract | MethodAttributes.Virtual | MethodAttributes.Public, - delegateSignature.ReturnType, - delegateParameters.Select(p => p.ParameterType).ToArray()); + // Parameter doesn't contain the IsReadOnly attribute. + if (isReadOnlyAttrType == null) + { + return; + } - // Copy original method attributes, so "out" parameters are recognized later. - for (var i = 0; i < delegateParameters.Length; i++) + // If the compiler generated attribute is used (e.g. runtime doesn't contain the attribute), + // the generated attribute type might be internal, so we cannot referecnce it in the dynamic assembly. + // In this case use the attribute type from the dynamic assembly. + if (!isReadOnlyAttrType.GetTypeInfo().IsVisible) { - // Increment position by 1 to skip the implicit "this" parameter. - methodBuilder.DefineParameter(i + 1, delegateParameters[i].Attributes, delegateParameters[i].Name); + isReadOnlyAttrType = GetIsReadOnlyAttributeInDynamicModule(dynamicModuleBuilder); } - // Preserve the original delegate type in attribute, so it can be retrieved later in code. - methodBuilder.SetCustomAttribute( - new CustomAttributeBuilder( - typeof(ProxiedDelegateTypeAttribute).GetConstructors().Single(), - new object[] {delegateType})); + paramBuilder.SetCustomAttribute( + new CustomAttributeBuilder(isReadOnlyAttrType.GetConstructor(Type.EmptyTypes), new object[0])); + } + + private static Type GetIsReadOnlyAttributeInDynamicModule(ModuleBuilder moduleBuilder) + { + var existingType = moduleBuilder.Assembly.GetType(IsReadOnlyAttributeFullTypeName, throwOnError: false, ignoreCase: false); + if (existingType != null) + { + return existingType; + } - return typeBuilder.CreateTypeInfo().AsType(); + return moduleBuilder + .DefineType( + IsReadOnlyAttributeFullTypeName, + TypeAttributes.Class | TypeAttributes.Sealed | TypeAttributes.NotPublic, + typeof(Attribute)) + .CreateTypeInfo().AsType(); } } } \ No newline at end of file diff --git a/tests/NSubstitute.Acceptance.Specs/FieldReports/Issue378_InValueTypes.cs b/tests/NSubstitute.Acceptance.Specs/FieldReports/Issue378_InValueTypes.cs index 3c4905e91..33e78aa1a 100644 --- a/tests/NSubstitute.Acceptance.Specs/FieldReports/Issue378_InValueTypes.cs +++ b/tests/NSubstitute.Acceptance.Specs/FieldReports/Issue378_InValueTypes.cs @@ -1,5 +1,4 @@ -using System; -using NUnit.Framework; +using NUnit.Framework; namespace NSubstitute.Acceptance.Specs.FieldReports { @@ -8,22 +7,91 @@ namespace NSubstitute.Acceptance.Specs.FieldReports /// public class Issue378_InValueTypes { - public readonly struct Struct { } + public readonly struct Struct + { + public Struct(int value) + { + Value = value; + } + + public int Value { get; } + } + + public interface IStructByReadOnlyRefConsumer { void Consume(in Struct value); } + + public interface IStructByValueConsumer { void Consume(Struct value); } - public interface IStructByRefConsumer { void Consume(in Struct message); } + public delegate void DelegateStructByReadOnlyRefConsumer(in Struct value); - public interface IStructByValueConsumer { void Consume(Struct message); } + public delegate void DelegateStructByReadOnlyRefConsumerMultipleArgs(in Struct value1, in Struct value2); [Test] - public void IStructByRefConsumer_Test() + public void IStructByReadOnlyRefConsumer_Test() { - _ = Substitute.For(); + var value = new Struct(42); + + var subs = Substitute.For(); + subs.Consume(in value); } [Test] public void IStructByValueConsumer_Test() { - _ = Substitute.For(); + var value = new Struct(42); + + var subs = Substitute.For(); + subs.Consume(value); + } + + [Test] + public void DelegateByReadOnlyRefConsumer_Test() + { + var value = new Struct(42); + + var subs = Substitute.For(); + subs.Invoke(in value); + } + + [Test] + public void InterfaceReadOnlyRefCannotBeModified() + { + var readOnlyValue = new Struct(42); + + var subs = Substitute.For(); + subs.When(x => x.Consume(Arg.Any())).Do(c => { c[0] = new Struct(24); }); + + subs.Consume(in readOnlyValue); + + Assert.That(readOnlyValue.Value, Is.EqualTo(42)); + } + + [Test] + public void DelegateReadOnlyRefCannotBeModified() + { + var readOnlyValue = new Struct(42); + + var subs = Substitute.For(); + subs.When(x => x.Invoke(Arg.Any())).Do(c => { c[0] = new Struct(24); }); + + subs.Invoke(in readOnlyValue); + + Assert.That(readOnlyValue.Value, Is.EqualTo(42)); + } + + [Test] + public void DelegateMultipleReadOnlyRefCannotBeModified() + { + var readOnlyValue1 = new Struct(42); + var readOnlyValue2 = new Struct(42); + + var subs = Substitute.For(); + subs.When(x => x.Invoke(Arg.Any(), Arg.Any())) + .Do(c => { c[0] = new Struct(24); c[1] = new Struct(24); }); + + subs.Invoke(in readOnlyValue1, in readOnlyValue2); + + Assert.That(readOnlyValue1.Value, Is.EqualTo(42)); + Assert.That(readOnlyValue2.Value, Is.EqualTo(42)); } } } \ No newline at end of file