[release/6.0] Fix thread-safety issues with enumerating ResourceManager. (#81281)

* Fix thread-safety issues with enumerating ResourceManager.

Concurrently enumerating a ResourceManager while also calling GetString()
and similar methods was prone to both transient errors and deadlock.

The transient errors were caused by RuntimeResourceSet calling internal
methods on ResourceReader that did not properly lock. Now, all exposed
methods on the Reader are thread-safe.

The deadlock was called by inconsistent lock ordering between
ResourceReader.ResourceEnumerator and RuntimeResourceSet which both lock
on the RuntimeResourceSet's cache and on the ResourceReader itself. Now,
the enumerator does not need to take both locks at the same time.

Fix #74052
Fix #74868

* Remove trailing whitespace

* Address feedback from #75054

* Add comment in response to #75054 (comment)

* Implement feedback from #75054

* Increase timeout for TestResourceManagerIsSafeForConcurrentAccessAndEnumeration (#80330)

This raises the timeout to 30s, the same as what we have for the equivalent ResourceManager test (https://github.com/dotnet/runtime/blob/15fcb990fe17348ab6ddde0939200b900169920b/src/libraries/System.Resources.ResourceManager/tests/ResourceManagerTests.cs#L255).

fix #80277

---------

Co-authored-by: Michael Adelson <[email protected]>
Co-authored-by: madelson <[email protected]>
Co-authored-by: Carlos Sanchez <[email protected]>
Co-authored-by: Buyaa Namnan <[email protected]>

Author: 4 people

src/libraries/System.Private.CoreLib/src/System/Resources/ResourceReader.cs

@@ -9,6 +9,7 @@
 using System.IO;
 using System.Runtime.CompilerServices;
 using System.Text;
+using System.Threading;
 
 namespace System.Resources
 #if RESOURCES_EXTENSIONS
@@ -58,10 +59,11 @@ public sealed partial class
         // it make sense to use anything less than one page?
         private const int DefaultFileStreamBufferSize = 4096;
 
-        private BinaryReader _store;    // backing store we're reading from.
-        // Used by RuntimeResourceSet and this class's enumerator.  Maps
-        // resource name to a value, a ResourceLocator, or a
-        // LooselyLinkedManifestResource.
+        // Backing store we're reading from. Usages outside of constructor
+        // initialization must be protected by lock (this).
+        private BinaryReader _store;
+        // Used by RuntimeResourceSet and this class's enumerator.
+        // Accesses must be protected by lock(_resCache).
         internal Dictionary<string, ResourceLocator>? _resCache;
         private long _nameSectionOffset;  // Offset to name section of file.
         private long _dataSectionOffset;  // Offset to Data section of file.
@@ -86,7 +88,6 @@ public sealed partial class
         // Version number of .resources file, for compatibility
         private int _version;
 
-
         public
 #if RESOURCES_EXTENSIONS
         DeserializingResourceReader(string fileName)
@@ -163,13 +164,16 @@ private unsafe void Dispose(bool disposing)
             }
         }
 
-        internal static unsafe int ReadUnalignedI4(int* p)
+        private static unsafe int ReadUnalignedI4(int* p)
         {
             return BinaryPrimitives.ReadInt32LittleEndian(new ReadOnlySpan<byte>(p, sizeof(int)));
         }
 
         private void SkipString()
         {
+            // Note: this method assumes that it is called either during object
+            // construction or within another method that locks on this.
+
             int stringLength = _store.Read7BitEncodedInt();
             if (stringLength < 0)
             {
@@ -228,6 +232,7 @@ public IDictionaryEnumerator GetEnumerator()
             return new ResourceEnumerator(this);
         }
 
+        // Called from RuntimeResourceSet
         internal ResourceEnumerator GetEnumeratorInternal()
         {
             return new ResourceEnumerator(this);
@@ -237,6 +242,7 @@ internal ResourceEnumerator GetEnumeratorInternal()
         // To read the data, seek to _dataSectionOffset + dataPos, then
         // read the resource type & data.
         // This does a binary search through the names.
+        // Called from RuntimeResourceSet
         internal int FindPosForResource(string name)
         {
             Debug.Assert(_store != null, "ResourceReader is closed!");
@@ -321,6 +327,8 @@ internal int FindPosForResource(string name)
         private unsafe bool CompareStringEqualsName(string name)
         {
             Debug.Assert(_store != null, "ResourceReader is closed!");
+            Debug.Assert(Monitor.IsEntered(this)); // uses _store
+
             int byteLen = _store.Read7BitEncodedInt();
             if (byteLen < 0)
             {
@@ -453,68 +461,74 @@ private unsafe string AllocateStringForNameIndex(int index, out int dataOffset)
         }
 
         // This takes a virtual offset into the data section and reads a String
-        // from that location.
-        // Anyone who calls LoadObject should make sure they take a lock so
-        // no one can cause us to do a seek in here.
+        // from that location. Called from RuntimeResourceSet
         internal string? LoadString(int pos)
         {
             Debug.Assert(_store != null, "ResourceReader is closed!");
-            _store.BaseStream.Seek(_dataSectionOffset + pos, SeekOrigin.Begin);
-            string? s = null;
-            int typeIndex = _store.Read7BitEncodedInt();
-            if (_version == 1)
-            {
-                if (typeIndex == -1)
-                    return null;
-                if (FindType(typeIndex) != typeof(string))
-                    throw new InvalidOperationException(SR.Format(SR.InvalidOperation_ResourceNotString_Type, FindType(typeIndex).FullName));
-                s = _store.ReadString();
-            }
-            else
+
+            lock (this)
             {
-                ResourceTypeCode typeCode = (ResourceTypeCode)typeIndex;
-                if (typeCode != ResourceTypeCode.String && typeCode != ResourceTypeCode.Null)
+                _store.BaseStream.Seek(_dataSectionOffset + pos, SeekOrigin.Begin);
+                string? s = null;
+                int typeIndex = _store.Read7BitEncodedInt();
+                if (_version == 1)
                 {
-                    string? typeString;
-                    if (typeCode < ResourceTypeCode.StartOfUserTypes)
-                        typeString = typeCode.ToString();
-                    else
-                        typeString = FindType(typeCode - ResourceTypeCode.StartOfUserTypes).FullName;
-                    throw new InvalidOperationException(SR.Format(SR.InvalidOperation_ResourceNotString_Type, typeString));
-                }
-                if (typeCode == ResourceTypeCode.String) // ignore Null
+                    if (typeIndex == -1)
+                        return null;
+                    if (FindType(typeIndex) != typeof(string))
+                        throw new InvalidOperationException(SR.Format(SR.InvalidOperation_ResourceNotString_Type, FindType(typeIndex).FullName));
                     s = _store.ReadString();
+                }
+                else
+                {
+                    ResourceTypeCode typeCode = (ResourceTypeCode)typeIndex;
+                    if (typeCode != ResourceTypeCode.String && typeCode != ResourceTypeCode.Null)
+                    {
+                        string? typeString;
+                        if (typeCode < ResourceTypeCode.StartOfUserTypes)
+                            typeString = typeCode.ToString();
+                        else
+                            typeString = FindType(typeCode - ResourceTypeCode.StartOfUserTypes).FullName;
+                        throw new InvalidOperationException(SR.Format(SR.InvalidOperation_ResourceNotString_Type, typeString));
+                    }
+                    if (typeCode == ResourceTypeCode.String) // ignore Null
+                        s = _store.ReadString();
+                }
+                return s;
             }
-            return s;
         }
 
         // Called from RuntimeResourceSet
         internal object? LoadObject(int pos)
         {
-            if (_version == 1)
-                return LoadObjectV1(pos);
-            return LoadObjectV2(pos, out _);
+            lock (this)
+            {
+                return _version == 1 ? LoadObjectV1(pos) : LoadObjectV2(pos, out _);
+            }
         }
 
+        // Called from RuntimeResourceSet
         internal object? LoadObject(int pos, out ResourceTypeCode typeCode)
         {
-            if (_version == 1)
+            lock (this)
             {
-                object? o = LoadObjectV1(pos);
-                typeCode = (o is string) ? ResourceTypeCode.String : ResourceTypeCode.StartOfUserTypes;
-                return o;
+                if (_version == 1)
+                {
+                    object? o = LoadObjectV1(pos);
+                    typeCode = (o is string) ? ResourceTypeCode.String : ResourceTypeCode.StartOfUserTypes;
+                    return o;
+                }
+                return LoadObjectV2(pos, out typeCode);
             }
-            return LoadObjectV2(pos, out typeCode);
         }
 
         // This takes a virtual offset into the data section and reads an Object
         // from that location.
-        // Anyone who calls LoadObject should make sure they take a lock so
-        // no one can cause us to do a seek in here.
-        internal object? LoadObjectV1(int pos)
+        private object? LoadObjectV1(int pos)
         {
             Debug.Assert(_store != null, "ResourceReader is closed!");
             Debug.Assert(_version == 1, ".resources file was not a V1 .resources file!");
+            Debug.Assert(Monitor.IsEntered(this)); // uses _store
 
             try
             {
@@ -534,6 +548,8 @@ private unsafe string AllocateStringForNameIndex(int index, out int dataOffset)
 
         private object? _LoadObjectV1(int pos)
         {
+            Debug.Assert(Monitor.IsEntered(this)); // uses _store
+
             _store.BaseStream.Seek(_dataSectionOffset + pos, SeekOrigin.Begin);
             int typeIndex = _store.Read7BitEncodedInt();
             if (typeIndex == -1)
@@ -586,10 +602,11 @@ private unsafe string AllocateStringForNameIndex(int index, out int dataOffset)
             }
         }
 
-        internal object? LoadObjectV2(int pos, out ResourceTypeCode typeCode)
+        private object? LoadObjectV2(int pos, out ResourceTypeCode typeCode)
         {
             Debug.Assert(_store != null, "ResourceReader is closed!");
             Debug.Assert(_version >= 2, ".resources file was not a V2 (or higher) .resources file!");
+            Debug.Assert(Monitor.IsEntered(this)); // uses _store
 
             try
             {
@@ -609,6 +626,8 @@ private unsafe string AllocateStringForNameIndex(int index, out int dataOffset)
 
         private object? _LoadObjectV2(int pos, out ResourceTypeCode typeCode)
         {
+            Debug.Assert(Monitor.IsEntered(this)); // uses _store
+
             _store.BaseStream.Seek(_dataSectionOffset + pos, SeekOrigin.Begin);
             typeCode = (ResourceTypeCode)_store.Read7BitEncodedInt();
 
@@ -745,6 +764,7 @@ private unsafe string AllocateStringForNameIndex(int index, out int dataOffset)
         [MemberNotNull(nameof(_typeNamePositions))]
         private void ReadResources()
         {
+            Debug.Assert(!Monitor.IsEntered(this)); // only called during init
             Debug.Assert(_store != null, "ResourceReader is closed!");
 
             try
@@ -767,6 +787,8 @@ private void ReadResources()
         [MemberNotNull(nameof(_typeNamePositions))]
         private void _ReadResources()
         {
+            Debug.Assert(!Monitor.IsEntered(this)); // only called during init
+
             // Read ResourceManager header
             // Check for magic number
             int magicNum = _store.ReadInt32();
@@ -952,6 +974,8 @@ private Type FindType(int typeIndex)
             "Custom readers as well as custom objects on the resources file are not observable by the trimmer and so required assemblies, types and members may be removed.")]
         private Type UseReflectionToGetType(int typeIndex)
         {
+            Debug.Assert(Monitor.IsEntered(this)); // uses _store
+
             long oldPos = _store.BaseStream.Position;
             try
             {
@@ -984,6 +1008,8 @@ private Type UseReflectionToGetType(int typeIndex)
         private string TypeNameFromTypeCode(ResourceTypeCode typeCode)
         {
             Debug.Assert(typeCode >= 0, "can't be negative");
+            Debug.Assert(Monitor.IsEntered(this)); // uses _store
+
             if (typeCode < ResourceTypeCode.StartOfUserTypes)
             {
                 Debug.Assert(!string.Equals(typeCode.ToString(), "LastPrimitive"), "Change ResourceTypeCode metadata order so LastPrimitive isn't what Enum.ToString prefers.");
@@ -1061,31 +1087,31 @@ public DictionaryEntry Entry
                     if (!_currentIsValid) throw new InvalidOperationException(SR.InvalidOperation_EnumNotStarted);
                     if (_reader._resCache == null) throw new InvalidOperationException(SR.ResourceReaderIsClosed);
 
-                    string key;
+                    string key = _reader.AllocateStringForNameIndex(_currentName, out _dataPosition); // AllocateStringForNameIndex could lock on _reader
+
                     object? value = null;
-                    lock (_reader)
-                    { // locks should be taken in the same order as in RuntimeResourceSet.GetObject to avoid deadlock
-                        lock (_reader._resCache)
+                    // Lock the cache first, then the reader (in this case, we don't actually need to lock the reader and cache at the same time).
+                    // Lock order MUST match RuntimeResourceSet.GetObject to avoid deadlock.
+                    Debug.Assert(!Monitor.IsEntered(_reader));
+                    lock (_reader._resCache)
+                    {
+                        if (_reader._resCache.TryGetValue(key, out ResourceLocator locator))
                         {
-                            key = _reader.AllocateStringForNameIndex(_currentName, out _dataPosition); // AllocateStringForNameIndex could lock on _reader
-                            if (_reader._resCache.TryGetValue(key, out ResourceLocator locator))
-                            {
-                                value = locator.Value;
-                            }
-                            if (value == null)
-                            {
-                                if (_dataPosition == -1)
-                                    value = _reader.GetValueForNameIndex(_currentName);
-                                else
-                                    value = _reader.LoadObject(_dataPosition);
-                                // If enumeration and subsequent lookups happen very
-                                // frequently in the same process, add a ResourceLocator
-                                // to _resCache here.  But WinForms enumerates and
-                                // just about everyone else does lookups.  So caching
-                                // here may bloat working set.
-                            }
+                            value = locator.Value;
                         }
                     }
+                    if (value is null)
+                    {
+                        if (_dataPosition == -1)
+                            value = _reader.GetValueForNameIndex(_currentName);
+                        else
+                            value = _reader.LoadObject(_dataPosition);
+                        // If enumeration and subsequent lookups happen very
+                        // frequently in the same process, add a ResourceLocator
+                        // to _resCache here (we'll also need to extend the lock block!).
+                        // But WinForms enumerates and just about everyone else does lookups.
+                        // So caching here may bloat working set.
+                    }
                     return new DictionaryEntry(key, value);
                 }
             }

src/libraries/System.Private.CoreLib/src/System/Resources/RuntimeResourceSet.cs

@@ -5,6 +5,7 @@
 using System.Collections.Generic;
 using System.Diagnostics;
 using System.IO;
+using System.Threading;
 
 namespace System.Resources
 #if RESOURCES_EXTENSIONS
@@ -276,6 +277,9 @@ private IDictionaryEnumerator GetEnumeratorHelper()
             object? value;
             ResourceLocator resEntry;
 
+            // Lock the cache first, then the reader (reader locks implicitly through its methods).
+            // Lock order MUST match ResourceReader.ResourceEnumerator.Entry to avoid deadlock.
+            Debug.Assert(!Monitor.IsEntered(reader));
             lock (cache)
             {
                 // Find the offset within the data section
@@ -288,7 +292,7 @@ private IDictionaryEnumerator GetEnumeratorHelper()
 
                     // When data type cannot be cached
                     dataPos = resEntry.DataPosition;
-                    return isString ? reader.LoadString(dataPos) : reader.LoadObject(dataPos, out _);
+                    return isString ? reader.LoadString(dataPos) : reader.LoadObject(dataPos);
                 }
 
                 dataPos = reader.FindPosForResource(key);
@@ -346,14 +350,11 @@ private IDictionaryEnumerator GetEnumeratorHelper()
             return value;
         }
 
-        private static object? ReadValue (ResourceReader reader, int dataPos, bool isString, out ResourceLocator locator)
+        private static object? ReadValue(ResourceReader reader, int dataPos, bool isString, out ResourceLocator locator)
         {
             object? value;
             ResourceTypeCode typeCode;
 
-            // Normally calling LoadString or LoadObject requires
-            // taking a lock.  Note that in this case, we took a
-            // lock before calling this method.
             if (isString)
             {
                 value = reader.LoadString(dataPos);