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.

2 files changed, 0 insertions, 69 deletions

diff --git a/Mono.Cecil/AssemblyReader.cs b/Mono.Cecil/AssemblyReader.cs
index 4180d78..0756fa8 100644
--- a/Mono.Cecil/AssemblyReader.cs
+++ b/Mono.Cecil/AssemblyReader.cs
@@ -899,8 +899,6 @@ namespace Mono.Cecil {
 					nested_types.Add (nested_type);
 			}
 
-			metadata.RemoveNestedTypeMapping (type);
-
 			return nested_types;
 		}
 
@@ -975,7 +973,6 @@ namespace Mono.Cecil {
 			if (!metadata.TryGetReverseNestedTypeMapping (type, out declaring_rid))
 				return null;
 
-			metadata.RemoveReverseNestedTypeMapping (type);
 			return GetTypeDefinition (declaring_rid);
 		}
 
@@ -1242,8 +1239,6 @@ namespace Mono.Cecil {
 						new MetadataToken(TokenType.InterfaceImpl, mapping [i].Col1)));
 			}
 
-			metadata.RemoveInterfaceMapping (type);
-
 			return interfaces;
 		}
 
@@ -1466,8 +1461,6 @@ namespace Mono.Cecil {
 
 			var events = new MemberDefinitionCollection<EventDefinition> (type, (int) range.Length);
 
-			metadata.RemoveEventsRange (type);
-
 			if (range.Length == 0)
 				return events;
 
@@ -1536,8 +1529,6 @@ namespace Mono.Cecil {
 			if (!metadata.TryGetPropertiesRange (type, out range))
 				return new MemberDefinitionCollection<PropertyDefinition> (type);
 
-			metadata.RemovePropertiesRange (type);
-
 			var properties = new MemberDefinitionCollection<PropertyDefinition> (type, (int) range.Length);
 
 			if (range.Length == 0)
@@ -1912,8 +1903,6 @@ namespace Mono.Cecil {
 			if (!metadata.TryGetGenericParameterRanges (provider, out ranges))
 				return new GenericParameterCollection (provider);
 
-			metadata.RemoveGenericParameterRange (provider);
-
 			var generic_parameters = new GenericParameterCollection (provider, RangesSize (ranges));
 
 			for (int i = 0; i < ranges.Length; i++)
@@ -2029,8 +2018,6 @@ namespace Mono.Cecil {
 						new MetadataToken (TokenType.GenericParamConstraint, mapping [i].Col1)));
 			}
 
-			metadata.RemoveGenericConstraintMapping (generic_parameter);
-
 			return constraints;
 		}
 
@@ -2083,8 +2070,6 @@ namespace Mono.Cecil {
 			for (int i = 0; i < mapping.Count; i++)
 				overrides.Add ((MethodReference) LookupToken (mapping [i]));
 
-			metadata.RemoveOverrideMapping (method);
-
 			return overrides;
 		}
 
@@ -2521,8 +2506,6 @@ namespace Mono.Cecil {
 			for (int i = 0; i < ranges.Length; i++)
 				ReadCustomAttributeRange (ranges [i], custom_attributes);
 
-			metadata.RemoveCustomAttributeRange (owner);
-
 			if (module.IsWindowsMetadata ())
 				foreach (var custom_attribute in custom_attributes)
 					WindowsRuntimeProjections.Project (owner, custom_attribute);
@@ -2676,8 +2659,6 @@ namespace Mono.Cecil {
 			for (int i = 0; i < ranges.Length; i++)
 				ReadSecurityDeclarationRange (ranges [i], security_declarations);
 
-			metadata.RemoveSecurityDeclarationRange (owner);
-
 			return security_declarations;
 		}
 
diff --git a/Mono.Cecil/MetadataSystem.cs b/Mono.Cecil/MetadataSystem.cs
index 749a39c..a877771 100644
--- a/Mono.Cecil/MetadataSystem.cs
+++ b/Mono.Cecil/MetadataSystem.cs
@@ -243,11 +243,6 @@ namespace Mono.Cecil {
 			NestedTypes [type_rid] = mapping;
 		}
 
-		public void RemoveNestedTypeMapping (TypeDefinition type)
-		{
-			NestedTypes.Remove (type.token.RID);
-		}
-
 		public bool TryGetReverseNestedTypeMapping (TypeDefinition type, out uint declaring)
 		{
 			return ReverseNestedTypes.TryGetValue (type.token.RID, out declaring);
@@ -258,11 +253,6 @@ namespace Mono.Cecil {
 			ReverseNestedTypes [nested] = declaring;
 		}
 
-		public void RemoveReverseNestedTypeMapping (TypeDefinition type)
-		{
-			ReverseNestedTypes.Remove (type.token.RID);
-		}
-
 		public bool TryGetInterfaceMapping (TypeDefinition type, out Collection<Row<uint, MetadataToken>> mapping)
 		{
 			return Interfaces.TryGetValue (type.token.RID, out mapping);
@@ -273,11 +263,6 @@ namespace Mono.Cecil {
 			Interfaces [type_rid] = mapping;
 		}
 
-		public void RemoveInterfaceMapping (TypeDefinition type)
-		{
-			Interfaces.Remove (type.token.RID);
-		}
-
 		public void AddPropertiesRange (uint type_rid, Range range)
 		{
 			Properties.Add (type_rid, range);
@@ -288,11 +273,6 @@ namespace Mono.Cecil {
 			return Properties.TryGetValue (type.token.RID, out range);
 		}
 
-		public void RemovePropertiesRange (TypeDefinition type)
-		{
-			Properties.Remove (type.token.RID);
-		}
-
 		public void AddEventsRange (uint type_rid, Range range)
 		{
 			Events.Add (type_rid, range);
@@ -303,41 +283,21 @@ namespace Mono.Cecil {
 			return Events.TryGetValue (type.token.RID, out range);
 		}
 
-		public void RemoveEventsRange (TypeDefinition type)
-		{
-			Events.Remove (type.token.RID);
-		}
-
 		public bool TryGetGenericParameterRanges (IGenericParameterProvider owner, out Range [] ranges)
 		{
 			return GenericParameters.TryGetValue (owner.MetadataToken, out ranges);
 		}
 
-		public void RemoveGenericParameterRange (IGenericParameterProvider owner)
-		{
-			GenericParameters.Remove (owner.MetadataToken);
-		}
-
 		public bool TryGetCustomAttributeRanges (ICustomAttributeProvider owner, out Range [] ranges)
 		{
 			return CustomAttributes.TryGetValue (owner.MetadataToken, out ranges);
 		}
 
-		public void RemoveCustomAttributeRange (ICustomAttributeProvider owner)
-		{
-			CustomAttributes.Remove (owner.MetadataToken);
-		}
-
 		public bool TryGetSecurityDeclarationRanges (ISecurityDeclarationProvider owner, out Range [] ranges)
 		{
 			return SecurityDeclarations.TryGetValue (owner.MetadataToken, out ranges);
 		}
 
-		public void RemoveSecurityDeclarationRange (ISecurityDeclarationProvider owner)
-		{
-			SecurityDeclarations.Remove (owner.MetadataToken);
-		}
-
 		public bool TryGetGenericConstraintMapping (GenericParameter generic_parameter, out Collection<Row<uint, MetadataToken>> mapping)
 		{
 			return GenericConstraints.TryGetValue (generic_parameter.token.RID, out mapping);
@@ -348,11 +308,6 @@ namespace Mono.Cecil {
 			GenericConstraints [gp_rid] = mapping;
 		}
 
-		public void RemoveGenericConstraintMapping (GenericParameter generic_parameter)
-		{
-			GenericConstraints.Remove (generic_parameter.token.RID);
-		}
-
 		public bool TryGetOverrideMapping (MethodDefinition method, out Collection<MetadataToken> mapping)
 		{
 			return Overrides.TryGetValue (method.token.RID, out mapping);
@@ -363,11 +318,6 @@ namespace Mono.Cecil {
 			Overrides [rid] = mapping;
 		}
 
-		public void RemoveOverrideMapping (MethodDefinition method)
-		{
-			Overrides.Remove (method.token.RID);
-		}
-
 		public Document GetDocument (uint rid)
 		{
 			if (rid < 1 || rid > Documents.Length)