Skip to content

Commit

Permalink
Fix a race condition between certain Has properties and their collect…
Browse files Browse the repository at this point in the history 
…ion 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.
  • Loading branch information
@mrvoorhe
mrvoorhe authored Sep 29, 2022
1 parent 65a2912 commit c4cfe16
Show file tree
Hide file tree
Showing 2 changed files with 0 additions and 69 deletions.
19 changes: 0 additions & 19 deletions Mono.Cecil/AssemblyReader.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -899,8 +899,6 @@ public Collection<TypeDefinition> ReadNestedTypes (TypeDefinition type)
nested_types.Add (nested_type);
}

metadata.RemoveNestedTypeMapping (type);

return nested_types;
}

Expand Down Expand Up @@ -975,7 +973,6 @@ TypeDefinition GetNestedTypeDeclaringType (TypeDefinition type)
if (!metadata.TryGetReverseNestedTypeMapping (type, out declaring_rid))
return null;

metadata.RemoveReverseNestedTypeMapping (type);
return GetTypeDefinition (declaring_rid);
}

Expand Down Expand Up @@ -1242,8 +1239,6 @@ public InterfaceImplementationCollection ReadInterfaces (TypeDefinition type)
new MetadataToken(TokenType.InterfaceImpl, mapping [i].Col1)));
}

metadata.RemoveInterfaceMapping (type);

return interfaces;
}

Expand Down Expand Up @@ -1466,8 +1461,6 @@ public Collection<EventDefinition> ReadEvents (TypeDefinition type)

var events = new MemberDefinitionCollection<EventDefinition> (type, (int) range.Length);

metadata.RemoveEventsRange (type);

if (range.Length == 0)
return events;

Expand Down Expand Up @@ -1536,8 +1529,6 @@ public Collection<PropertyDefinition> ReadProperties (TypeDefinition type)
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)
Expand Down Expand Up @@ -1912,8 +1903,6 @@ public Collection<GenericParameter> 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++)
Expand Down Expand Up @@ -2029,8 +2018,6 @@ public GenericParameterConstraintCollection ReadGenericConstraints (GenericParam
new MetadataToken (TokenType.GenericParamConstraint, mapping [i].Col1)));
}

metadata.RemoveGenericConstraintMapping (generic_parameter);

return constraints;
}

Expand Down Expand Up @@ -2083,8 +2070,6 @@ public Collection<MethodReference> ReadOverrides (MethodDefinition method)
for (int i = 0; i < mapping.Count; i++)
overrides.Add ((MethodReference) LookupToken (mapping [i]));

metadata.RemoveOverrideMapping (method);

return overrides;
}

Expand Down Expand Up @@ -2521,8 +2506,6 @@ public Collection<CustomAttribute> 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);
Expand Down Expand Up @@ -2676,8 +2659,6 @@ public Collection<SecurityDeclaration> ReadSecurityDeclarations (ISecurityDeclar
for (int i = 0; i < ranges.Length; i++)
ReadSecurityDeclarationRange (ranges [i], security_declarations);

metadata.RemoveSecurityDeclarationRange (owner);

return security_declarations;
}

Expand Down
50 changes: 0 additions & 50 deletions Mono.Cecil/MetadataSystem.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -243,11 +243,6 @@ public void SetNestedTypeMapping (uint type_rid, Collection<uint> 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);
Expand All @@ -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<Row<uint, MetadataToken>> mapping)
{
return Interfaces.TryGetValue (type.token.RID, out mapping);
Expand All @@ -273,11 +263,6 @@ public void SetInterfaceMapping (uint type_rid, Collection<Row<uint, MetadataTok
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);
Expand All @@ -288,11 +273,6 @@ public bool TryGetPropertiesRange (TypeDefinition type, out Range range)
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);
Expand All @@ -303,41 +283,21 @@ public bool TryGetEventsRange (TypeDefinition type, out Range range)
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);
Expand All @@ -348,11 +308,6 @@ public void SetGenericConstraintMapping (uint gp_rid, Collection<Row<uint, Metad
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);
Expand All @@ -363,11 +318,6 @@ public void SetOverrideMapping (uint rid, Collection<MetadataToken> 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)
Expand Down

0 comments on commit c4cfe16

Please sign in to comment.