[release/10.0] Fix GC hole when method return is hijacked for GC suspension - #131708
Open
steveisok wants to merge 1 commit into
Open
[release/10.0] Fix GC hole when method return is hijacked for GC suspension#131708steveisok wants to merge 1 commit into
steveisok wants to merge 1 commit into
Conversation
…ension Backport of dotnet#129714. Except on x86, no registers are scanned as part of the HijackFrame on top of the stack. The hijacked return value is only reported to the GC through the calling method's GC info. When the caller is CallDescrWorkerInternal, which is hand written assembly with no GC info, an objectref left in the return register at hijack time is neither reported nor updated, so a compacting GC leaves the caller holding a stale pointer and the heap gets corrupted. Fix this by declining to hijack when the return address is CallDescrWorkerInternal's. The upstream change also strips the PAC signature from the return address and handles returns into the interpreter. Neither applies here: release/10.0 has no PacStripPtr and no FEATURE_INTERPRETER, so only the CallDescrWorkerInternal early-out is taken. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 9d15f64b-d86c-4a00-ba47-5d08f2b5f2cf
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
Contributor
|
Tagging subscribers to this area: @agocke |
Contributor
There was a problem hiding this comment.
Pull request overview
Backports the coreclr suspension/hijack safety check to release/10.0 to prevent a GC hole when a thread is hijacked while returning into CallDescrWorkerInternal (which has no GC info), avoiding untracked/unupdated objectrefs in the return register on non-x86 architectures.
Changes:
- Adds a non-x86 early-out in
Thread::HijackThreadwhen the return address isCallDescrWorkerInternal. - Introduces a local forward declaration for
IsCallDescrWorkerInternalReturnAddressto reuse the existing helper.
Member
|
I have fixed inaccurate description. In particular, the original description said that this is not a regression that's not correct. This is fixing .NET 10 regression |
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Fixes Issue #131267
main PR #129714
Description
Except on x86, no registers are scanned as part of the
HijackFrameon top of the stack —HijackFrame::GcScanRoots_Implis compiled only under#ifdef TARGET_X86. The hijacked return value is instead reported to the GC through the calling method's GC info. That works as long as the caller is managed code.It breaks when the caller is
CallDescrWorkerInternal, which is hand-written assembly with no GC info. An objectref left in the return register (x0/rax) at hijack time is then neither reported nor updated.This declines to hijack when the return address is
CallDescrWorkerInternal's, which is what the main fix does.Customer Impact
Reported by Gearset, a commercial Salesforce DevOps product, as intermittent SIGSEGV test-host crashes in their CI on released 10.0.x. Their suite compiles LINQ
Expressions and Moq mocks (LCGDynamicMethodemit) while a parallel test runner drives GC load. Crashes are rare per run but frequent enough across a day's builds to force manual re-runs and stall their merge queue, and there is no managed-code workaround.Any
Call_RetOBJECTREFcall site is exposed — there are 33 inrelease/10.0— as is reflection invoke viaMethodBaseInvoker.InterpretedInvoke_Method->RuntimeMethodHandle_InvokeMethod->CallDescrWorkerInternal. Symptoms are silent heap corruption: SIGSEGV ingc_heap::mark_object_simple, orObject::ValidateInner/GetMethodTableasserts on checked builds.Regression
Yes. This is a regression introduced in .NET 10 by #110799
Testing
Local A/B on a checked
release/10.0build (osx-arm64), swapping onlylibcoreclr.dylib, 8 runs x 30 s each against a reflection-invoke repro that returns abyte[]from a long straight-line (partially interruptible) region under concurrentGC.Collect:release/10.0unpatchedObject::ValidateInner(pMT->Validate()), 2xGetMethodTableMARKED_BITassertThe same repro also faults on shipped 10.0.9 (SIGSEGV in
WKS::gc_heap::mark_object_simple) and on 11.0.0-preview.6, and passes on currentmain, which already carries #129714.Separately, an instrumented build logging every hijack recorded one landing directly on
System.Reflection.Emit.DynamicResolver::GetCodeInfowith the return address insideCallDescrWorkerInternal— i.e. thebyte[]behindLCGMethodResolver::GetCodeInfolive and unreported inx0. Reaching that required widening the race window, since hijacking is rare in that workload../build.sh clr -rc checkedis clean (0 errors, 0 warnings). All local runs are osx-arm64; CI covers the rest of the matrix.Risk
Low. 15 lines in one function, guarded to non-x86.
The change only declines an optimization: hijacking is one of several ways to bring a thread to a safe point. A thread returning into
CallDescrWorkerInternalis leaving managed code and will hit a preemptive-mode transition, so suspension still completes — it is not a hang risk.The equivalent change has been in
mainsince 2026-06-23 (#129714).Note
This pull request description was drafted with the help of GitHub Copilot.