Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix a StackOverflowException reading windows runtime assemblies. #879

Merged
merged 1 commit into from
Nov 16, 2022
Merged

Fix a StackOverflowException reading windows runtime assemblies. #879

merged 1 commit into from
Nov 16, 2022

Conversation

mrvoorhe
Copy link
Contributor

@mrvoorhe mrvoorhe commented 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 #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

@SimonCropp
Copy link
Contributor

including a test that verifies the change might be helpful

@mrvoorhe
Copy link
Contributor Author

mrvoorhe commented Oct 7, 2022

Fair point. I'll see if I can come up with something

@mrvoorhe mrvoorhe force-pushed the fix-stack-overflow-due-to-winrt-projections branch from 30e34d5 to 27c09ce Compare October 7, 2022 15:25
@mrvoorhe
Copy link
Contributor Author

mrvoorhe commented Oct 7, 2022

I updated the PR with tests. A few comments on the tests

  • There was some logic to look in the 8.1 SDK. This is outdated and shouldn't exist anymore. I added logic to locate Windows.winmd from the windows 10 SDK.

  • In order for the StackOverflowException to be triggered, Windows.winmd needed to be read with ApplyWindowsRuntimeProjections=true. It turns out none of the windows runtime tests were doing that today. I spoke with @TautvydasZilys about this and the conclusion was that it wouldn't be a bad thing if all of the windows runtime tests ran with ApplyWindowsRuntimeProjections set to false and to true. That is what the [TestCase(true)] and [TestCase(false)] changes are for.

@jbevain please take a look.

@jbevain
Copy link
Owner

jbevain commented Nov 15, 2022

Hi @mrvoorhe,

Thanks for the PR. Please address the tidbits and this is good to go.

@mrvoorhe
Fix a StackOverflowException reading windows runtime assemblies.
86dd3ae 
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 mrvoorhe force-pushed the fix-stack-overflow-due-to-winrt-projections branch from 27c09ce to 86dd3ae Compare November 15, 2022 18:14
@mrvoorhe
Copy link
Contributor Author

@jbevain I addressed your comments. Thanks!

@jbevain jbevain merged commit cc48622 into jbevain:master Nov 16, 2022
@jbevain
Copy link
Owner

jbevain commented Nov 16, 2022

Thanks!

jonpryor pushed a commit to dotnet/android that referenced this pull request Jul 3, 2024
@jpobst
Bump to jbevain/cecil@8c123e1 (#9078)
54a9c8c 
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jbevain jbevain jbevain left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mrvoorhe @SimonCropp @jbevain