Fix #5807 - Race condition in Invoker

This change addressed a race condition in the ObjectMethodExecutor where
the default argument values array can become visible before it is
initialized. If a second observer accesses the array while it is being
initialized, it can observe a null value for a reference type parameter,
leading to a nullref.

The fix here is to make everything immutable and initialize it all up
front. There's no reason to create an OME without eventually running it,
so there's no downside to doing the initialization up front.

src/Microsoft.AspNetCore.Mvc.Core/Internal/ObjectMethodExecutor.cs

@@ -8,26 +8,26 @@ using System.Linq;
 using System.Linq.Expressions;
 using System.Reflection;
 using System.Threading.Tasks;
-using Microsoft.AspNetCore.Mvc.Core;
 using Microsoft.Extensions.Internal;
 
 namespace Microsoft.AspNetCore.Mvc.Internal
 {
     public class ObjectMethodExecutor
     {
-        private object[] _parameterDefaultValues;
+        private readonly object[] _parameterDefaultValues;
-        private ActionExecutorAsync _executorAsync;
+        private readonly ActionExecutorAsync _executorAsync;
-        private ActionExecutor _executor;
+        private readonly ActionExecutor _executor;
 
         private static readonly MethodInfo _convertOfTMethod =
             typeof(ObjectMethodExecutor).GetRuntimeMethods().Single(methodInfo => methodInfo.Name == nameof(ObjectMethodExecutor.Convert));
 
         private ObjectMethodExecutor(MethodInfo methodInfo, TypeInfo targetTypeInfo)
-        {            
+        {
             if (methodInfo == null)
             {
                 throw new ArgumentNullException(nameof(methodInfo));
             }
+
             MethodInfo = methodInfo;
             TargetTypeInfo = targetTypeInfo;
             ActionParameters = methodInfo.GetParameters();
@@ -35,6 +35,20 @@ namespace Microsoft.AspNetCore.Mvc.Internal
             IsMethodAsync = typeof(Task).IsAssignableFrom(MethodReturnType);
             TaskGenericType = IsMethodAsync ? GetTaskInnerTypeOrNull(MethodReturnType) : null;
             IsTypeAssignableFromIActionResult = typeof(IActionResult).IsAssignableFrom(TaskGenericType ?? MethodReturnType);
+
+            if (IsMethodAsync && TaskGenericType != null)
+            {
+                // For backwards compatibility we're creating a sync-executor for an async method. This was
+                // supported in the past even though MVC wouldn't have called it.
+                _executor = GetExecutor(methodInfo, targetTypeInfo);
+                _executorAsync = GetExecutorAsync(TaskGenericType, methodInfo, targetTypeInfo);
+            }
+            else
+            {
+                _executor = GetExecutor(methodInfo, targetTypeInfo);
+            }
+
+            _parameterDefaultValues = GetParameterDefaultValues(ActionParameters);
         }
 
         private delegate Task<object> ActionExecutorAsync(object target, object[] parameters);
@@ -58,29 +72,15 @@ namespace Microsoft.AspNetCore.Mvc.Internal
 
         public bool IsTypeAssignableFromIActionResult { get; }
 
-        private ActionExecutorAsync TaskOfTActionExecutorAsync
-        {
-            get
-            {
-                if (_executorAsync == null)
-                {
-                    _executorAsync = GetExecutorAsync(TaskGenericType, MethodInfo, TargetTypeInfo);
-                }
-
-                return _executorAsync;
-            }
-        }
-
         public static ObjectMethodExecutor Create(MethodInfo methodInfo, TypeInfo targetTypeInfo)
         {
             var executor = new ObjectMethodExecutor(methodInfo, targetTypeInfo);
-            executor._executor = GetExecutor(methodInfo, targetTypeInfo);
             return executor;
         }
 
         public Task<object> ExecuteAsync(object target, object[] parameters)
         {
-            return TaskOfTActionExecutorAsync(target, parameters);
+            return _executorAsync(target, parameters);
         }
 
         public object Execute(object target, object[] parameters)
@@ -95,8 +95,6 @@ namespace Microsoft.AspNetCore.Mvc.Internal
                 throw new ArgumentOutOfRangeException(nameof(index));
             }
 
-            EnsureParameterDefaultValues();
-
             return _parameterDefaultValues[index];
         }
 
@@ -216,42 +214,40 @@ namespace Microsoft.AspNetCore.Mvc.Internal
             return CastToObject<T>(task);
         }
 
-        private void EnsureParameterDefaultValues()
+        private static object[] GetParameterDefaultValues(ParameterInfo[] parameters)
         {
-            if (_parameterDefaultValues == null)
+            var values = new object[parameters.Length];
+
+            for (var i = 0; i < parameters.Length; i++)
             {
-                var count = ActionParameters.Length;
+                var parameterInfo = parameters[i];
-                _parameterDefaultValues = new object[count];
+                object defaultValue;
 
-                for (var i = 0; i < count; i++)
+                if (parameterInfo.HasDefaultValue)
                 {
-                    var parameterInfo = ActionParameters[i];
+                    defaultValue = parameterInfo.DefaultValue;
-                    object defaultValue;
+                }
+                else
+                {
+                    var defaultValueAttribute = parameterInfo
+                        .GetCustomAttribute<DefaultValueAttribute>(inherit: false);
 
-                    if (parameterInfo.HasDefaultValue)
+                    if (defaultValueAttribute?.Value == null)
                     {
-                        defaultValue = parameterInfo.DefaultValue;
+                        defaultValue = parameterInfo.ParameterType.GetTypeInfo().IsValueType
+                            ? Activator.CreateInstance(parameterInfo.ParameterType)
+                            : null;
                     }
                     else
                     {
-                        var defaultValueAttribute = parameterInfo
+                        defaultValue = defaultValueAttribute.Value;
-                            .GetCustomAttribute<DefaultValueAttribute>(inherit: false);
-
-                        if (defaultValueAttribute?.Value == null)
-                        {
-                            defaultValue = parameterInfo.ParameterType.GetTypeInfo().IsValueType
-                                ? Activator.CreateInstance(parameterInfo.ParameterType)
-                                : null;
-                        }
-                        else
-                        {
-                            defaultValue = defaultValueAttribute.Value;
-                        }
                     }
-
-                    _parameterDefaultValues[i] = defaultValue;
                 }
+
+                values[i] = defaultValue;
             }
+
+            return values;
         }
     }
 }