Fix a race condition between certain Has properties and their collection property

[mrvoorhe]

We found a rare race condition between MethodDefinition.HasOverrides and MethodDefinition.Overrides.

What can happen is

1. Thread 1 get's past the null check in MethodDefinition.HasOverrides and then is suspended.

2. Thread 2, calls MethodDefinition.Overrides and executes at least as far as the metadata.RemoveOverrideMapping (method) call in AssemblyReader.ReadOverrides

3. Thread 1 resumes on return HasImage && Module.Read (this, (method, reader) => reader.HasOverrides (method)); It now proceeds to AssemblyReader.HasOverrides. No overrides are found and false is returned due to the overrides for that method having been removed from MetadataSystem

To recap, the two notable behaviors are triggering this are

a) The following check in MethodDefinition.HasOverrides happens outside of the module lock.

if (overrides != null)
    return overrides.Count > 0;

b) The call to metadata.RemoveOverrideMapping in AssemblyReader.ReadOverrides means that AssemblyReader.ReadOverrides and AssemblyReader.HasOverrides cannot be called again after the first call to AssemblyReader.ReadOverrides

I did not attempt to reproduce this vulnerability for every pair of properties that follows this pattern. However, I think it's safe to assume any pair of properties that follows this same pattern are vulnerable.

Using ReadingMode.Deferred also appears to be a required prerequisite to encounter this problem.

We had two thoughts on how to fix this

1. Repeat the collection null check after obtaining the module lock in Module.Read during MethodDefinition.HasOverrides

2. Remove the behavior of AssemblyReader removing data from the MetadataSystem.

I decided to go with Fix 2 because it was easy to find all of problematic property pairings by searching MetadataSystem.cs for Remove. I also feel that this behavior of modifying the metadata system is asking for problems and probably not worth the freed memory is provides.

If you'd prefer Fix 1 instead. Or both Fix 1 & Fix 2, or some other fix, let me know and I can change around the PR.

[mrvoorhe]

@jbevain What are your thoughts on this PR?

[mrvoorhe]

@jbevain Would you be willing to take this PR?

[jbevain]

@mrvoorhe I would. Thank you!