TaskFactoryWrapper: guard against multi-threaded access to dictionaries (#8928)

jdrobison · web-flow · commit edeacbbef9de · 2023-07-28T11:41:26.000+08:00
Fixes https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1824802 This change protects the dictionaries owned by TaskFactoryWrapper from being mutated while being accessed by multiple threads. Change TaskFactoryWrapper's dictionary fields from IDictionary<K,V> to IReadOnlyDictionary<K,V> Add support for IReadOnlyDictionary<K,V> to ReadOnlyEmptyDictionary Ensure mutually exclusive access to TaskFactoryWrapper.PopulatePropertyInfoCacheIfNecessary, which is the place where all of the object's dictionaries are created.
diff --git a/src/Build/BackEnd/TaskExecutionHost/TaskExecutionHost.cs b/src/Build/BackEnd/TaskExecutionHost/TaskExecutionHost.cs
@@ -333,7 +333,7 @@ bool ITaskExecutionHost.SetTaskParameters(IDictionary<string, (string, ElementLo
             // Get the properties that exist on this task.  We need to gather all of the ones that are marked
             // "required" so that we can keep track of whether or not they all get set.
             var setParameters = new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase);
-            IDictionary<string, string> requiredParameters = GetNamesOfPropertiesWithRequiredAttribute();
+            IReadOnlyDictionary<string, string> requiredParameters = GetNamesOfPropertiesWithRequiredAttribute();
 
             // look through all the attributes of the task element
             foreach (KeyValuePair<string, (string, ElementLocation)> parameter in parameters)
@@ -1540,10 +1540,10 @@ private void GatherArrayStringAndValueOutputs(bool outputTargetIsItem, string ou
         /// Returns them as keys in a dictionary.
         /// </summary>
         /// <returns>Gets a list of properties which are required.</returns>
-        private IDictionary<string, string> GetNamesOfPropertiesWithRequiredAttribute()
+        private IReadOnlyDictionary<string, string> GetNamesOfPropertiesWithRequiredAttribute()
         {
             ErrorUtilities.VerifyThrow(_taskFactoryWrapper != null, "Expected taskFactoryWrapper to not be null");
-            IDictionary<string, string> requiredParameters = null;
+            IReadOnlyDictionary<string, string> requiredParameters = null;
 
             try
             {
diff --git a/src/Build/Instance/TaskFactoryWrapper.cs b/src/Build/Instance/TaskFactoryWrapper.cs
@@ -8,8 +8,6 @@
 using Microsoft.Build.Framework;
 using Microsoft.Build.Shared;
 
-#nullable disable
-
 namespace Microsoft.Build.Execution
 {
     /// <summary>
@@ -19,30 +17,50 @@ internal sealed class TaskFactoryWrapper
     {
         #region Data
 
+        private struct PropertyData
+        {
+            /// <summary>
+            /// Cache of names of required properties on this type
+            /// </summary>
+            public readonly IReadOnlyDictionary<string, string> NamesOfPropertiesWithRequiredAttribute;
+
+            /// <summary>
+            /// Cache of names of output properties on this type
+            /// </summary>
+            public readonly IReadOnlyDictionary<string, string> NamesOfPropertiesWithOutputAttribute;
+
+            /// <summary>
+            /// Cache of names of properties on this type whose names are ambiguous
+            /// </summary>
+            public readonly IReadOnlyDictionary<string, string> NamesOfPropertiesWithAmbiguousMatches;
+
+            /// <summary>
+            /// Cache of PropertyInfos for this type
+            /// </summary>
+            public readonly IReadOnlyDictionary<string, TaskPropertyInfo> PropertyInfoCache;
+
+            public PropertyData(
+                IReadOnlyDictionary<string, string> namesOfPropertiesWithRequiredAttribute,
+                IReadOnlyDictionary<string, string> namesOfPropertiesWithOutputAttribute,
+                IReadOnlyDictionary<string, string> namesOfPropertiesWithAmbiguousMatches,
+                IReadOnlyDictionary<string, TaskPropertyInfo> propertyInfoCache)
+            {
+                NamesOfPropertiesWithRequiredAttribute = namesOfPropertiesWithRequiredAttribute;
+                NamesOfPropertiesWithOutputAttribute = namesOfPropertiesWithOutputAttribute;
+                NamesOfPropertiesWithAmbiguousMatches = namesOfPropertiesWithAmbiguousMatches;
+                PropertyInfoCache = propertyInfoCache;
+            }
+        }
+
         /// <summary>
         /// Factory which is wrapped by the wrapper
         /// </summary>
         private ITaskFactory _taskFactory;
 
         /// <summary>
-        /// Cache of names of required properties on this type
-        /// </summary>
-        private IDictionary<string, string> _namesOfPropertiesWithRequiredAttribute;
-
-        /// <summary>
-        /// Cache of names of output properties on this type
-        /// </summary>
-        private IDictionary<string, string> _namesOfPropertiesWithOutputAttribute;
-
-        /// <summary>
-        /// Cache of names of properties on this type whose names are ambiguous
-        /// </summary>
-        private IDictionary<string, string> _namesOfPropertiesWithAmbiguousMatches;
-
-        /// <summary>
-        /// Cache of PropertyInfos for this type
+        /// Wrapper of lazy initializable property data.
         /// </summary>
-        private IDictionary<string, TaskPropertyInfo> _propertyInfoCache;
+        private Lazy<PropertyData> _propertyData;
 
         /// <summary>
         /// The name of the task this factory can create.
@@ -70,6 +88,7 @@ internal TaskFactoryWrapper(ITaskFactory taskFactory, LoadedType taskFactoryLoad
             _taskName = taskName;
             TaskFactoryLoadedType = taskFactoryLoadInfo;
             _factoryIdentityParameters = factoryIdentityParameters;
+            _propertyData = new Lazy<PropertyData>(PopulatePropertyInfo);
         }
 
         #endregion
@@ -101,13 +120,11 @@ public ITaskFactory TaskFactory
         /// Caches the result - since it can't change during the build.
         /// </summary>
         /// <returns></returns>
-        public IDictionary<string, string> GetNamesOfPropertiesWithRequiredAttribute
+        public IReadOnlyDictionary<string, string> GetNamesOfPropertiesWithRequiredAttribute
         {
             get
             {
-                PopulatePropertyInfoCacheIfNecessary();
-
-                return _namesOfPropertiesWithRequiredAttribute;
+                return _propertyData.Value.NamesOfPropertiesWithRequiredAttribute;
             }
         }
 
@@ -116,13 +133,11 @@ public IDictionary<string, string> GetNamesOfPropertiesWithRequiredAttribute
         /// Caches the result - since it can't change during the build.
         /// </summary>
         /// <returns></returns>
-        public IDictionary<string, string> GetNamesOfPropertiesWithOutputAttribute
+        public IReadOnlyDictionary<string, string> GetNamesOfPropertiesWithOutputAttribute
         {
             get
             {
-                PopulatePropertyInfoCacheIfNecessary();
-
-                return _namesOfPropertiesWithOutputAttribute;
+                return _propertyData.Value.NamesOfPropertiesWithOutputAttribute;
             }
         }
 
@@ -158,18 +173,16 @@ public IDictionary<string, string> FactoryIdentityParameters
         /// </summary>
         /// <param name="propertyName">property name</param>
         /// <returns>PropertyInfo</returns>
-        public TaskPropertyInfo GetProperty(string propertyName)
+        public TaskPropertyInfo? GetProperty(string propertyName)
         {
-            PopulatePropertyInfoCacheIfNecessary();
-
-            TaskPropertyInfo propertyInfo;
-            if (!_propertyInfoCache.TryGetValue(propertyName, out propertyInfo))
+            TaskPropertyInfo? propertyInfo;
+            if (!_propertyData.Value.PropertyInfoCache.TryGetValue(propertyName, out propertyInfo))
             {
                 return null;
             }
             else
             {
-                if (_namesOfPropertiesWithAmbiguousMatches.ContainsKey(propertyName))
+                if (_propertyData.Value.NamesOfPropertiesWithAmbiguousMatches.ContainsKey(propertyName))
                 {
                     // See comment in PopulatePropertyInfoCache
                     throw new AmbiguousMatchException();
@@ -187,37 +200,37 @@ internal void SetPropertyValue(ITask task, TaskPropertyInfo property, object val
             ErrorUtilities.VerifyThrowArgumentNull(task, nameof(task));
             ErrorUtilities.VerifyThrowArgumentNull(property, nameof(property));
 
-            IGeneratedTask generatedTask = task as IGeneratedTask;
+            IGeneratedTask? generatedTask = task as IGeneratedTask;
             if (generatedTask != null)
             {
                 generatedTask.SetPropertyValue(property, value);
             }
             else
             {
                 ReflectableTaskPropertyInfo propertyInfo = (ReflectableTaskPropertyInfo)property;
-                propertyInfo.Reflection.SetValue(task, value, null);
+                propertyInfo.Reflection?.SetValue(task, value, null);
             }
         }
 
         /// <summary>
         /// Gets the value of a given property on the given task.
         /// </summary>
-        internal object GetPropertyValue(ITask task, TaskPropertyInfo property)
+        internal object? GetPropertyValue(ITask task, TaskPropertyInfo property)
         {
             ErrorUtilities.VerifyThrowArgumentNull(task, nameof(task));
             ErrorUtilities.VerifyThrowArgumentNull(property, nameof(property));
 
-            IGeneratedTask generatedTask = task as IGeneratedTask;
+            IGeneratedTask? generatedTask = task as IGeneratedTask;
             if (generatedTask != null)
             {
                 return generatedTask.GetPropertyValue(property);
             }
             else
             {
-                ReflectableTaskPropertyInfo propertyInfo = property as ReflectableTaskPropertyInfo;
+                ReflectableTaskPropertyInfo? propertyInfo = property as ReflectableTaskPropertyInfo;
                 if (propertyInfo != null)
                 {
-                    return propertyInfo.Reflection.GetValue(task, null);
+                    return propertyInfo.Reflection?.GetValue(task, null);
                 }
                 else
                 {
@@ -242,77 +255,79 @@ internal bool IsCreatableByFactory(string taskName)
         /// <summary>
         /// Populate the cache of PropertyInfos for this type
         /// </summary>
-        private void PopulatePropertyInfoCacheIfNecessary()
+        private PropertyData PopulatePropertyInfo()
         {
-            if (_propertyInfoCache == null)
+            Dictionary<string, TaskPropertyInfo>? propertyInfoCache = null;
+            Dictionary<string, string>? namesOfPropertiesWithRequiredAttribute = null;
+            Dictionary<string, string>? namesOfPropertiesWithOutputAttribute = null;
+            Dictionary<string, string>? namesOfPropertiesWithAmbiguousMatches = null;
+
+            bool taskTypeImplementsIGeneratedTask = typeof(IGeneratedTask).IsAssignableFrom(_taskFactory.TaskType);
+            TaskPropertyInfo[] propertyInfos = _taskFactory.GetTaskParameters();
+
+            for (int i = 0; i < propertyInfos.Length; i++)
             {
-                bool taskTypeImplementsIGeneratedTask = typeof(IGeneratedTask).IsAssignableFrom(_taskFactory.TaskType);
-                TaskPropertyInfo[] propertyInfos = _taskFactory.GetTaskParameters();
+                // If the task implements IGeneratedTask, we must use the TaskPropertyInfo the factory gives us.
+                // Otherwise, we never have to hand the TaskPropertyInfo back to the task or factory, so we replace
+                // theirs with one of our own that will allow us to cache reflection data per-property.
+                TaskPropertyInfo propertyInfo = propertyInfos[i];
+                if (!taskTypeImplementsIGeneratedTask)
+                {
+                    propertyInfo = new ReflectableTaskPropertyInfo(propertyInfo, _taskFactory.TaskType);
+                }
 
-                for (int i = 0; i < propertyInfos.Length; i++)
+                try
                 {
-                    // If the task implements IGeneratedTask, we must use the TaskPropertyInfo the factory gives us.
-                    // Otherwise, we never have to hand the TaskPropertyInfo back to the task or factory, so we replace
-                    // theirs with one of our own that will allow us to cache reflection data per-property.
-                    TaskPropertyInfo propertyInfo = propertyInfos[i];
-                    if (!taskTypeImplementsIGeneratedTask)
+                    if (propertyInfoCache == null)
                     {
-                        propertyInfo = new ReflectableTaskPropertyInfo(propertyInfo, _taskFactory.TaskType);
+                        propertyInfoCache = new Dictionary<string, TaskPropertyInfo>(StringComparer.OrdinalIgnoreCase);
                     }
 
-                    try
-                    {
-                        if (_propertyInfoCache == null)
-                        {
-                            _propertyInfoCache = new Dictionary<string, TaskPropertyInfo>(StringComparer.OrdinalIgnoreCase);
-                        }
-
-                        _propertyInfoCache.Add(propertyInfo.Name, propertyInfo);
-                    }
-                    catch (ArgumentException)
+                    propertyInfoCache.Add(propertyInfo.Name, propertyInfo);
+                }
+                catch (ArgumentException)
+                {
+                    // We have encountered a duplicate entry in our hashtable; if we had used BindingFlags.IgnoreCase this
+                    // would have produced an AmbiguousMatchException. In the old code, before this cache existed,
+                    // that wouldn't have been thrown unless and until the project actually tried to set this ambiguous parameter.
+                    // So rather than fail here, we store a list of ambiguous names and throw later, when one of them
+                    // is requested.
+                    if (namesOfPropertiesWithAmbiguousMatches == null)
                     {
-                        // We have encountered a duplicate entry in our hashtable; if we had used BindingFlags.IgnoreCase this
-                        // would have produced an AmbiguousMatchException. In the old code, before this cache existed,
-                        // that wouldn't have been thrown unless and until the project actually tried to set this ambiguous parameter.
-                        // So rather than fail here, we store a list of ambiguous names and throw later, when one of them
-                        // is requested.
-                        if (_namesOfPropertiesWithAmbiguousMatches == null)
-                        {
-                            _namesOfPropertiesWithAmbiguousMatches = new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase);
-                        }
-
-                        _namesOfPropertiesWithAmbiguousMatches[propertyInfo.Name] = String.Empty;
+                        namesOfPropertiesWithAmbiguousMatches = new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase);
                     }
 
-                    if (propertyInfos[i].Required)
-                    {
-                        if (_namesOfPropertiesWithRequiredAttribute == null)
-                        {
-                            _namesOfPropertiesWithRequiredAttribute = new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase);
-                        }
-
-                        // we have a require attribute defined, keep a record of that
-                        _namesOfPropertiesWithRequiredAttribute[propertyInfo.Name] = String.Empty;
-                    }
+                    namesOfPropertiesWithAmbiguousMatches[propertyInfo.Name] = String.Empty;
+                }
 
-                    if (propertyInfos[i].Output)
+                if (propertyInfos[i].Required)
+                {
+                    if (namesOfPropertiesWithRequiredAttribute == null)
                     {
-                        if (_namesOfPropertiesWithOutputAttribute == null)
-                        {
-                            _namesOfPropertiesWithOutputAttribute = new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase);
-                        }
-
-                        // we have a output attribute defined, keep a record of that
-                        _namesOfPropertiesWithOutputAttribute[propertyInfo.Name] = String.Empty;
+                        namesOfPropertiesWithRequiredAttribute = new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase);
                     }
+
+                    // we have a require attribute defined, keep a record of that
+                    namesOfPropertiesWithRequiredAttribute[propertyInfo.Name] = String.Empty;
                 }
 
-                _propertyInfoCache ??= ReadOnlyEmptyDictionary<string, TaskPropertyInfo>.Instance;
+                if (propertyInfos[i].Output)
+                {
+                    if (namesOfPropertiesWithOutputAttribute == null)
+                    {
+                        namesOfPropertiesWithOutputAttribute = new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase);
+                    }
 
-                _namesOfPropertiesWithRequiredAttribute ??= ReadOnlyEmptyDictionary<string, string>.Instance;
-                _namesOfPropertiesWithOutputAttribute ??= ReadOnlyEmptyDictionary<string, string>.Instance;
-                _namesOfPropertiesWithAmbiguousMatches ??= ReadOnlyEmptyDictionary<string, string>.Instance;
+                    // we have a output attribute defined, keep a record of that
+                    namesOfPropertiesWithOutputAttribute[propertyInfo.Name] = String.Empty;
+                }
             }
+
+            return new PropertyData(
+                (IReadOnlyDictionary<string, string>?)namesOfPropertiesWithRequiredAttribute ?? ReadOnlyEmptyDictionary<string, string>.Instance,
+                (IReadOnlyDictionary<string, string>?)namesOfPropertiesWithOutputAttribute ?? ReadOnlyEmptyDictionary<string, string>.Instance,
+                (IReadOnlyDictionary<string, string>?)namesOfPropertiesWithAmbiguousMatches ?? ReadOnlyEmptyDictionary<string, string>.Instance,
+                (IReadOnlyDictionary<string, TaskPropertyInfo>?)propertyInfoCache ?? ReadOnlyEmptyDictionary<string, TaskPropertyInfo>.Instance);
         }
         #endregion
     }
diff --git a/src/Shared/ReadOnlyEmptyDictionary.cs b/src/Shared/ReadOnlyEmptyDictionary.cs
@@ -16,7 +16,7 @@ namespace Microsoft.Build.Collections
     /// </summary>
     /// <typeparam name="K">Key</typeparam>
     /// <typeparam name="V">Value</typeparam>
-    internal class ReadOnlyEmptyDictionary<K, V> : IDictionary<K, V>, IDictionary
+    internal class ReadOnlyEmptyDictionary<K, V> : IDictionary<K, V>, IReadOnlyDictionary<K, V>, IDictionary
     {
         /// <summary>
         /// The single instance
@@ -127,6 +127,22 @@ ICollection IDictionary.Values
             get { return (ICollection)((IDictionary<K, V>)this).Values; }
         }
 
+        /// <summary>
+        /// Keys
+        /// </summary>
+        IEnumerable<K> IReadOnlyDictionary<K, V>.Keys
+        {
+            get { return Keys; }
+        }
+
+        /// <summary>
+        /// Values
+        /// </summary>
+        IEnumerable<V> IReadOnlyDictionary<K, V>.Values
+        {
+            get { return Values; }
+        }
+
         /// <summary>
         /// Indexer
         /// </summary>
@@ -292,3 +308,22 @@ public void CopyTo(System.Array array, int index)
         }
     }
 }
+
+#if NET35
+namespace System.Collections.Generic
+{
+    public interface IReadOnlyCollection<T> : IEnumerable<T>
+    {
+        int Count { get; }
+    }
+
+    public interface IReadOnlyDictionary<TKey, TValue> : IReadOnlyCollection<KeyValuePair<TKey, TValue>>
+    {
+        TValue this[TKey key] { get; }
+        IEnumerable<TKey> Keys { get; }
+        IEnumerable<TValue> Values { get; }
+        bool ContainsKey(TKey key);
+        bool TryGetValue(TKey key, out TValue value);
+    }
+}
+#endif
