Fix a race condition between certain Has properties and their collection property. (#843)

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 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 is 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 let me know and I can change around the PR.

0 files changed, 0 insertions, 0 deletions