Merge pull request #364 from castleproject/interceptor-returning-null…

…-value-type

Replace NullReferenceException thrown when interceptors swallow exceptions and cause a null value type to be returned

CHANGELOG.md

@@ -16,6 +16,7 @@ Bugfixes:
 - Fix TraceLoggerFactory to allow specifying the default logger level (@acjh, #342)
 - Ensure that DynamicProxy doesn't create invalid dynamic assemblies when proxying types from non-strong-named assemblies (@stakx, #327)
 - Fix interceptor selectors being passed `System.RuntimeType` for class proxies instead of the target type (@stakx, #359)
+- Replace NullReferenceException with descriptive one thrown when interceptors swallow exceptions and cause a null value type to be returned (@jonorossi, #85)
 
 ## 4.2.1 (2017-10-11)
 

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

@@ -0,0 +1,117 @@
+// Copyright 2004-2018 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.
+
+namespace Castle.DynamicProxy.Tests
+{
+	using System;
+
+	using NUnit.Framework;
+
+	// The purpose of this test fixture is to ensure that DynamicProxy throws a useful exception
+	// if an interceptor fails to perform its job to ensure that a non-void method that returns
+	// a value type must return either a value or throw an exception.
+	[TestFixture]
+	public class InterceptorsMustReturnNonNullValueTypesTestCase : BasePEVerifyTestCase
+	{
+		[Test]
+		public void Proxy_method_without_interceptors_returns_struct_or_throws()
+		{
+			var proxy = generator.CreateClassProxy<Class>();
+
+			Assert.AreEqual(1, proxy.IntReturn());
+			Exception ex = Assert.Throws<Exception>(() => proxy.IntThrow());
+			Assert.AreEqual("Throw from Class", ex.Message);
+
+			Assert.AreEqual(null, proxy.NullableIntReturn());
+			ex = Assert.Throws<Exception>(() => proxy.NullableIntThrow());
+			Assert.AreEqual("Throw from Class", ex.Message);
+		}
+
+		[Test]
+		public void Proxy_method_that_swallows_exception_and_returns_null_struct()
+		{
+			var interceptor = new SwallowExceptionInterceptor();
+			var proxy = generator.CreateClassProxy<Class>(interceptor);
+
+			Assert.AreEqual(1, proxy.IntReturn());
+			Exception ex = Assert.Throws<InvalidOperationException>(() => proxy.IntThrow());
+			Assert.AreEqual("Interceptors failed to set a return value, or swallowed the exception thrown by the target", ex.Message);
+
+			Assert.AreEqual(null, proxy.NullableIntReturn());
+			Assert.AreEqual(null, proxy.NullableIntThrow());
+		}
+
+		[Test]
+		public void Proxy_method_that_returns_clears_return_value_or_throws()
+		{
+			var interceptor = new ReturnNullValueInterceptor(); // Clears the ReturnValue
+			var proxy = generator.CreateClassProxy<Class>(interceptor);
+
+			Exception ex = Assert.Throws<InvalidOperationException>(() => proxy.IntReturn());
+			Assert.AreEqual("Interceptors failed to set a return value, or swallowed the exception thrown by the target", ex.Message);
+			Assert.Throws<Exception>(() => proxy.IntThrow(), "Throw from Class");
+
+			Assert.AreEqual(null, proxy.NullableIntReturn());
+			ex = Assert.Throws<Exception>(() => proxy.NullableIntThrow());
+			Assert.AreEqual("Throw from Class", ex.Message);
+		}
+
+		public class Class
+		{
+			public virtual int IntReturn()
+			{
+				return 1;
+			}
+
+			public virtual int IntThrow()
+			{
+				throw new Exception("Throw from Class"); // Throw an exception without returning any result
+			}
+
+			public virtual int? NullableIntReturn()
+			{
+				return null;
+			}
+
+			public virtual int? NullableIntThrow()
+			{
+				throw new Exception("Throw from Class"); // Throw an exception without returning any result
+			}
+		}
+
+		public class SwallowExceptionInterceptor : IInterceptor
+		{
+			public void Intercept(IInvocation invocation)
+			{
+				try
+				{
+					invocation.Proceed();
+				}
+				catch (Exception)
+				{
+					// Swallow the exception, leaving invocation.ReturnValue=null
+				}
+			}
+		}
+
+		public class ReturnNullValueInterceptor : IInterceptor
+		{
+			public void Intercept(IInvocation invocation)
+			{
+				invocation.Proceed(); // If this throws, ReturnValue will remain null
+				invocation.ReturnValue = null;
+			}
+		}
+	}
+}

src/Castle.Core/DynamicProxy/Generators/Emitters/MethodEmitter.cs

@@ -231,7 +231,7 @@ private void CopyDefaultValueConstant(ParameterInfo from, ParameterBuilder to)
 
 				if (defaultValue == null)
 				{
-					if (parameterType.GetTypeInfo().IsGenericType && parameterType.GetGenericTypeDefinition() == typeof(Nullable<>))
+					if (parameterType.IsNullableType())
 					{
 						// This guards against a Mono bug that prohibits setting default value `null`
 						// for a `Nullable<T>` parameter. See https://github.com/mono/mono/issues/8504.
@@ -254,7 +254,7 @@ private void CopyDefaultValueConstant(ParameterInfo from, ParameterBuilder to)
 						return;
 					}
 				}
-				else if (parameterType.GetTypeInfo().IsGenericType && parameterType.GetGenericTypeDefinition() == typeof(Nullable<>))
+				else if (parameterType.IsNullableType())
 				{
 					var genericArg = from.ParameterType.GetGenericArguments()[0];
 					if (genericArg.GetTypeInfo().IsEnum || genericArg.IsAssignableFrom(defaultValue.GetType()))

src/Castle.Core/DynamicProxy/Generators/Emitters/SimpleAST/IfNullExpression.cs

@@ -14,24 +14,41 @@
 
 namespace Castle.DynamicProxy.Generators.Emitters.SimpleAST
 {
+	using System;
 	using System.Reflection.Emit;
 
 	public class IfNullExpression : Expression
 	{
 		private readonly IILEmitter ifNotNull;
 		private readonly IILEmitter ifNull;
 		private readonly Reference reference;
+		private readonly Expression expression;
 
 		public IfNullExpression(Reference reference, IILEmitter ifNull, IILEmitter ifNotNull = null)
 		{
-			this.reference = reference;
+			this.reference = reference ?? throw new ArgumentNullException(nameof(reference));
+			this.ifNull = ifNull;
+			this.ifNotNull = ifNotNull;
+		}
+
+		public IfNullExpression(Expression expression, IILEmitter ifNull, IILEmitter ifNotNull = null)
+		{
+			this.expression = expression ?? throw new ArgumentNullException(nameof(expression));
 			this.ifNull = ifNull;
 			this.ifNotNull = ifNotNull;
 		}
 
 		public override void Emit(IMemberEmitter member, ILGenerator gen)
 		{
-			ArgumentsUtil.EmitLoadOwnerAndReference(reference, gen);
+			if (reference != null)
+			{
+				ArgumentsUtil.EmitLoadOwnerAndReference(reference, gen);
+			}
+			else if (expression != null)
+			{
+				expression.Emit(member, gen);
+			}
+
 			var notNull = gen.DefineLabel();
 			gen.Emit(OpCodes.Brtrue_S, notNull);
 			ifNull.Emit(member, gen);

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

@@ -26,6 +26,7 @@ namespace Castle.DynamicProxy.Generators
 	using Castle.DynamicProxy.Contributors;
 	using Castle.DynamicProxy.Generators.Emitters;
 	using Castle.DynamicProxy.Generators.Emitters.SimpleAST;
+	using Castle.DynamicProxy.Internal;
 	using Castle.DynamicProxy.Tokens;
 
 	public class MethodWithInvocationGenerator : MethodGenerator
@@ -135,8 +136,19 @@ protected override MethodEmitter BuildProxiedMethodBody(MethodEmitter emitter, C
 
 			if (MethodToOverride.ReturnType != typeof(void))
 			{
-				// Emit code to return with cast from ReturnValue
 				var getRetVal = new MethodInvocationExpression(invocationLocal, InvocationMethods.GetReturnValue);
+
+				// Emit code to ensure a value type return type is not null, otherwise the cast will cause a null-deref
+				if (emitter.ReturnType.GetTypeInfo().IsValueType && !emitter.ReturnType.IsNullableType())
+				{
+					LocalReference returnValue = emitter.CodeBuilder.DeclareLocal(typeof(object));
+					emitter.CodeBuilder.AddStatement(new AssignStatement(returnValue, getRetVal));
+
+					emitter.CodeBuilder.AddExpression(new IfNullExpression(returnValue, new ThrowStatement(typeof(InvalidOperationException),
+						"Interceptors failed to set a return value, or swallowed the exception thrown by the target")));
+				}
+
+				// Emit code to return with cast from ReturnValue
 				emitter.CodeBuilder.AddStatement(new ReturnStatement(new ConvertExpression(emitter.ReturnType, getRetVal)));
 			}
 			else

src/Castle.Core/DynamicProxy/Internal/TypeUtil.cs

@@ -24,6 +24,12 @@ namespace Castle.DynamicProxy.Internal
 
 	public static class TypeUtil
 	{
+		public static bool IsNullableType(this Type type)
+		{
+			return type.GetTypeInfo().IsGenericType &&
+			       type.GetGenericTypeDefinition() == typeof(Nullable<>);
+		}
+
 		public static FieldInfo[] GetAllFields(this Type type)
 		{
 			if (type == null)