Skip to content

Commit 15fcb99

Browse files
authored
Fix thread-safety issues with enumerating ResourceManager. (#75054)
* 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
1 parent 6c2b939 commit 15fcb99

File tree

7 files changed

+704
-90
lines changed

7 files changed

+704
-90
lines changed

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

Lines changed: 91 additions & 65 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
using System.IO;
1010
using System.Runtime.CompilerServices;
1111
using System.Text;
12+
using System.Threading;
1213

1314
namespace System.Resources
1415
#if RESOURCES_EXTENSIONS
@@ -60,10 +61,11 @@ public sealed partial class
6061
// it make sense to use anything less than one page?
6162
private const int DefaultFileStreamBufferSize = 4096;
6263

63-
private BinaryReader _store; // backing store we're reading from.
64-
// Used by RuntimeResourceSet and this class's enumerator. Maps
65-
// resource name to a value, a ResourceLocator, or a
66-
// LooselyLinkedManifestResource.
64+
// Backing store we're reading from. Usages outside of constructor
65+
// initialization must be protected by lock (this).
66+
private BinaryReader _store;
67+
// Used by RuntimeResourceSet and this class's enumerator.
68+
// Accesses must be protected by lock(_resCache).
6769
internal Dictionary<string, ResourceLocator>? _resCache;
6870
private long _nameSectionOffset; // Offset to name section of file.
6971
private long _dataSectionOffset; // Offset to Data section of file.
@@ -88,7 +90,6 @@ public sealed partial class
8890
// Version number of .resources file, for compatibility
8991
private int _version;
9092

91-
9293
public
9394
#if RESOURCES_EXTENSIONS
9495
DeserializingResourceReader(string fileName)
@@ -173,13 +174,16 @@ private unsafe void Dispose(bool disposing)
173174
}
174175
}
175176

176-
internal static unsafe int ReadUnalignedI4(int* p)
177+
private static unsafe int ReadUnalignedI4(int* p)
177178
{
178179
return BinaryPrimitives.ReadInt32LittleEndian(new ReadOnlySpan<byte>(p, sizeof(int)));
179180
}
180181

181182
private void SkipString()
182183
{
184+
// Note: this method assumes that it is called either during object
185+
// construction or within another method that locks on this.
186+
183187
int stringLength = _store.Read7BitEncodedInt();
184188
if (stringLength < 0)
185189
{
@@ -238,6 +242,7 @@ public IDictionaryEnumerator GetEnumerator()
238242
return new ResourceEnumerator(this);
239243
}
240244

245+
// Called from RuntimeResourceSet
241246
internal ResourceEnumerator GetEnumeratorInternal()
242247
{
243248
return new ResourceEnumerator(this);
@@ -247,6 +252,7 @@ internal ResourceEnumerator GetEnumeratorInternal()
247252
// To read the data, seek to _dataSectionOffset + dataPos, then
248253
// read the resource type & data.
249254
// This does a binary search through the names.
255+
// Called from RuntimeResourceSet
250256
internal int FindPosForResource(string name)
251257
{
252258
Debug.Assert(_store != null, "ResourceReader is closed!");
@@ -331,6 +337,8 @@ internal int FindPosForResource(string name)
331337
private unsafe bool CompareStringEqualsName(string name)
332338
{
333339
Debug.Assert(_store != null, "ResourceReader is closed!");
340+
Debug.Assert(Monitor.IsEntered(this)); // uses _store
341+
334342
int byteLen = _store.Read7BitEncodedInt();
335343
if (byteLen < 0)
336344
{
@@ -463,68 +471,74 @@ private unsafe string AllocateStringForNameIndex(int index, out int dataOffset)
463471
}
464472

465473
// This takes a virtual offset into the data section and reads a String
466-
// from that location.
467-
// Anyone who calls LoadObject should make sure they take a lock so
468-
// no one can cause us to do a seek in here.
474+
// from that location. Called from RuntimeResourceSet
469475
internal string? LoadString(int pos)
470476
{
471477
Debug.Assert(_store != null, "ResourceReader is closed!");
472-
_store.BaseStream.Seek(_dataSectionOffset + pos, SeekOrigin.Begin);
473-
string? s = null;
474-
int typeIndex = _store.Read7BitEncodedInt();
475-
if (_version == 1)
476-
{
477-
if (typeIndex == -1)
478-
return null;
479-
if (FindType(typeIndex) != typeof(string))
480-
throw new InvalidOperationException(SR.Format(SR.InvalidOperation_ResourceNotString_Type, FindType(typeIndex).FullName));
481-
s = _store.ReadString();
482-
}
483-
else
478+
479+
lock (this)
484480
{
485-
ResourceTypeCode typeCode = (ResourceTypeCode)typeIndex;
486-
if (typeCode != ResourceTypeCode.String && typeCode != ResourceTypeCode.Null)
481+
_store.BaseStream.Seek(_dataSectionOffset + pos, SeekOrigin.Begin);
482+
string? s = null;
483+
int typeIndex = _store.Read7BitEncodedInt();
484+
if (_version == 1)
487485
{
488-
string? typeString;
489-
if (typeCode < ResourceTypeCode.StartOfUserTypes)
490-
typeString = typeCode.ToString();
491-
else
492-
typeString = FindType(typeCode - ResourceTypeCode.StartOfUserTypes).FullName;
493-
throw new InvalidOperationException(SR.Format(SR.InvalidOperation_ResourceNotString_Type, typeString));
494-
}
495-
if (typeCode == ResourceTypeCode.String) // ignore Null
486+
if (typeIndex == -1)
487+
return null;
488+
if (FindType(typeIndex) != typeof(string))
489+
throw new InvalidOperationException(SR.Format(SR.InvalidOperation_ResourceNotString_Type, FindType(typeIndex).FullName));
496490
s = _store.ReadString();
491+
}
492+
else
493+
{
494+
ResourceTypeCode typeCode = (ResourceTypeCode)typeIndex;
495+
if (typeCode != ResourceTypeCode.String && typeCode != ResourceTypeCode.Null)
496+
{
497+
string? typeString;
498+
if (typeCode < ResourceTypeCode.StartOfUserTypes)
499+
typeString = typeCode.ToString();
500+
else
501+
typeString = FindType(typeCode - ResourceTypeCode.StartOfUserTypes).FullName;
502+
throw new InvalidOperationException(SR.Format(SR.InvalidOperation_ResourceNotString_Type, typeString));
503+
}
504+
if (typeCode == ResourceTypeCode.String) // ignore Null
505+
s = _store.ReadString();
506+
}
507+
return s;
497508
}
498-
return s;
499509
}
500510

501511
// Called from RuntimeResourceSet
502512
internal object? LoadObject(int pos)
503513
{
504-
if (_version == 1)
505-
return LoadObjectV1(pos);
506-
return LoadObjectV2(pos, out _);
514+
lock (this)
515+
{
516+
return _version == 1 ? LoadObjectV1(pos) : LoadObjectV2(pos, out _);
517+
}
507518
}
508519

520+
// Called from RuntimeResourceSet
509521
internal object? LoadObject(int pos, out ResourceTypeCode typeCode)
510522
{
511-
if (_version == 1)
523+
lock (this)
512524
{
513-
object? o = LoadObjectV1(pos);
514-
typeCode = (o is string) ? ResourceTypeCode.String : ResourceTypeCode.StartOfUserTypes;
515-
return o;
525+
if (_version == 1)
526+
{
527+
object? o = LoadObjectV1(pos);
528+
typeCode = (o is string) ? ResourceTypeCode.String : ResourceTypeCode.StartOfUserTypes;
529+
return o;
530+
}
531+
return LoadObjectV2(pos, out typeCode);
516532
}
517-
return LoadObjectV2(pos, out typeCode);
518533
}
519534

520535
// This takes a virtual offset into the data section and reads an Object
521536
// from that location.
522-
// Anyone who calls LoadObject should make sure they take a lock so
523-
// no one can cause us to do a seek in here.
524-
internal object? LoadObjectV1(int pos)
537+
private object? LoadObjectV1(int pos)
525538
{
526539
Debug.Assert(_store != null, "ResourceReader is closed!");
527540
Debug.Assert(_version == 1, ".resources file was not a V1 .resources file!");
541+
Debug.Assert(Monitor.IsEntered(this)); // uses _store
528542

529543
try
530544
{
@@ -544,6 +558,8 @@ private unsafe string AllocateStringForNameIndex(int index, out int dataOffset)
544558

545559
private object? _LoadObjectV1(int pos)
546560
{
561+
Debug.Assert(Monitor.IsEntered(this)); // uses _store
562+
547563
_store.BaseStream.Seek(_dataSectionOffset + pos, SeekOrigin.Begin);
548564
int typeIndex = _store.Read7BitEncodedInt();
549565
if (typeIndex == -1)
@@ -600,10 +616,11 @@ private unsafe string AllocateStringForNameIndex(int index, out int dataOffset)
600616
}
601617
}
602618

603-
internal object? LoadObjectV2(int pos, out ResourceTypeCode typeCode)
619+
private object? LoadObjectV2(int pos, out ResourceTypeCode typeCode)
604620
{
605621
Debug.Assert(_store != null, "ResourceReader is closed!");
606622
Debug.Assert(_version >= 2, ".resources file was not a V2 (or higher) .resources file!");
623+
Debug.Assert(Monitor.IsEntered(this)); // uses _store
607624

608625
try
609626
{
@@ -623,6 +640,8 @@ private unsafe string AllocateStringForNameIndex(int index, out int dataOffset)
623640

624641
private object? _LoadObjectV2(int pos, out ResourceTypeCode typeCode)
625642
{
643+
Debug.Assert(Monitor.IsEntered(this)); // uses _store
644+
626645
_store.BaseStream.Seek(_dataSectionOffset + pos, SeekOrigin.Begin);
627646
typeCode = (ResourceTypeCode)_store.Read7BitEncodedInt();
628647

@@ -759,6 +778,7 @@ private unsafe string AllocateStringForNameIndex(int index, out int dataOffset)
759778
[MemberNotNull(nameof(_typeNamePositions))]
760779
private void ReadResources()
761780
{
781+
Debug.Assert(!Monitor.IsEntered(this)); // only called during init
762782
Debug.Assert(_store != null, "ResourceReader is closed!");
763783

764784
try
@@ -781,6 +801,8 @@ private void ReadResources()
781801
[MemberNotNull(nameof(_typeNamePositions))]
782802
private void _ReadResources()
783803
{
804+
Debug.Assert(!Monitor.IsEntered(this)); // only called during init
805+
784806
// Read ResourceManager header
785807
// Check for magic number
786808
int magicNum = _store.ReadInt32();
@@ -968,6 +990,8 @@ private Type FindType(int typeIndex)
968990
"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.")]
969991
private Type UseReflectionToGetType(int typeIndex)
970992
{
993+
Debug.Assert(Monitor.IsEntered(this)); // uses _store
994+
971995
long oldPos = _store.BaseStream.Position;
972996
try
973997
{
@@ -1006,6 +1030,8 @@ private Type UseReflectionToGetType(int typeIndex)
10061030
private string TypeNameFromTypeCode(ResourceTypeCode typeCode)
10071031
{
10081032
Debug.Assert(typeCode >= 0, "can't be negative");
1033+
Debug.Assert(Monitor.IsEntered(this)); // uses _store
1034+
10091035
if (typeCode < ResourceTypeCode.StartOfUserTypes)
10101036
{
10111037
Debug.Assert(!string.Equals(typeCode.ToString(), "LastPrimitive"), "Change ResourceTypeCode metadata order so LastPrimitive isn't what Enum.ToString prefers.");
@@ -1083,31 +1109,31 @@ public DictionaryEntry Entry
10831109
if (!_currentIsValid) throw new InvalidOperationException(SR.InvalidOperation_EnumNotStarted);
10841110
if (_reader._resCache == null) throw new InvalidOperationException(SR.ResourceReaderIsClosed);
10851111

1086-
string key;
1112+
string key = _reader.AllocateStringForNameIndex(_currentName, out _dataPosition); // AllocateStringForNameIndex could lock on _reader
1113+
10871114
object? value = null;
1088-
lock (_reader)
1089-
{ // locks should be taken in the same order as in RuntimeResourceSet.GetObject to avoid deadlock
1090-
lock (_reader._resCache)
1115+
// 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).
1116+
// Lock order MUST match RuntimeResourceSet.GetObject to avoid deadlock.
1117+
Debug.Assert(!Monitor.IsEntered(_reader));
1118+
lock (_reader._resCache)
1119+
{
1120+
if (_reader._resCache.TryGetValue(key, out ResourceLocator locator))
10911121
{
1092-
key = _reader.AllocateStringForNameIndex(_currentName, out _dataPosition); // AllocateStringForNameIndex could lock on _reader
1093-
if (_reader._resCache.TryGetValue(key, out ResourceLocator locator))
1094-
{
1095-
value = locator.Value;
1096-
}
1097-
if (value == null)
1098-
{
1099-
if (_dataPosition == -1)
1100-
value = _reader.GetValueForNameIndex(_currentName);
1101-
else
1102-
value = _reader.LoadObject(_dataPosition);
1103-
// If enumeration and subsequent lookups happen very
1104-
// frequently in the same process, add a ResourceLocator
1105-
// to _resCache here. But WinForms enumerates and
1106-
// just about everyone else does lookups. So caching
1107-
// here may bloat working set.
1108-
}
1122+
value = locator.Value;
11091123
}
11101124
}
1125+
if (value is null)
1126+
{
1127+
if (_dataPosition == -1)
1128+
value = _reader.GetValueForNameIndex(_currentName);
1129+
else
1130+
value = _reader.LoadObject(_dataPosition);
1131+
// If enumeration and subsequent lookups happen very
1132+
// frequently in the same process, add a ResourceLocator
1133+
// to _resCache here (we'll also need to extend the lock block!).
1134+
// But WinForms enumerates and just about everyone else does lookups.
1135+
// So caching here may bloat working set.
1136+
}
11111137
return new DictionaryEntry(key, value);
11121138
}
11131139
}

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

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
using System.Collections.Generic;
66
using System.Diagnostics;
77
using System.IO;
8+
using System.Threading;
89

910
namespace System.Resources
1011
#if RESOURCES_EXTENSIONS
@@ -287,6 +288,9 @@ private IDictionaryEnumerator GetEnumeratorHelper()
287288
object? value;
288289
ResourceLocator resEntry;
289290

291+
// Lock the cache first, then the reader (reader locks implicitly through its methods).
292+
// Lock order MUST match ResourceReader.ResourceEnumerator.Entry to avoid deadlock.
293+
Debug.Assert(!Monitor.IsEntered(reader));
290294
lock (cache)
291295
{
292296
// Find the offset within the data section
@@ -299,7 +303,7 @@ private IDictionaryEnumerator GetEnumeratorHelper()
299303

300304
// When data type cannot be cached
301305
dataPos = resEntry.DataPosition;
302-
return isString ? reader.LoadString(dataPos) : reader.LoadObject(dataPos, out _);
306+
return isString ? reader.LoadString(dataPos) : reader.LoadObject(dataPos);
303307
}
304308

305309
dataPos = reader.FindPosForResource(key);
@@ -357,14 +361,11 @@ private IDictionaryEnumerator GetEnumeratorHelper()
357361
return value;
358362
}
359363

360-
private static object? ReadValue (ResourceReader reader, int dataPos, bool isString, out ResourceLocator locator)
364+
private static object? ReadValue(ResourceReader reader, int dataPos, bool isString, out ResourceLocator locator)
361365
{
362366
object? value;
363367
ResourceTypeCode typeCode;
364368

365-
// Normally calling LoadString or LoadObject requires
366-
// taking a lock. Note that in this case, we took a
367-
// lock before calling this method.
368369
if (isString)
369370
{
370371
value = reader.LoadString(dataPos);

0 commit comments

Comments
 (0)