JIT: Fix patchpoint info out-of-bounds read for special OSR locals#125378
Merged
jakobbotsch merged 2 commits intodotnet:mainfrom Mar 10, 2026
Merged
JIT: Fix patchpoint info out-of-bounds read for special OSR locals#125378jakobbotsch merged 2 commits intodotnet:mainfrom
jakobbotsch merged 2 commits intodotnet:mainfrom
Conversation
github-actions
Bot
added
the
area-CodeGen-coreclr
Contributor
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes an OSR JIT crash caused by computing tier0 stack offsets for special OSR locals (MonitorAcquired and async context locals) using the normal patchpoint local-offset table, which can lead to out-of-bounds reads when those locals are enregistered.
Changes:
- Introduced
Compiler::lvaOSRLocalTier0FrameOffsetto centralize tier0 frame offset lookup for OSR locals, including special-case locals. - Updated OSR virtual frame offset assignment and OSR enregistration initialization to use the new helper instead of
PatchpointInfo::Offset. - Removed duplicated special-local offset logic from
lvaAssignVirtualFrameOffsetsToLocals.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/coreclr/jit/lclvars.cpp | Uses the new helper for tier0 frame offset computation (including promoted fields) to avoid OOB reads. |
| src/coreclr/jit/compiler.h | Declares lvaOSRLocalTier0FrameOffset. |
| src/coreclr/jit/compiler.cpp | Implements the helper with explicit handling for special OSR locals and bounds assertions for normal locals. |
| src/coreclr/jit/codegencommon.cpp | Uses the helper when initializing enregistered OSR locals from the tier0 frame. |
Member
Author
|
cc @dotnet/jit-contrib PTAL @AndyAyersMS |
AndyAyersMS
approved these changes
Mar 10, 2026
Member
AndyAyersMS
left a comment
AndyAyersMS
left a comment
There was a problem hiding this comment.
Interesting that this never came up before.
Copilot AI
pushed a commit
that referenced
this pull request
Mar 13, 2026
…125378) `genEnregisterOSRArgsAndLocals` was not properly handling computing the stack offset for special OSR locals like `MonitorAcquired` and the async contexts. It meant that if these ended up enregistered we would hit an out-of-bounds when trying to obtain their offset. This was a regression introduced by #121672 which started marking these 3 locals as OSR locals. Before that we only had `MonitorAcquired`, and I suspect that one was never enregistered, so even though it was not handled by `genEnregisterOSRArgsAndLocals` that was never a problem. However, the async contexts can be enregistered in the OSR function so they do need to be handled here.
This file contains hidden or 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
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.
genEnregisterOSRArgsAndLocalswas not properly handling computing the stack offset for special OSR locals likeMonitorAcquiredand the async contexts. It meant that if these ended up enregistered we would hit an out-of-bounds when trying to obtain their offset.Fix #125255
This was a regression introduced by #121672 which started marking these 3 locals as OSR locals. Before that we only had
MonitorAcquired, and I suspect that one was never enregistered, so even though it was not handled bygenEnregisterOSRArgsAndLocalsthat was never a problem. However, the async contexts can be enregistered in the OSR function so they do need to be handled here.