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

[Mono] Fix uninitialized vtable bug #67746

Merged
merged 13 commits into from
Apr 12, 2022
Merged

[Mono] Fix uninitialized vtable bug #67746

merged 13 commits into from
Apr 12, 2022

Conversation

simonrozsival
Copy link
Member

@simonrozsival simonrozsival commented Apr 8, 2022 •
edited
Loading

Fixes #67402

The code which checks whether a class derived from System.IO.Stream has overridden certain methods didn't explicitly setup the vtable before it tried to dereference the vtable pointer. When AOT was enabled the vtable pointer was null and dereferencing it caused a crash.

I also checked the other places where we access vtable and in most cases, we call the mono_class_setup_vtable method before we access the vtable except in two cases where it isn't obvious that the vtable is setup:

  • cominterop.c:502
  • mini-trampolines.c:593

I don't want to add any modifications to those pieces of code in this PR but I'll check those two instances in more detail and I might follow-up with another PR later.

EDIT: I analzyed the two pieces of code I mentioned previously and I came to the conclusion that accessing the vtable won't be a problem in those cases.

@ghost ghost assigned simonrozsival Apr 8, 2022
@ghost
Copy link

ghost commented Apr 8, 2022

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #67402

Author: simonrozsival
Assignees: -
Labels:

area-System.Net.Http
Milestone: -

@simonrozsival
Copy link
Member Author

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

eerhardt
eerhardt approved these changes Apr 8, 2022
View reviewed changes
Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. We may want to consider a write test, but given the code quickly calls into a common method I don't think it is crucial.

@lambdageek
Copy link
Member

/backport to release/6.0

@github-actions
Copy link
Contributor

github-actions bot commented Apr 8, 2022

Started backporting to release/6.0: https://github.com/dotnet/runtime/actions/runs/2115720964

@github-actions github-actions bot mentioned this pull request Apr 8, 2022
Merged
@simonrozsival
Copy link
Member Author

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jahmai-ca
Copy link

Thanks so much for fixing this. Is there a workaround that doesn't include disabling AOT?

@simonrozsival
Copy link
Member Author

The runtime-extra-platforms (Build tvOS arm64 Release AllSubsets_Mono) tests keep failing and I'm not sure why and how it is related to this change. I'm changing this PR to a draft while I investigate.

@simonrozsival simonrozsival marked this pull request as draft April 11, 2022 06:19
@simonrozsival
Copy link
Member Author

@jahmai-ca right now I don't know any other workaround

@simonrozsival
Revert "Fix vtable setup"
877b499 
This reverts commit daf631e.
@simonrozsival
Copy link
Member Author

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@simonrozsival
Copy link
Member Author

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@simonrozsival
Copy link
Member Author

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@simonrozsival simonrozsival marked this pull request as ready for review April 11, 2022 15:21
lambdageek
lambdageek approved these changes Apr 11, 2022
View reviewed changes
src/mono/mono/metadata/icall.c Outdated Show resolved Hide resolved
@simonrozsival
Copy link
Member Author

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@simonrozsival simonrozsival mentioned this pull request Apr 11, 2022
Closed
@simonrozsival
Copy link
Member Author

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@steveisok
Copy link
Member

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@runfoapp runfoapp bot mentioned this pull request Apr 11, 2022
Closed
@simonrozsival
Copy link
Member Author

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@simonrozsival
Copy link
Member Author

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@steveisok steveisok merged commit e74da7c into dotnet:main Apr 12, 2022
@rzikm rzikm mentioned this pull request Apr 22, 2022
Closed
@jonathanpeppers jonathanpeppers mentioned this pull request Apr 25, 2022
Closed
@ghost ghost locked as resolved and limited conversation to collaborators May 13, 2022
@karelz karelz added this to the 7.0.0 milestone Jul 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@eerhardt eerhardt eerhardt approved these changes

@lambdageek lambdageek lambdageek approved these changes

@steveisok steveisok steveisok approved these changes

@vargaz vargaz Awaiting requested review from vargaz

@thaystg thaystg Awaiting requested review from thaystg thaystg is a code owner

Assignees

@simonrozsival simonrozsival

Labels
area-System.Net.Http
Projects
None yet
Milestone
7.0.0
Development

Successfully merging this pull request may close these issues.

HttpClient crashes android app when trimmed
7 participants
@simonrozsival @lambdageek @jahmai-ca @steveisok @vargaz @eerhardt @karelz