Skip to content

Fix thread-safety issues with enumerating ResourceManager. #75054

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Jan 3, 2023
Merged

Fix thread-safety issues with enumerating ResourceManager. #75054

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
862e7fe
Fix thread-safety issues with enumerating ResourceManager.
madelson Sep 3, 2022
0c274c1
Remove trailing whitespace
madelson Sep 3, 2022
5d99bf6
Address feedback from https://github.com/dotnet/runtime/pull/75054
madelson Nov 9, 2022
a8a8b61
Add comment in response to https://github.com/dotnet/runtime/pull/750…
madelson Nov 17, 2022
a6278d0
Merge remote-tracking branch 'origin/main' into fix-74052-and-74868
madelson Dec 10, 2022
eea9c08
Implement feedback from https://github.com/dotnet/runtime/pull/75054
madelson Dec 11, 2022
7307995
Merge branch 'dotnet:main' into fix-74052-and-74868
madelson Dec 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
156 changes: 91 additions & 65 deletions src/libraries/System.Private.CoreLib/src/System/Resources/ResourceReader.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
using System.IO;
using System.Runtime.CompilerServices;
using System.Text;
using System.Threading;

namespace System.Resources
#if RESOURCES_EXTENSIONS
Expand Down Expand Up @@ -60,10 +61,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.
Expand All @@ -88,7 +90,6 @@ public sealed partial class
// Version number of .resources file, for compatibility
private int _version;


public
#if RESOURCES_EXTENSIONS
DeserializingResourceReader(string fileName)
Expand Down Expand Up @@ -173,13 +174,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)
{
Expand Down Expand Up @@ -238,6 +242,7 @@ public IDictionaryEnumerator GetEnumerator()
return new ResourceEnumerator(this);
}

// Called from RuntimeResourceSet
internal ResourceEnumerator GetEnumeratorInternal()
{
return new ResourceEnumerator(this);
Expand All @@ -247,6 +252,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!");
Expand Down Expand Up @@ -331,6 +337,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)
{
Expand Down Expand Up @@ -463,68 +471,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
{
Expand All @@ -544,6 +558,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)
Expand Down Expand Up @@ -600,10 +616,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
{
Expand All @@ -623,6 +640,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();

Expand Down Expand Up @@ -759,6 +778,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
Expand All @@ -781,6 +801,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();
Expand Down Expand Up @@ -968,6 +990,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
{
Expand Down Expand Up @@ -1006,6 +1030,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.");
Expand Down Expand Up @@ -1083,31 +1109,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);
}
}
Expand Down
11 changes: 6 additions & 5 deletions src/libraries/System.Private.CoreLib/src/System/Resources/RuntimeResourceSet.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
using System.Collections.Generic;
using System.Diagnostics;
using System.IO;
using System.Threading;

namespace System.Resources
#if RESOURCES_EXTENSIONS
Expand Down Expand Up @@ -287,6 +288,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
Expand All @@ -299,7 +303,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);
Expand Down Expand Up @@ -357,14 +361,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.
Copy link
Contributor Author

@madelson madelson Sep 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment was not (no longer?) correct. We do typically call these within a lock, but it is a lock on either _resCache or _caseInsensitiveTable. That lock was previously making things thread-safe so long as the caller was only using GetString/GetObject and was not flipping ResourceManager.IgnoreCase on and off.

Now, LoadString/LoadObject do lock internally so there is no edge case here.

if (isString)
{
value = reader.LoadString(dataPos);
Expand Down
Loading