[ResourceLoader] Collected assembly can cause AccessViolationException

[KevinCathcart]

The following code when run in .NET 5 (adjusting the assembly path if needed), will crash with an AccessViolation.

Since the user has written no unsafe code here, and is also not using any of the Unsafe equivlent framework methods, like some of the Marshal ones or the Unsafe class, and the user has not written any PInvokes or similar, it is not supposed to be possible to hit an access violation, meaning we have a "runtime" bug. However, the bug here is not in the native code, but actually in a managed code. Explanation will be found below the code.

void Main()
{
   // This is loosely based on the example code in https://docs.microsoft.com/en-us/dotnet/standard/assembly/unloadability
    
    string assembly = @"C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App\5.0.5\PresentationCore.dll";
    string basename = "PresentationCore.g";
    string resource = "fonts/globalserif.compositefont";
    
    var stream = GetResourceAndUnload(assembly, basename, resource, out var alc);

    // Wait for the load context to get collected (and thus assembly unloaded)
	for (int i = 0; alc.IsAlive && (i < 10); i++)
	{
		GC.Collect();
		GC.WaitForPendingFinalizers();
	}
	
	var test = new byte[10];
	stream.Read(test); // Reads unmapped memory, and dies.
}

[MethodImpl(MethodImplOptions.NoInlining)]
static Stream GetResourceAndUnload(string assemblyPath, string baseName, string resource, out WeakReference alcWeakRef)
{
	var alc = new TestAssemblyLoadContext();
	Assembly a = alc.LoadFromAssemblyPath(assemblyPath);

	alcWeakRef = new WeakReference(alc, trackResurrection: true);
    
    ResourceManager rm = new ResourceManager(baseName, a);

	var stream = rm.GetStream(resource);

	alc.Unload();

	return stream;
}

class TestAssemblyLoadContext : AssemblyLoadContext
{
	public TestAssemblyLoadContext() : base(isCollectible: true)
	{
	}

	protected override Assembly Load(AssemblyName name)
	{
		return null;
	}
}

The bug here is that an UmanagedMemoryStream pointing into the assembly itself is returned for the Resource, but nothing is keeping the assembly alive, so the assembly is subject to getting collected. Once collected, the file in unmapped, and trying to read from the pointer inside UnmanagedMemoryStream ends up reading unmapped memory, and this explodes.

This same basic bug was fixed for the RuntimeAssembly.GetManifestResourceStream case back in dotnet/coreclr#22925, but this case was not fixed.

The new UmanagedMemoryStream is actually created from the one returned by the previous method, so if it used a subclass that could keep the original stream around (similar to how the previous fix worked) that would prevent the unloading while the stream was still accessible. Or there could be other approaches too.

Note: I did not stumble upon this organically.

Tagging subscribers to this area: @vitek-karas, @agocke, @CoffeeFlux, @VSadov
See info in area-owners.md if you want to be subscribed.

Issue Details

---

The following code when run in .NET 5 (adjusting the assembly path if needed), will crash with an AccessViolation.

Since the user has written no unsafe code here, and is also not using any of the Unsafe equivlent framework methods, like some of the Marshal ones or the Unsafe class, and the user has not written any PInvokes or similar, it is not supposed to be possible to hit an access violation, meaning we have a "runtime" bug. However, the bug here is not in the native code, but actually in a managed code. Explanation will be found below the code.

void Main()
{
   // This is loosely based on the example code in https://docs.microsoft.com/en-us/dotnet/standard/assembly/unloadability
    
    string assembly = @"C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App\5.0.5\PresentationCore.dll";
    string basename = "PresentationCore.g";
    string resource = "fonts/globalserif.compositefont";
    
    var stream = GetResourceAndUnload(assembly, basename, resource, out var alc);

    // Wait for the load context to get collected (and thus assembly unloaded)
	for (int i = 0; alc.IsAlive && (i < 10); i++)
	{
		GC.Collect();
		GC.WaitForPendingFinalizers();
	}
	
	var test = new byte[10];
	stream.Read(test); // Reads unmapped memory, and dies.
}

[MethodImpl(MethodImplOptions.NoInlining)]
static Stream GetResourceAndUnload(string assemblyPath, string baseName, string resource, out WeakReference alcWeakRef)
{
	var alc = new TestAssemblyLoadContext();
	Assembly a = alc.LoadFromAssemblyPath(assemblyPath);

	alcWeakRef = new WeakReference(alc, trackResurrection: true);
    
    ResourceManager rm = new ResourceManager(baseName, a);

	var stream = rm.GetStream(resource);

	alc.Unload();

	return stream;
}

class TestAssemblyLoadContext : AssemblyLoadContext
{
	public TestAssemblyLoadContext() : base(isCollectible: true)
	{
	}

	protected override Assembly Load(AssemblyName name)
	{
		return null;
	}
}

The bug here is that an UmanagedMemoryStream pointing into the assembly itself is returned for the Resource, but nothing is keeping the assembly alive, so the assembly is subject to getting collected. Once collected, the file in unmapped, and trying to read from the pointer inside UnmanagedMemoryStream ends up reading unmapped memory, and this explodes.

This same basic bug was fixed for the RuntimeAssembly.GetManifestResourceStream case back in dotnet/coreclr#22925, but this case was not fixed.

The new UmanagedMemoryStream is actually created from the one returned by the previous method, so if it used a subclass that could keep the original stream around (similar to how the previous fix worked) that would prevent the unloading while the stream was still accessible. Or there could be other approaches too.

Note: I did not stumble upon this organically.

Author:	KevinCathcart
Assignees:	-
Labels:	area-AssemblyLoader-coreclr, untriaged
Milestone:	-

[KevinCathcart]

This also applies to DeserializingResourceReader, which actually has two cases where this can crash the runtime due to assembly unloading.

Lastly I've not yet traced out System.Reflection.TypeLoading.Ecma.EcmaAssemblies usage of UnmanagedMemoryStream but am concerned that I don't immediately see anything that would ensure the memory cannot be unloaded while the returned stream lives on. On the other hand it sounds like MetadataLoadContext is known to potentially cause AccessViolations if the context is disposed while other things are trying to read from it.

[vitek-karas]

@steveharter for the reflection API implications.
@joperezr for the resource API implications.

I actually don't know the best area this should belong to... (not that it matters, we should fix this regardless)