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

JIT: Disallow mismatched GC-ness for physical promotions #90694

Merged
merged 2 commits into from
Aug 17, 2023

Conversation

jakobbotsch
Copy link
Member

Physical promotion was working under the assumption that reinterpreting GC pointers is undefined behavior, and would happily promote GC pointers as integers if it saw such accesses. However, physical promotion is function wide while the UB accesses can be happening in a restricted (dynamically unreachable) scope. This exact situation happens in MemoryExtensions.Contains. The issue was uncovered under jit stress where we did not fold away the guard early enough, meaning that promotion then saw a TYP_LONG access of a struct { object, int } and proceeded to promote it as such.

Fix #90602

No diffs are expected.

Physical promotion was working under the assumption that reinterpreting
GC pointers is undefined behavior, and would happily promote GC pointers
as integers if it saw such accesses. However, physical promotion is
function wide while the UB accesses can be happening in a restricted
(dynamically unreachable) scope. This exact situation happens in
MemoryExtensions.Contains. The issue was uncovered under jit stress
where we did not fold away the guard early enough, meaning that
promotion then saw a `TYP_LONG` access of a `struct { object, int }` and
proceeded to promote it as such.

Fix dotnet#90602
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Aug 16, 2023
@ghost ghost assigned jakobbotsch Aug 16, 2023
@ghost
Copy link

ghost commented Aug 16, 2023

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Issue Details

Physical promotion was working under the assumption that reinterpreting GC pointers is undefined behavior, and would happily promote GC pointers as integers if it saw such accesses. However, physical promotion is function wide while the UB accesses can be happening in a restricted (dynamically unreachable) scope. This exact situation happens in MemoryExtensions.Contains. The issue was uncovered under jit stress where we did not fold away the guard early enough, meaning that promotion then saw a TYP_LONG access of a struct { object, int } and proceeded to promote it as such.

Fix #90602

No diffs are expected.

Author: jakobbotsch
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

@jakobbotsch jakobbotsch reopened this Aug 16, 2023
@jakobbotsch jakobbotsch marked this pull request as ready for review August 17, 2023 07:58
@jakobbotsch
Copy link
Member Author

cc @dotnet/jit-contrib PTAL @EgorBo

No diffs.

@jakobbotsch jakobbotsch requested a review from EgorBo August 17, 2023 08:48
Copy link
Member

@EgorBo EgorBo left a comment

Choose a reason for hiding this comment

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

Does it need some fast path when lcl's layout is known to not contain any gc pointers? Although, I guess zero tp diffs say no.

@jakobbotsch
Copy link
Member Author

Does it need some fast path when lcl's layout is known to not contain any gc pointers? Although, I guess zero tp diffs say no.

Can't hurt to add it to the general helper function.

@jakobbotsch jakobbotsch merged commit 45acd38 into dotnet:main Aug 17, 2023
@jakobbotsch jakobbotsch deleted the fix-90602 branch August 17, 2023 14:16
@jakobbotsch
Copy link
Member Author

/azp backport to release/8.0

@jakobbotsch
Copy link
Member Author

/backport to release/8.0

@github-actions
Copy link
Contributor

Started backporting to release/8.0: https://github.com/dotnet/runtime/actions/runs/5892120122

@ghost ghost locked as resolved and limited conversation to collaborators Sep 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Assert failure: !CREATE_CHECK_STRING(pMT && pMT->Validate())
3 participants