From 8d8eeadcd943eeb334295faa1d1f4700dfb42b87 Mon Sep 17 00:00:00 2001 From: Michael Voorhees Date: Mon, 21 Mar 2022 10:53:33 -0400 Subject: [PATCH] Fix a race condition between certain Has properties and their collection property. 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. --- Mono.Cecil/AssemblyReader.cs | 19 -------------- Mono.Cecil/MetadataSystem.cs | 50 ------------------------------------ 2 files changed, 69 deletions(-) diff --git a/Mono.Cecil/AssemblyReader.cs b/Mono.Cecil/AssemblyReader.cs index ee2c229a9..89c288be9 100644 --- a/Mono.Cecil/AssemblyReader.cs +++ b/Mono.Cecil/AssemblyReader.cs @@ -899,8 +899,6 @@ public Collection ReadNestedTypes (TypeDefinition type) nested_types.Add (nested_type); } - metadata.RemoveNestedTypeMapping (type); - return nested_types; } @@ -975,7 +973,6 @@ TypeDefinition GetNestedTypeDeclaringType (TypeDefinition type) if (!metadata.TryGetReverseNestedTypeMapping (type, out declaring_rid)) return null; - metadata.RemoveReverseNestedTypeMapping (type); return GetTypeDefinition (declaring_rid); } @@ -1242,8 +1239,6 @@ public InterfaceImplementationCollection ReadInterfaces (TypeDefinition type) new MetadataToken(TokenType.InterfaceImpl, mapping [i].Col1))); } - metadata.RemoveInterfaceMapping (type); - return interfaces; } @@ -1466,8 +1461,6 @@ public Collection ReadEvents (TypeDefinition type) var events = new MemberDefinitionCollection (type, (int) range.Length); - metadata.RemoveEventsRange (type); - if (range.Length == 0) return events; @@ -1536,8 +1529,6 @@ public Collection ReadProperties (TypeDefinition type) if (!metadata.TryGetPropertiesRange (type, out range)) return new MemberDefinitionCollection (type); - metadata.RemovePropertiesRange (type); - var properties = new MemberDefinitionCollection (type, (int) range.Length); if (range.Length == 0) @@ -1912,8 +1903,6 @@ public Collection ReadGenericParameters (IGenericParameterProv 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 @@ public GenericParameterConstraintCollection ReadGenericConstraints (GenericParam new MetadataToken (TokenType.GenericParamConstraint, mapping [i].Col1))); } - metadata.RemoveGenericConstraintMapping (generic_parameter); - return constraints; } @@ -2083,8 +2070,6 @@ public Collection ReadOverrides (MethodDefinition method) for (int i = 0; i < mapping.Count; i++) overrides.Add ((MethodReference) LookupToken (mapping [i])); - metadata.RemoveOverrideMapping (method); - return overrides; } @@ -2521,8 +2506,6 @@ public Collection ReadCustomAttributes (ICustomAttributeProvide 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 @@ public Collection ReadSecurityDeclarations (ISecurityDeclar 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 749a39ca4..a8777716d 100644 --- a/Mono.Cecil/MetadataSystem.cs +++ b/Mono.Cecil/MetadataSystem.cs @@ -243,11 +243,6 @@ public void SetNestedTypeMapping (uint type_rid, Collection mapping) 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 @@ public void SetReverseNestedTypeMapping (uint nested, uint declaring) ReverseNestedTypes [nested] = declaring; } - public void RemoveReverseNestedTypeMapping (TypeDefinition type) - { - ReverseNestedTypes.Remove (type.token.RID); - } - public bool TryGetInterfaceMapping (TypeDefinition type, out Collection> mapping) { return Interfaces.TryGetValue (type.token.RID, out mapping); @@ -273,11 +263,6 @@ public void SetInterfaceMapping (uint type_rid, Collection> mapping) { return GenericConstraints.TryGetValue (generic_parameter.token.RID, out mapping); @@ -348,11 +308,6 @@ public void SetGenericConstraintMapping (uint gp_rid, Collection mapping) { return Overrides.TryGetValue (method.token.RID, out mapping); @@ -363,11 +318,6 @@ public void SetOverrideMapping (uint rid, Collection mapping) Overrides [rid] = mapping; } - public void RemoveOverrideMapping (MethodDefinition method) - { - Overrides.Remove (method.token.RID); - } - public Document GetDocument (uint rid) { if (rid < 1 || rid > Documents.Length)