Fix a race condition between certain Has properties and their collection property #843
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ion 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.
|
@jbevain What are your thoughts on this PR? |
|
@jbevain Would you be willing to take this PR? |
|
@mrvoorhe I would. Thank you! |
mrvoorhe
added a commit
to Unity-Technologies/cecil
that referenced
this pull request
Oct 5, 2022
During `AssemblyReader.ReadCustomAttributes` there is a call to `WindowsRuntimeProjections.Project` ``` if (module.IsWindowsMetadata ()) foreach (var custom_attribute in custom_attributes) WindowsRuntimeProjections.Project (owner, custom_attributes, custom_attribute); ``` `WindowsRuntimeProjections.Project` would call `WindowsRuntimeProjections.HasAttribute`, which would then call `type.CustomAttributes`, which would end up back in `AssemblyReader.ReadCustomAttributes`. This would lead to a StackOverflowException. This wasn't an issue previously. My PR jbevain#843 caused this sequence of calls to start resulting in a StackOverflowException. Prior to my PR, there was a call to `metadata.RemoveCustomAttributeRange (owner);` before the call to `WindowsRuntimeProjections.Project`. This meant that when `WindowsRuntimeProjections.HasAttribute` would call `type.CustomAttributes`, we'd still end up in `AssemblyReader.ReadCustomAttributes`, however, no attributes would be found because the following if would be true and lead to returning an empty collection. ``` if (!metadata.TryGetCustomAttributeRanges (owner, out ranges)) return new Collection<CustomAttribute> (); ``` The old behavior was probably the wrong. Although I'm not certain what the tangible impact was. The fix was pretty easy. `AssemblyReader.ReadCustomAttributes` will now pass in the custom attributes to `WindowsRuntimeProjections.Project` avoiding the need to call `type.CustomAttributes`
mrvoorhe
added a commit
to Unity-Technologies/cecil
that referenced
this pull request
Oct 7, 2022
During `AssemblyReader.ReadCustomAttributes` there is a call to `WindowsRuntimeProjections.Project` ``` if (module.IsWindowsMetadata ()) foreach (var custom_attribute in custom_attributes) WindowsRuntimeProjections.Project (owner, custom_attributes, custom_attribute); ``` `WindowsRuntimeProjections.Project` would call `WindowsRuntimeProjections.HasAttribute`, which would then call `type.CustomAttributes`, which would end up back in `AssemblyReader.ReadCustomAttributes`. This would lead to a StackOverflowException. This wasn't an issue previously. My PR jbevain#843 caused this sequence of calls to start resulting in a StackOverflowException. Prior to my PR, there was a call to `metadata.RemoveCustomAttributeRange (owner);` before the call to `WindowsRuntimeProjections.Project`. This meant that when `WindowsRuntimeProjections.HasAttribute` would call `type.CustomAttributes`, we'd still end up in `AssemblyReader.ReadCustomAttributes`, however, no attributes would be found because the following if would be true and lead to returning an empty collection. ``` if (!metadata.TryGetCustomAttributeRanges (owner, out ranges)) return new Collection<CustomAttribute> (); ``` The old behavior was probably the wrong. Although I'm not certain what the tangible impact was. The fix was pretty easy. `AssemblyReader.ReadCustomAttributes` will now pass in the custom attributes to `WindowsRuntimeProjections.Project` avoiding the need to call `type.CustomAttributes`
mrvoorhe
added a commit
to Unity-Technologies/cecil
that referenced
this pull request
Nov 15, 2022
During `AssemblyReader.ReadCustomAttributes` there is a call to `WindowsRuntimeProjections.Project` ``` if (module.IsWindowsMetadata ()) foreach (var custom_attribute in custom_attributes) WindowsRuntimeProjections.Project (owner, custom_attributes, custom_attribute); ``` `WindowsRuntimeProjections.Project` would call `WindowsRuntimeProjections.HasAttribute`, which would then call `type.CustomAttributes`, which would end up back in `AssemblyReader.ReadCustomAttributes`. This would lead to a StackOverflowException. This wasn't an issue previously. My PR jbevain#843 caused this sequence of calls to start resulting in a StackOverflowException. Prior to my PR, there was a call to `metadata.RemoveCustomAttributeRange (owner);` before the call to `WindowsRuntimeProjections.Project`. This meant that when `WindowsRuntimeProjections.HasAttribute` would call `type.CustomAttributes`, we'd still end up in `AssemblyReader.ReadCustomAttributes`, however, no attributes would be found because the following if would be true and lead to returning an empty collection. ``` if (!metadata.TryGetCustomAttributeRanges (owner, out ranges)) return new Collection<CustomAttribute> (); ``` The old behavior was probably the wrong. Although I'm not certain what the tangible impact was. The fix was pretty easy. `AssemblyReader.ReadCustomAttributes` will now pass in the custom attributes to `WindowsRuntimeProjections.Project` avoiding the need to call `type.CustomAttributes`
jbevain
pushed a commit
that referenced
this pull request
Nov 16, 2022
During `AssemblyReader.ReadCustomAttributes` there is a call to `WindowsRuntimeProjections.Project` ``` if (module.IsWindowsMetadata ()) foreach (var custom_attribute in custom_attributes) WindowsRuntimeProjections.Project (owner, custom_attributes, custom_attribute); ``` `WindowsRuntimeProjections.Project` would call `WindowsRuntimeProjections.HasAttribute`, which would then call `type.CustomAttributes`, which would end up back in `AssemblyReader.ReadCustomAttributes`. This would lead to a StackOverflowException. This wasn't an issue previously. My PR #843 caused this sequence of calls to start resulting in a StackOverflowException. Prior to my PR, there was a call to `metadata.RemoveCustomAttributeRange (owner);` before the call to `WindowsRuntimeProjections.Project`. This meant that when `WindowsRuntimeProjections.HasAttribute` would call `type.CustomAttributes`, we'd still end up in `AssemblyReader.ReadCustomAttributes`, however, no attributes would be found because the following if would be true and lead to returning an empty collection. ``` if (!metadata.TryGetCustomAttributeRanges (owner, out ranges)) return new Collection<CustomAttribute> (); ``` The old behavior was probably the wrong. Although I'm not certain what the tangible impact was. The fix was pretty easy. `AssemblyReader.ReadCustomAttributes` will now pass in the custom attributes to `WindowsRuntimeProjections.Project` avoiding the need to call `type.CustomAttributes`
jonpryor
pushed a commit
to dotnet/android
that referenced
this pull request
Jul 3, 2024
Context: #9043 Changes: jbevain/cecil@0.11.4...0.11.5 * jbevain/cecil@8c123e1: Bump to 0.11.5 * jbevain/cecil@870ce3e: Fix RVA field alignment (jbevain/cecil#888) * jbevain/cecil@4ad9c0f: Fix that method resolution would consider all function-pointers to be the same (jbevain/cecil#885) * jbevain/cecil@cc48622: Fix a StackOverflowException reading windows runtime assemblies. (jbevain/cecil#879) * jbevain/cecil@341fb14: Treat instance and static methods as different methods during resolution (jbevain/cecil#882) * jbevain/cecil@e052ab5: Address issue #873 (jbevain/cecil#874) * jbevain/cecil@92f32da: Add `MethodImplAttributes.AggressiveOptimization` (jbevain/cecil#855) * jbevain/cecil@c4cfe16: Fix a race condition between certain Has properties and their collection property. (jbevain/cecil#843) * jbevain/cecil@65a2912: Add more style configuration (jbevain/cecil#854) * jbevain/cecil@9eb00e4: ILProcessor should also update custom debug info (jbevain/cecil#867) * jbevain/cecil@7d36386: Fix corrupted debug header directory entry when writing multiple such entries. (jbevain/cecil#869) * jbevain/cecil@6f94613: InvariantCulture for operand to string conversion in Instruction.ToString() (jbevain/cecil#870) * jbevain/cecil@42b9ef1: Add support for generic attributes (jbevain/cecil#871) * jbevain/cecil@49b1c52: Add `Unmanaged` calling convention (jbevain/cecil#852) * jbevain/cecil@2c68927: Fix mixed module ReadSymbols() (jbevain/cecil#851) * jbevain/cecil@f7b64f7: Fix custom attribute with enum on generic type (jbevain/cecil#827) * jbevain/cecil@79b43e8: Fix deterministic MVID and add PdbChecksum (jbevain/cecil#810) * jbevain/cecil@8b593d5: Harden debug scope update logic (jbevain/cecil#824) * jbevain/cecil@a56b5bd: FieldRVA alignment (jbevain/cecil#817) * jbevain/cecil@75372c7: Switch to netcoreapp3.1 for tests (jbevain/cecil#823) * jbevain/cecil@5f69faa: Add support for generating the method and generic method comment signature with nested types (jbevain/cecil#801) * jbevain/cecil@a0a6ce4: Addressing issue #781 (jbevain/cecil#782) * jbevain/cecil@2f1077d: Update the version of Microsoft.NETFramework.ReferenceAssemblies.net40 (jbevain/cecil#787) * jbevain/cecil@ede17f9: Fix handling of empty string constants (jbevain/cecil#776) #9043 stops building API-34 and makes API-35 stable, and in attempting to do so encounters this error when running all of the in-tree Windows smoke tests on CI: D:\a\_work\1\s\bin\Release\dotnet\packs\Microsoft.Android.Sdk.Windows\35.0.0-ci.pr.gh9043.6\tools\Xamarin.Android.Bindings.JavaDependencyVerification.targets(22,5): error MSB4062: The "Xamarin.Android.Tasks.GetMicrosoftNuGetPackagesMap" task could not be loaded from the assembly D:\a\_work\1\s\bin\Release\dotnet\packs\Microsoft.Android.Sdk.Windows\35.0.0-ci.pr.gh9043.6\tools\Xamarin.Android.Build.Tasks.dll. Could not load file or assembly 'Mono.Cecil, Version=0.11.4.0, Culture=neutral, PublicKeyToken=50cebf1cceb9d05e'. The system cannot find the file specified. Confirm that the <UsingTask> declaration is correct, that the assembly and all its dependencies are available, and that the task contains a public class that implements Microsoft.Build.Framework.ITask. [D:\a\_work\1\a\TestRelease\07-02_16.40.15\temp\DotNetBuildandroid-armFalseFalseFalse\UnnamedProject.csproj] This appears to be caused by the fact that previously we: 1. Build `Microsoft.Android.Sdk.ILLink.csproj`, which pulls in [Microsoft.NET.ILLink/9.0.0-preview.6.24319.11][0], which pulls in [Microsoft.DotNet.Cecil/0.11.4-alpha.24313.1][1], which contains `Mono.Cecil.dll` versioned as 0.11.5.0. (Yes, it's "odd" that `Microsoft.DotNet.Cecil/0.11.4*` would contain a Cecil versioned as 0.11.5, but that's what it has!) 2. Build the rest of dotnet/android with the [Mono.Cecil/0.11.4][2] package. 3. The Mono.Cecil/0.11.4 package "wins" and ends up in the output directory and the sdk pack. So long as the `ILLink*`-related assemblies don't use any Mono.Cecil/0.11.5 APIs, this works, however it is risky because we don't know exactly what API `ILLink*` uses. #9043 appears to change the build order such that Mono.Cecil/0.11.5 now "wins" and is in the output directory. This causes the above MSB4062 assembly load error. We could try to fix the ordering and make Mono.Cecil/0.11.4 "win", but this leaves us vulnerable to missing some API that `ILLink*` needs. As such, it's better that we update the rest of dotnet/android to use the `0.11.5` version of `Mono.Cecil`. This update breaks the `LinkerTests.FixAbstractMethodsStep_Explicit()` unit test. Specifically, this logic now returns `null` instead of being able to be resolved: new MethodReference (iface_method.Name, void_type, iface) .Resolve (); It feels like this makes sense: a method name and return type doesn't seem like it would be enough to resolve, as parameters are not considered. The fix is simply to use the existing `MethodDefinition` as the `MethodReference`, there is no reason to create a new one. This change fixes the test. [0]: https://dev.azure.com/dnceng/public/_artifacts/feed/dotnet9-transport/NuGet/Microsoft.NET.ILLink/overview/9.0.0-preview.7.24328.10 [1]: https://dev.azure.com/dnceng/public/_artifacts/feed/dotnet9-transport/NuGet/Microsoft.DotNet.Cecil/overview/0.11.4-alpha.24313.1 [2]: https://www.nuget.org/packages/Mono.Cecil/0.11.4
# for free
to join this conversation on GitHub.
Already have an account?
# to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We found a rare race condition between
MethodDefinition.HasOverridesandMethodDefinition.Overrides.What can happen is
Thread 1 get's past the null check in
MethodDefinition.HasOverridesand then is suspended.Thread 2, calls
MethodDefinition.Overridesand executes at least as far as themetadata.RemoveOverrideMapping (method)call inAssemblyReader.ReadOverridesThread 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 fromMetadataSystemTo recap, the two notable behaviors are triggering this are
a) The following check in
MethodDefinition.HasOverrideshappens outside of the module lock.b) The call to
metadata.RemoveOverrideMappinginAssemblyReader.ReadOverridesmeans thatAssemblyReader.ReadOverridesandAssemblyReader.HasOverridescannot be called again after the first call toAssemblyReader.ReadOverridesI 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.Deferredalso appears to be a required prerequisite to encounter this problem.We had two thoughts on how to fix this
Repeat the collection null check after obtaining the module lock in
Module.ReadduringMethodDefinition.HasOverridesRemove the behavior of
AssemblyReaderremoving data from theMetadataSystem.I decided to go with Fix 2 because it was easy to find all of problematic property pairings by searching
MetadataSystem.csforRemove. 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.