DynamicProxy Throw NullReferenceException When Intercepted Method Throw Eaten Exception

[Charles-521]

The issue will happen all of the below condition met:

1. The intercepted method has return value.
2. The intercepted method throws an exception.
3. The above exception is handled in _public void Intercept(IInvocation invocation)_ and it is NOT re-thrown.

We put our logging logic when the exception happens and since we do not intend to crash the application. Therefore such exception does not re-thrown.

Currently I can workaround this by putting below code in _finally block_:

if (!invocation.Method.ReturnType.IsClass
    && !invocation.Method.ReturnType.IsInterface
    && invocation.Method.ReturnType.Name != "Void"
    && invocation.Method.ReturnParameter != null
    && invocation.ReturnValue == null)
        invocation.ReturnValue = Activator.CreateInstance(invocation.Method.ReturnType);

However, it might be better to handle this in the framework itself.

[kkozmic-seek]

I have looked at this a long time ago (BTW, this only affects value type return values and out parameters) but baking it into the framework would be quite tricky and add performance overhead that's unnecessary 99.99% of the time.

A better option might be catching the NullReferenceException and rethrowing something more descriptive but that too seemed quite tricky to get right.

But then, by all means, if you want to start a pull request and have a crack at it (preferably the second option though) I'm happy to help out

[Charles-521]

Thanks for the clarification. I can't re-throw it as we do not want to have try-catch applied on intercepted method. It then become an unhandled exception.

I'd like to help out here, do you mind give me some hint on which part of the code that I should look into? It seems that the exception is thrown at a dynamic generated proxy method which I am not familiar with the pattern yet.

[kkozmic-seek]

Sure. Can you start with some tests to demonstrate the issue and expected behaviour?

[Charles-521]

Sure, I will add the code in Castle.Core.Tests. The workaround is updated for anyone who like to have a quick fix.

[Charles-521]

I've added the test code under Castle.Core.Tests/BugsReported, However, the code fails to run, the error indicates that: Inheritance security rules violated while overriding member

I am quite sure that previous Castle.Core.Tests does not have this problem. The issue happens on existing unit tests too, it seems none of them could run. Is there anything that I missed?

Below is the code that I added as DynProxy85.cs:

// Copyright 2004-2010 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 CastleTests.BugsReported
{
    #region

    using Castle.DynamicProxy;
    using NUnit.Framework;
    using System;
    using System.Security;

    #endregion

    [TestFixture]
    public class DynProxy85 : BasePEVerifyTestCase
    {
        public class Test85<T> where T : struct
        {
            public virtual T ReturnValueTypeMethod()
            {
                //To throw an exception without return any result
                throw new InvalidOperationException();
            }
        }

        public class LogInterceptor : IInterceptor
        {
            public void Intercept(IInvocation invocation)
            {
                try
                {
                    invocation.Proceed();
                }
                catch (Exception)
                {
                    //Eat the exception and perform logging here
                }
            }
        }

        [Test]
        [ExpectedException(typeof(NullReferenceException))]
        public void ShouldThrowNullRefException()
        {
            var interceptor = new LogInterceptor();
            var proxy = generator.CreateClassProxy<Test85<Guid>>(interceptor);
            proxy.ReturnValueTypeMethod();
        }
    }
}

[kkozmic-seek]

what configuration are you building it with? Try .NET 4.0 or 4.5 configuration. Also please make sure there are no #regions in the code

[Charles-521]

Thank you @kkozmic-seek , I didn't change any setting, the .NET configuration is the original project setting from repo which is .NET 4.5. I will remove the #region. Out of curiosity, is there any issue having #region around using statement? It is added by resharper.

[Charles-521]

I think the problem is the ReSharper NUnit Plugin for Visual Studio 2013, there is no problem by using nunit directly.

[kkozmic]

Cool. As for regions we avoid them altogether in our codebase. Resharper
should pick up our settings if you run full file reformat

On Fri, 24 Apr 2015 17:27 Charles-521 notifications@github.com wrote:

I think the problem is the ReSharper NUnit Plugin for Visual Studio 2013,
there is no problem by using nunit directly.

—
Reply to this email directly or view it on GitHub
#85 (comment).

[stakx]

IMHO, this is a user error, and not a fault with DynamicProxy: If you create a proxy without a target, then the interceptors are responsible for "implementing" the proxied methods' behaviour. In the above example, if the interceptor chooses to swallow any exception, then it's still responsible for "completing" the invocation. So it must either throw a new exception, or set the return value.

Basically, the above code makes the same mistake as the following (only it surfaces at run-time instead of at compile-time):

int GetSomeValue()
{
    try
    {
        throw new InvalidOperationException();
    }
    catch
    {
    }
    // compile-time error: method does not return a value.
}

(Granted though, the type of exception thrown, NullReferenceException, unfortunately isn't a very good hint at what causes the problem.)

[jonorossi]

@stakx after reading this thread again I agree, and just as both @kkozmic and yourself mentioned the exception could be much more helpful.

I've submitted a pull request to improve the exception for return values, and also included if an interceptor plays with out argument values by setting one to null which also resulted in a null deref. See #364.

[jonorossi]

Just merged #364 that replaces the null ref exception with something more useful, it is still the job of an interceptor to set a return value if it wants to swallow exceptions.