Ensure parameters with 'in' modifier cannot be modified

src/NSubstitute/NSubstitute.csproj

@@ -36,7 +36,7 @@
  </PropertyGroup>
 
  <ItemGroup>
- <PackageReference Include="Castle.Core" Version="4.3.0-*" />
+ <PackageReference Include="Castle.Core" Version="4.3.1-*" />
  <PackageReference Include="System.Threading.Tasks.Extensions" Version="4.3.0-*" />
  </ItemGroup>
 

src/NSubstitute/Proxies/CastleDynamicProxy/CastleDynamicProxyFactory.cs

@@ -46,12 +46,19 @@ public object GenerateProxy(ICallRouter callRouter, Type typeToProxy, Type[] add
  }
 
  /// <summary>
- /// Allows to dynamically create a type in runtime. Returns an instance of <see cref="TypeBuilder"/>,
- /// so type could be customized and built later.
+ /// Allows to dynamically create a type in runtime.
+ /// Use the <paramref name="typeBuildCallback"/> callback to define and build the type.
  /// </summary>
- public TypeBuilder DefineDynamicType(string typeName, TypeAttributes flags)
+ /// <param name="typeBuildCallback">Callback used to construct the type.</param>
+ /// <returns>The result returned by <paramref name="typeBuildCallback"/> callback.</returns>
+ public Type DefineDynamicType(Func<ModuleBuilder, Type> 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,

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<Type, Type> _delegateContainerCache = new ConcurrentDictionary<Type, Type>();
  private long _typeSuffixCounter;
@@ -39,7 +40,7 @@ public object GenerateProxy(ICallRouter callRouter, Type typeToProxy, Type[] add
  return DelegateProxy(typeToProxy, callRouter);
  }
 
- private bool HasItems<T>(T[] array)
+ private static bool HasItems<T>(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();
  }
  }
 }

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
  /// </summary>
  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<IStructByRefConsumer>();
+ var value = new Struct(42);
+
+ var subs = Substitute.For<IStructByReadOnlyRefConsumer>();
+ subs.Consume(in value);
  }
 
  [Test]
  public void IStructByValueConsumer_Test()
  {
- _ = Substitute.For<IStructByValueConsumer>();
+ var value = new Struct(42);
+
+ var subs = Substitute.For<IStructByValueConsumer>();
+ subs.Consume(value);
+ }
+
+ [Test]
+ public void DelegateByReadOnlyRefConsumer_Test()
+ {
+ var value = new Struct(42);
+
+ var subs = Substitute.For<DelegateStructByReadOnlyRefConsumer>();
+ subs.Invoke(in value);
+ }
+
+ [Test]
+ public void InterfaceReadOnlyRefCannotBeModified()
+ {
+ var readOnlyValue = new Struct(42);
+
+ var subs = Substitute.For<IStructByReadOnlyRefConsumer>();
+ subs.When(x => x.Consume(Arg.Any<Struct>())).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<DelegateStructByReadOnlyRefConsumer>();
+ subs.When(x => x.Invoke(Arg.Any<Struct>())).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<DelegateStructByReadOnlyRefConsumerMultipleArgs>();
+ subs.When(x => x.Invoke(Arg.Any<Struct>(), Arg.Any<Struct>()))
+ .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));
  }
  }
 }