[Mono]: Fix static closed delegate fnptr crash. #68701
Merged
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
When accessing a function pointer to a static closed delegate like done in added test: GetFunctionPointerForDelegate_MarshalledClosedStaticDelegate it will trigger a read outside of the allocated mspecs buffer since invoke_sig and method signature arguments wont match. There was already logic to adjust this in emit_managed_wrapper_ilgen, but done after call to emit_managed_wrapper_validate_signature that will touch memory and depending on its content, trigger a crash. Fix make sure we do signature adjustments first and then validate the signature. Fix also adjust a couple of mspecs allocations to use g_new0 as all others to make sure we get NULL pointers in the mspecs array. Since this scenario was not covered on CI, commit also adds a new test in GetFunctionPointerForDelegateTests.cs covering this scenario.
|
I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label. |
|
/cc @naricc FYI |
lambdageek
approved these changes
Apr 29, 2022
This was referenced Apr 29, 2022
lambdageek
reviewed
Apr 29, 2022
...vices.UnitTests/System/Runtime/InteropServices/Marshal/GetFunctionPointerForDelegateTests.cs
Outdated
Show resolved
Hide resolved
|
Browser needs to be disabled, same as other tests in that test suite. |
jonpryor
pushed a commit
to dotnet/android
that referenced
this pull request
May 5, 2022
Context: dotnet/runtime#56989 Context: dotnet/runtime#68734 Context: dotnet/runtime#68914 Context: dotnet/runtime#68701 Changes: dotnet/installer@04e40fa...c7afae6 % git diff --shortstat 04e40fa9...c7afae69 98 files changed, 1788 insertions(+), 1191 deletions(-) Changes: dotnet/runtime@a21b9a2...c5d40c9 % git diff --shortstat a21b9a2d...c5d40c9e 28347 files changed, 1609359 insertions(+), 1066473 deletions(-) Changes: dotnet/linker@01c4f59...04c49c9 % git diff --shortstat 01c4f590...04c49c9d 577 files changed, 28039 insertions(+), 10835 deletions(-) Updates to build with the .NET 7 SDK and use the runtime specified by the SDK. We no longer use different SDK & runtime versions. This produces a 7.0.100 Android workload. After this is merged we should be able to enable Maestro to consume future .NET 7 builds from dotnet/installer/main. ~~ Known Issues ~~ AOT+LLVM crashes on startup: * dotnet/runtime#68914 Xamarin.Build.Download hits a breaking change with `ZipEntry`: * dotnet/runtime#68734 * xamarin/XamarinComponents#1368 illink outputs different MVIDs per architecture: * dotnet/linker#2203 * dotnet/runtime#67660 Size of `libmonosgen-2.0.so` regressed: * dotnet/runtime#68330 * dotnet/runtime#68354 Newer .NET 7 builds crash on startup: * dotnet/runtime#68701 * This is worked around by *disabling* lazy loading of AOT'd assemblies 6dc426f. * TODO: re-enable once we get a fixed .NET 7 runtime. TODO: We can't yet push to the `dotnet7` feed. Working on this. Co-authored-by: Jonathan Peppers <jonathan.peppers@microsoft.com> Co-authored-by: Peter Collins <pecolli@microsoft.com>
ghost
locked as resolved and limited conversation to collaborators
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
When accessing a function pointer to a static closed delegate like done in added test:
GetFunctionPointerForDelegate_MarshalledClosedStaticDelegate
it will trigger a read outside of the allocated mspecs buffer since invoke_sig and method signature arguments wont match.
There was already logic to adjust this in emit_managed_wrapper_ilgen, but done after call to emit_managed_wrapper_validate_signature that will touch memory and depending on its content, trigger a crash.
Fix make sure we do signature adjustments first and then validate the signature. Fix also adjust a couple of mspecs allocations to use g_new0 as all others to make sure we get NULL pointers in the mspecs array.
Since this scenario was not covered on CI, commit also adds a new tests GetFunctionPointerForDelegateTests.cs covering this scenario.