gh-jbevain/cecil
Watch
1
0
Fork
You've already forked cecil
 0
Code Issues 79 Pull requests 35 Releases 6 Wiki Activity

Fix a StackOverflowException reading windows runtime assemblies. #879

Merged
mrvoorhe merged 1 commit from fix-stack-overflow-due-to-winrt-projections into master 2022-11-16 12:00:39 -06:00
Conversation 10 Commits 1 Files changed 5 +87 -43
mrvoorhe commented 2022-10-05 09:22:58 -05:00 (Migrated from github.com)

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 https://github.com/jbevain/cecil/pull/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

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 https://github.com/jbevain/cecil/pull/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 commented 2022-10-05 15:44:46 -05:00 (Migrated from github.com)

including a test that verifies the change might be helpful

including a test that verifies the change might be helpful
mrvoorhe commented 2022-10-07 09:42:50 -05:00 (Migrated from github.com)

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

Fair point. I'll see if I can come up with something
mrvoorhe commented 2022-10-07 10:32:20 -05:00 (Migrated from github.com)

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.

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 (Migrated from github.com) reviewed 2022-11-15 11:59:35 -06:00
Test/Mono.Cecil.Tests/ImageReadTests.cs
@ -182,7 +182,7 @@ namespace Mono.Cecil.Tests {
[Test]
jbevain (Migrated from github.com) commented 2022-11-15 11:59:35 -06:00

applyWindowsRuntimeProjections: false

`applyWindowsRuntimeProjections: false`
jbevain (Migrated from github.com) reviewed 2022-11-15 12:00:24 -06:00
Test/Mono.Cecil.Tests/WindowsRuntimeProjectionsTests.cs
@ -17,19 +17,21 @@ namespace Mono.Cecil.Tests {
protected abstract string [] ManagedClassTypeNames { get; }
jbevain (Migrated from github.com) commented 2022-11-15 12:00:23 -06:00

Style:

[TestCase (true)]
[TestCase (false)]
Style: ```csharp [TestCase (true)] [TestCase (false)] ```
jbevain commented 2022-11-15 12:01:11 -06:00 (Migrated from github.com)

Hi @mrvoorhe,

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

Hi @mrvoorhe, Thanks for the PR. Please address the tidbits and this is good to go.
mrvoorhe (Migrated from github.com) reviewed 2022-11-15 12:14:56 -06:00
Test/Mono.Cecil.Tests/WindowsRuntimeProjectionsTests.cs
@ -17,19 +17,21 @@ namespace Mono.Cecil.Tests {
protected abstract string [] ManagedClassTypeNames { get; }
mrvoorhe (Migrated from github.com) commented 2022-11-15 12:14:55 -06:00

Fixed

Fixed
mrvoorhe (Migrated from github.com) reviewed 2022-11-15 12:14:58 -06:00
Test/Mono.Cecil.Tests/ImageReadTests.cs
@ -182,7 +182,7 @@ namespace Mono.Cecil.Tests {
[Test]
mrvoorhe (Migrated from github.com) commented 2022-11-15 12:14:58 -06:00

Fixed

Fixed
mrvoorhe commented 2022-11-15 12:15:34 -06:00 (Migrated from github.com)

@jbevain I addressed your comments. Thanks!

@jbevain I addressed your comments. Thanks!
jbevain commented 2022-11-16 12:00:45 -06:00 (Migrated from github.com)

Thanks!

Thanks!
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
1.0

   
bug

   
enhancement

   
invalid
No labels
1.0
bug
enhancement
invalid
Milestone
Clear milestone
No items
No milestone
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
gh-jbevain/cecil!879
No description provided.