runtime: Concurrency bug with PEReader when loading from FileStream on Linux

Description

PEReader appears to not be properly thread safe if passed a FileStream (it will use memory mapping) and can blow up on Linux if used from multiple threads. We noticed this because ILVerify kept blowing up on weird race condition errors only on Linux.

Reproduction Steps

Run this code block: it will blow up on Linux after a few seconds but work fine on Windows. Needs Microsoft.ILVerification.

using System;
using System.Collections.Concurrent;
using System.IO;
using System.Reflection;
using System.Reflection.PortableExecutable;
using System.Threading.Tasks;
using ILVerify;

namespace pereader
{
    class Program
    {
        static void Main(string[] args)
        {
            while (true)
            {
                var resolver = new Resolver();
                Parallel.For(1, 32, _ => {
                    var verifier = new Verifier(resolver);
                    verifier.SetSystemModuleName(new AssemblyName("System.Runtime"));
                });
            }
        }
    }

    class Resolver : IResolver
    {
        private readonly ConcurrentDictionary<string, PEReader> _dictionary = new();

        public static readonly string BasePath = Path.GetDirectoryName(typeof(Console).Assembly.Location); 

        private PEReader ResolveCore(string simpleName)
        {
            return new PEReader(File.OpenRead(Path.Combine(BasePath, $"{simpleName}.dll")));
        }

        public PEReader Resolve(string simpleName)
        {
            return _dictionary.GetOrAdd(simpleName, ResolveCore);
        }
    }
}

Expected behavior

PEReader to be thread safe on Linux

Actual behavior

PEReader is not thread safe on Linux.

Some examples of exceptions that occur (it’s not consistent):

Unhandled exception. System.AggregateException: One or more errors occurred. (Unknown PE Magic value.)                                               ---> System.BadImageFormatException: Unknown PE Magic value.
at System.Reflection.PortableExecutable.PEHeader..ctor(PEBinaryReader& reader)
at System.Reflection.PortableExecutable.PEHeaders..ctor(Stream peStream, Int32 size, Boolean isLoadedImage)
at System.Reflection.PortableExecutable.PEReader.InitializePEHeaders()
at Internal.TypeSystem.Ecma.EcmaModule.CreateMetadataReader(TypeSystemContext context, PEReader peReader)
at Internal.TypeSystem.Ecma.EcmaModule.Create(TypeSystemContext context, PEReader peReader, IAssemblyDesc containingAssembly, IModuleResolver customModuleResolver)
at ILVerify.ILVerifyTypeSystemContext.GetModule(PEReader peReader, IAssemblyDesc containingAssembly)
at ILVerify.Verifier.SetSystemModuleName(AssemblyName name)
at pereader.Program.<>c__DisplayClass0_0.<Main>b__0(Int32 _) in /home/pj/pereader/Program.cs:line 20

 ---> System.ObjectDisposedException: Cannot access a closed file.                                                                                     at System.IO.FileStream.Seek(Int64 offset, SeekOrigin origin)at System.Reflection.PortableExecutable.PEReader.InitializePEHeaders()
at System.Reflection.PortableExecutable.PEReader.get_HasMetadata()
at Internal.TypeSystem.Ecma.EcmaModule.CreateMetadataReader(TypeSystemContext context, PEReader peReader)
at Internal.TypeSystem.Ecma.EcmaModule.Create(TypeSystemContext context, PEReader peReader, IAssemblyDesc containingAssembly, IModuleResolver customModuleResolver)
at ILVerify.ILVerifyTypeSystemContext.GetModule(PEReader peReader, IAssemblyDesc containingAssembly)
at ILVerify.Verifier.SetSystemModuleName(AssemblyName name)
at pereader.Program.<>c__DisplayClass0_0.<Main>b__0(Int32 _) in /home/pj/pereader/Program.cs:line 20

Regression?

No response

Known Workarounds

Loading from a non-FileStream (e.g. copying to a MemoryStream first) works around the problem because it will use a slower path that doesn’t use memory mapping.

Configuration

.NET runtime: 5.0.11
OS: Ubuntu Server 20.04.3, everything up to date package wise
Architecture: x64
Specific: many other people on Linux have reported this.

Other information

No response

About this issue

Original URL
State: closed
Created 3 years ago
Comments: 16 (16 by maintainers)

Most upvoted comments

From the doc You can create multiple views of the memory-mapped file, including views of parts of the file. You can map the same part of a file to more than one address to create concurrent memory. For two views to remain concurrent, they have to be created from the same memory-mapped file. Creating two file mappings of the same file with two views does not provide concurrency. https://docs.microsoft.com/en-us/dotnet/api/system.io.memorymappedfiles.memorymappedfile?view=net-5.0#remarks So calls to MemoryMappedFile.CreateViewAccessor are expected to be thread-safe, apparently, it was not thread-safe on Linux, and it is possible that the PR you attached fixed that.

I believe the docs are saying that you can use safely two view accessors concurrently. They do not say that you can create a view accessor simultaneously to using the underlying handle. The recent change simply made that less fragile but it’s still not supported.

I believe that the call here to CreateViewAccessor needs to be protected by the lock object protecting the stream.

danmoseley on Nov 17, 2021

@buyaa-n thanks for sharing the stack trace!

This is the interesting part:

Unhandled exception. System.AggregateException: One or more errors occurred. (Invalid argument)
 ---> System.IO.IOException: Invalid argument
   at System.IO.FileStream.CheckFileCall(Int64 result, Boolean ignoreNotSupported)
   at System.IO.FileStream.FlushReadBuffer()
   at System.IO.FileStream.FlushInternalBuffer()
   at System.IO.FileStream.Flush(Boolean flushToDisk)
   at System.IO.FileStream.Flush()
   at System.IO.FileStream.get_SafeFileHandle()
   at System.IO.MemoryMappedFiles.MemoryMappedView.CreateView(SafeMemoryMappedFileHandle memMappedFileHandle, MemoryMappedFileAccess access, Int64 requestedOffset, Int64 requestedSize)

It seems that Flush() method was throwing when FileStream.SafeFileHandle was accessed. This is still the case for today when buffering is enabled:

https://github.com/dotnet/runtime/blob/0666ebc475871c27f5b9d4ee8e91922f20be46e9/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/BufferedFileStreamStrategy.cs#L101-L108

And most likely the PR pointed by @danmoseley (https://github.com/dotnet/runtime/pull/53538) has fixed it by just storing the SafeFileHandle value in a field.

So most likely the FileStream rewrite has not affected it in any way. But it does not answer the question whether PEReader is guaranteed to be thread safe.

adamsitnik on Nov 17, 2021

PEReader accepts a Stream, which it wraps (in this case) in a StreamMemoryBlockProvider which it stores as a field _peImage.

StreamMemoryBlockProvider has a field _streamGuard containing an object it uses to lock access to the stream. https://github.com/dotnet/runtime/blob/03e90a540cb2dfe5cab4086ea54ad5dd1f655749/src/libraries/System.Reflection.Metadata/src/System/Reflection/Internal/MemoryBlocks/StreamMemoryBlockProvider.cs#L25-L28

However StreamMemoryBlockProvider.GetStream() will return the stream - along with a StreamConstraints object that contains the _streamGuard lock.

PEReaders.PEHeaders calls PEReader.InitializePEHeaders() which retrieves the stream through this GetStream(), and it locks to ensure the PEHeaders constructor is not run concurrently, and PEHeaders only uses the stream during construction, and none of its fields hold a reference to the stream. https://github.com/dotnet/runtime/blob/03e90a540cb2dfe5cab4086ea54ad5dd1f655749/src/libraries/System.Reflection.Metadata/src/System/Reflection/PortableExecutable/PEReader.cs#L306-L314

So far so good.

StreamMemoryBlockProvider creates the MemoryMappedFile over the stream within a lock https://github.com/dotnet/runtime/blob/03e90a540cb2dfe5cab4086ea54ad5dd1f655749/src/libraries/System.Reflection.Metadata/src/System/Reflection/Internal/MemoryBlocks/StreamMemoryBlockProvider.cs#L132

however it then calls MemoryMappedFile.CreateViewAccessor outside of the lock https://github.com/dotnet/runtime/blob/03e90a540cb2dfe5cab4086ea54ad5dd1f655749/src/libraries/System.Reflection.Metadata/src/System/Reflection/Internal/MemoryBlocks/StreamMemoryBlockProvider.cs#L163

as seen in the 3rd stack above, that is not guaranteed safe for concurrent calls, nor for concurrent access to the stream, and by refactoring it it just happens to have become less fragile.

I didn’t investigate whether subsequent use of MemoryMappedFileBlock is safe, but the above issue is consistent with the above symptoms.

danmoseley on Nov 17, 2021

I’m not comfortably relying on “not reproing” as evidence that a race condition has been fixed. Although there were major changes in FileStream, I’m not aware of any changes that intended to fixed thread safety issues - in general, the IO API are not intended to be thread safe. It’s possible that the bug simply got moved around due to refactoring. Anyway not all the stacks above have FileStream on them, which strongly suggests that there is protection missing higher up in PEReader, which is after all what claims to be safe.

Ah indeed, in a quick look, this change https://github.com/dotnet/runtime/commit/c2e8973c625dfb890dab3aa86d7d5a6be636cbde#diff-6b7d6a3d9f12a289e4205e436a7a5191378b664f47e6f04f9fc3932d5a8f9fb1R68 likely hid the FlushInternalBuffer() callstack above. Getting the handle was triggering a flush; it now caches the handle in the MemoryMappedView. I don’t think there was a correctness issue: it’s not intended to be safe for concurrent access.

The other stacks may also be still latent.

I suggest looking at PEReader to understand how it intends to be thread safe and whether there is a hole.

danmoseley on Nov 17, 2021

I ran a ~10 minute stress test of it on .NET 6 and didn’t get any exception. Seems good?

PJB3005 on Nov 17, 2021