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.

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

[mono][s390x] Wrong implementation of OP_CHECK_THIS #76915

Closed
uweigand opened this issue Oct 12, 2022 · 2 comments · Fixed by #76916
Closed

[mono][s390x] Wrong implementation of OP_CHECK_THIS #76915

uweigand opened this issue Oct 12, 2022 · 2 comments · Fixed by #76916

Comments

@uweigand
Copy link
Contributor

As described here #75996 (comment) I've seen frequent crashes with rc1 on s390x (Mono-based big-endian platform) - these were all segmentation faults in SpanHelpers.IndexOfValueType called from various different places, caused by accessing beyond the end of a page into an unmapped page.

The symptom disappeared after the SpanHelper changes were reverted on Mono, so I initially thought it was somehow a problem in those changes. However, as it turns out, the real underlying problem is a Mono codegen bug on s390x.

The accesses that span the page boundary turn out to be emitted from the implementation of the OP_CHECK_THIS opcode. This is intended to perform a memory access, just to see if this triggers a segmentation fault if the incoming pointer was invalid. However, the OP_CHECK_THIS implementation currently always emits an 8-byte memory access - potentially resulting in a false positive if the pointer was valid, but only to access less than 8 bytes.

This should be fixed by changing OP_CHECK_THIS to only access a single byte. I'll submit a PR to that effect shortly.

This should probably be backported to .NET 7 as well, even though ithe bug no longer triggers in the runtime itself - it could still be triggered by other code.

FYI @nealef @lambdageek @vargaz @akoeplinger

Also FYI @janani66 - you should look into that on ppc as well, just from code inspection the ppc code appears to have the same bug.

@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Oct 12, 2022
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Oct 12, 2022
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Oct 12, 2022
akoeplinger pushed a commit that referenced this issue Oct 12, 2022
* Only access a single byte in memory for OP_CHECK_THIS

* Remove unnecessary ltgr instruction

* Fixes #76915
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Oct 12, 2022
@akoeplinger
Copy link
Member

This should probably be backported to .NET 7 as well, even though ithe bug no longer triggers in the runtime itself - it could still be triggered by other code.

I've opened the backport PR but as far as I know it's too late for 7.0 release so it'll have to wait for 7.0.1 servicing

github-actions bot pushed a commit that referenced this issue Oct 12, 2022
* Only access a single byte in memory for OP_CHECK_THIS

* Remove unnecessary ltgr instruction

* Fixes #76915
carlossanlop pushed a commit that referenced this issue Oct 13, 2022
* Only access a single byte in memory for OP_CHECK_THIS

* Remove unnecessary ltgr instruction

* Fixes #76915

Co-authored-by: Ulrich Weigand <ulrich.weigand@de.ibm.com>
@ghost ghost locked as resolved and limited conversation to collaborators Nov 11, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants