Skip to content

Commit d75e5b3

Browse files
jonorossijonorossi
committed
Replace NullReferenceException thrown when interceptors swallow exceptions and cause a null value type to be returned
1 parent 98b37bc commit d75e5b3

File tree

6 files changed

+158
-5
lines changed

6 files changed

+158
-5
lines changed

‎CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ Bugfixes:
1616
- Fix TraceLoggerFactory to allow specifying the default logger level (@acjh, #342)
1717
- Ensure that DynamicProxy doesn't create invalid dynamic assemblies when proxying types from non-strong-named assemblies (@stakx, #327)
1818
- Fix interceptor selectors being passed `System.RuntimeType` for class proxies instead of the target type (@stakx, #359)
19+
- Replace NullReferenceException with descriptive one thrown when interceptors swallow exceptions and cause a null value type to be returned (@jonorossi, #85)
1920

2021
## 4.2.1 (2017-10-11)
2122

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

Lines changed: 117 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,117 @@
1+
// Copyright 2004-2018 Castle Project - http://www.castleproject.org/
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License");
4+
// you may not use this file except in compliance with the License.
5+
// You may obtain a copy of the License at
6+
//
7+
// http://www.apache.org/licenses/LICENSE-2.0
8+
//
9+
// Unless required by applicable law or agreed to in writing, software
10+
// distributed under the License is distributed on an "AS IS" BASIS,
11+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
// See the License for the specific language governing permissions and
13+
// limitations under the License.
14+
15+
namespace Castle.DynamicProxy.Tests
16+
{
17+
using System;
18+
19+
using NUnit.Framework;
20+
21+
// The purpose of this test fixture is to ensure that DynamicProxy throws a useful exception
22+
// if an interceptor fails to perform its job to ensure that a non-void method that returns
23+
// a value type must return either a value or throw an exception.
24+
[TestFixture]
25+
public class InterceptorsMustReturnNonNullValueTypesTestCase : BasePEVerifyTestCase
26+
{
27+
[Test]
28+
public void Proxy_method_without_interceptors_returns_struct_or_throws()
29+
{
30+
var proxy = generator.CreateClassProxy<Class>();
31+
32+
Assert.AreEqual(1, proxy.IntReturn());
33+
Exception ex = Assert.Throws<Exception>(() => proxy.IntThrow());
34+
Assert.AreEqual("Throw from Class", ex.Message);
35+
36+
Assert.AreEqual(null, proxy.NullableIntReturn());
37+
ex = Assert.Throws<Exception>(() => proxy.NullableIntThrow());
38+
Assert.AreEqual("Throw from Class", ex.Message);
39+
}
40+
41+
[Test]
42+
public void Proxy_method_that_swallows_exception_and_returns_null_struct()
43+
{
44+
var interceptor = new SwallowExceptionInterceptor();
45+
var proxy = generator.CreateClassProxy<Class>(interceptor);
46+
47+
Assert.AreEqual(1, proxy.IntReturn());
48+
Exception ex = Assert.Throws<InvalidOperationException>(() => proxy.IntThrow());
49+
Assert.AreEqual("Interceptors failed to set a return value, or swallowed the exception thrown by the target", ex.Message);
50+
51+
Assert.AreEqual(null, proxy.NullableIntReturn());
52+
Assert.AreEqual(null, proxy.NullableIntThrow());
53+
}
54+
55+
[Test]
56+
public void Proxy_method_that_returns_clears_return_value_or_throws()
57+
{
58+
var interceptor = new ReturnNullValueInterceptor(); // Clears the ReturnValue
59+
var proxy = generator.CreateClassProxy<Class>(interceptor);
60+
61+
Exception ex = Assert.Throws<InvalidOperationException>(() => proxy.IntReturn());
62+
Assert.AreEqual("Interceptors failed to set a return value, or swallowed the exception thrown by the target", ex.Message);
63+
Assert.Throws<Exception>(() => proxy.IntThrow(), "Throw from Class");
64+
65+
Assert.AreEqual(null, proxy.NullableIntReturn());
66+
ex = Assert.Throws<Exception>(() => proxy.NullableIntThrow());
67+
Assert.AreEqual("Throw from Class", ex.Message);
68+
}
69+
70+
public class Class
71+
{
72+
public virtual int IntReturn()
73+
{
74+
return 1;
75+
}
76+
77+
public virtual int IntThrow()
78+
{
79+
throw new Exception("Throw from Class"); // Throw an exception without returning any result
80+
}
81+
82+
public virtual int? NullableIntReturn()
83+
{
84+
return null;
85+
}
86+
87+
public virtual int? NullableIntThrow()
88+
{
89+
throw new Exception("Throw from Class"); // Throw an exception without returning any result
90+
}
91+
}
92+
93+
public class SwallowExceptionInterceptor : IInterceptor
94+
{
95+
public void Intercept(IInvocation invocation)
96+
{
97+
try
98+
{
99+
invocation.Proceed();
100+
}
101+
catch (Exception)
102+
{
103+
// Swallow the exception, leaving invocation.ReturnValue=null
104+
}
105+
}
106+
}
107+
108+
public class ReturnNullValueInterceptor : IInterceptor
109+
{
110+
public void Intercept(IInvocation invocation)
111+
{
112+
invocation.Proceed(); // If this throws, ReturnValue will remain null
113+
invocation.ReturnValue = null;
114+
}
115+
}
116+
}
117+
}

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -231,7 +231,7 @@ private void CopyDefaultValueConstant(ParameterInfo from, ParameterBuilder to)
231231

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

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

Lines changed: 19 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,24 +14,41 @@
1414

1515
namespace Castle.DynamicProxy.Generators.Emitters.SimpleAST
1616
{
17+
using System;
1718
using System.Reflection.Emit;
1819

1920
public class IfNullExpression : Expression
2021
{
2122
private readonly IILEmitter ifNotNull;
2223
private readonly IILEmitter ifNull;
2324
private readonly Reference reference;
25+
private readonly Expression expression;
2426

2527
public IfNullExpression(Reference reference, IILEmitter ifNull, IILEmitter ifNotNull = null)
2628
{
27-
this.reference = reference;
29+
this.reference = reference ?? throw new ArgumentNullException(nameof(reference));
30+
this.ifNull = ifNull;
31+
this.ifNotNull = ifNotNull;
32+
}
33+
34+
public IfNullExpression(Expression expression, IILEmitter ifNull, IILEmitter ifNotNull = null)
35+
{
36+
this.expression = expression ?? throw new ArgumentNullException(nameof(expression));
2837
this.ifNull = ifNull;
2938
this.ifNotNull = ifNotNull;
3039
}
3140

3241
public override void Emit(IMemberEmitter member, ILGenerator gen)
3342
{
34-
ArgumentsUtil.EmitLoadOwnerAndReference(reference, gen);
43+
if (reference != null)
44+
{
45+
ArgumentsUtil.EmitLoadOwnerAndReference(reference, gen);
46+
}
47+
else if (expression != null)
48+
{
49+
expression.Emit(member, gen);
50+
}
51+
3552
var notNull = gen.DefineLabel();
3653
gen.Emit(OpCodes.Brtrue_S, notNull);
3754
ifNull.Emit(member, gen);

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

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@ namespace Castle.DynamicProxy.Generators
2626
using Castle.DynamicProxy.Contributors;
2727
using Castle.DynamicProxy.Generators.Emitters;
2828
using Castle.DynamicProxy.Generators.Emitters.SimpleAST;
29+
using Castle.DynamicProxy.Internal;
2930
using Castle.DynamicProxy.Tokens;
3031

3132
public class MethodWithInvocationGenerator : MethodGenerator
@@ -135,8 +136,19 @@ protected override MethodEmitter BuildProxiedMethodBody(MethodEmitter emitter, C
135136

136137
if (MethodToOverride.ReturnType != typeof(void))
137138
{
138-
// Emit code to return with cast from ReturnValue
139139
var getRetVal = new MethodInvocationExpression(invocationLocal, InvocationMethods.GetReturnValue);
140+
141+
// Emit code to ensure a value type return type is not null, otherwise the cast will cause a null-deref
142+
if (emitter.ReturnType.GetTypeInfo().IsValueType && !emitter.ReturnType.IsNullableType())
143+
{
144+
LocalReference returnValue = emitter.CodeBuilder.DeclareLocal(typeof(object));
145+
emitter.CodeBuilder.AddStatement(new AssignStatement(returnValue, getRetVal));
146+
147+
emitter.CodeBuilder.AddExpression(new IfNullExpression(returnValue, new ThrowStatement(typeof(InvalidOperationException),
148+
"Interceptors failed to set a return value, or swallowed the exception thrown by the target")));
149+
}
150+
151+
// Emit code to return with cast from ReturnValue
140152
emitter.CodeBuilder.AddStatement(new ReturnStatement(new ConvertExpression(emitter.ReturnType, getRetVal)));
141153
}
142154
else

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

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,12 @@ namespace Castle.DynamicProxy.Internal
2424

2525
public static class TypeUtil
2626
{
27+
public static bool IsNullableType(this Type type)
28+
{
29+
return type.GetTypeInfo().IsGenericType &&
30+
type.GetGenericTypeDefinition() == typeof(Nullable<>);
31+
}
32+
2733
public static FieldInfo[] GetAllFields(this Type type)
2834
{
2935
if (type == null)

0 commit comments

Comments
 (0)