Fix AV in IsTailCall by checking for NULL trace address

[Copilot]

Description

PRs #108942 and #108414 introduced TRACE_MULTICAST_DELEGATE_HELPER and TRACE_EXTERNAL_METHOD_FIXUP trace types that set address = NULL. This caused null reference exceptions in IsTailCall (controller.cpp) when calling GetNativeCodeMethodDesc() with a NULL address.

Per reviewer feedback, the NULL addresses for these trace types are by-design:

• TRACE_MULTICAST_DELEGATE_HELPER and TRACE_EXTERNAL_METHOD_FIXUP use callbacks that pass content to TraceManager() to determine where execution will go next
• The address field is not intended to be used for these trace types

Changes

• Fixed IsTailCall in src/coreclr/debug/ee/controller.cpp to check if trace.GetAddress() returns NULL before calling GetNativeCodeMethodDesc()

// Before (causes AV when GetAddress() returns NULL)
MethodDesc* pTargetMD =
    trace.GetTraceType() == TRACE_UNJITTED_METHOD
    ? trace.GetMethodDesc()
    : g_pEEInterface->GetNativeCodeMethodDesc(trace.GetAddress());

// After (safely handles NULL address)
MethodDesc* pTargetMD = NULL;
if (trace.GetTraceType() == TRACE_UNJITTED_METHOD)
{
    pTargetMD = trace.GetMethodDesc();
}
else if (trace.GetAddress() != (PCODE)NULL)
{
    pTargetMD = g_pEEInterface->GetNativeCodeMethodDesc(trace.GetAddress());
}

Note: There may be a separate tail-call stepping correctness issue for cross ReadyToRun image calls that should be tracked in a separate issue.

Original prompt

#108942 and #108414 introduced new types of TraceDestinations TRACE_MULTICAST_DELEGATE_HELPER and TRACE_EXTERNAL_METHOD_FIXUP. These types broke the expected invariant that GetAddress() works if the type is not TRACE_UNJITTED_METHOD and GetMethodDesc() works if the type is `TRACE_UNJITTED_METHOD.

Can you fix these types so that they have a valid PCODE address and modify the init funcions and invocations accordingly. We ran into null reference exceptions in IsTailCall in controller.cpp as trace.GetAddress() returned NULL.

---

💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

[dotnet-policy-service]

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

[Copilot]

Pull request overview

This PR fixes a broken invariant in the TraceDestination class where GetAddress() should return a valid PCODE for all trace types except TRACE_UNJITTED_METHOD. PRs #108942 and #108414 introduced two new trace types (TRACE_MULTICAST_DELEGATE_HELPER and TRACE_EXTERNAL_METHOD_FIXUP) that incorrectly set address = NULL, causing null reference exceptions in debugger code.

Changes:

• Updated InitForMulticastDelegateHelper and InitForExternalMethodFixup to accept a PCODE address parameter
• Modified both call sites to pass stubStartAddress instead of leaving the address as NULL
• Ensures the invariant that all non-TRACE_UNJITTED_METHOD trace types have valid addresses

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
src/coreclr/vm/stubmgr.h	Added PCODE addr parameter to InitForMulticastDelegateHelper and InitForExternalMethodFixup methods, and updated both to assign the address to this->address
src/coreclr/vm/stubmgr.cpp	Updated call sites in RangeSectionStubManager::DoTraceStub and ILStubManager::DoTraceStub to pass stubStartAddress to the Init methods

[rcj1]

I do not think stubStartAddress is meaningful in the case of InitForExternalMethodFixup. It will fall in here

runtime/src/coreclr/vm/codeman.cpp

Line 6450 in 061f153

StubCodeBlockKind ReadyToRunJitManager::GetStubCodeBlockKind(RangeSection * pRangeSection, PCODE currentPC)

, you cannot get a method desc from it, and any further operations will be meaningless. I would suggest leaving the address null for external fixup and adding back the change to check if the address is null; the one spot that I found is also the only piece of code where the result of GetAddress() is operated on without checking if address is null, and that is a bug.

runtime/src/coreclr/vm/codeman.cpp

Line 6688 in 061f153

BOOL ReadyToRunJitManager::JitCodeToMethodInfo(RangeSection * pRangeSection,

runtime/src/coreclr/vm/jitinterface.cpp

Line 15181 in 061f153

if (!pRS->_pjit->JitCodeToMethodInfo(pRS, codeAddress, &m_pMD, this))

[noahfalk]

fyi @lateralusX

These TraceDestination addresses became NULL by-design as part of Johan's changes to the stubmanager. Although we could potentially put a non-NULL address in that fields I think we'd need to decide what that address is supposed to mean. Right now if you call TraceStub/FollowTrace the TraceDestination you get back has three potential meanings:

1. TRACE_UNJITTED_METHOD - The MethodDesc field describes a method that will be jitted and executed in the future, code address not yet known.
2. TRACE_MULTICAST_DELEGATE_HELPER and TRACE_EXTERNAL_METHOD_FIXUP - A callback can be turned on and the content of that callback can be passed to TraceManager() to determine where execution will go next.
3. The others - The address is some IP that will be executed in the future where a breakpoint can be placed and then further actions taken from there.

Putting the stubStartAddress in the TraceDestination field doesn't line up with how case (3) uses the address and none of the other cases are intended to use the address at all.

Checking the address for NULL in IsTailCall() would resolve the immediate AV issue but I suspect we may also have a tailcall stepping issue as well. In particular if ReadyToRun image A has method Foo1() which tailcalls to Foo2() then Foo1() probably has a call to one of the tailcall helpers at the end that might be indirected through an ExternalMethodFixup. The code of IsTailCall() assumes that given the call IP we can immediately determine the destination MethodDesc that will be invoked but that isn't something the StubManager for ExternalMethodFixup figures out immediately. We'd need to redesign something to make that work.

I think it would be fine to fix the AV now and file a separate issue about tail-call stepping correctness in ReadyToRun assemblies.

[lateralusX]

fyi @lateralusX

These TraceDestination addresses became NULL by-design as part of Johan's changes to the stubmanager. Although we could potentially put a non-NULL address in that fields I think we'd need to decide what that address is supposed to mean. Right now if you call TraceStub/FollowTrace the TraceDestination you get back has three potential meanings:

1. TRACE_UNJITTED_METHOD - The MethodDesc field describes a method that will be jitted and executed in the future, code address not yet known.
2. TRACE_MULTICAST_DELEGATE_HELPER and TRACE_EXTERNAL_METHOD_FIXUP - A callback can be turned on and the content of that callback can be passed to TraceManager() to determine where execution will go next.
3. The others - The address is some IP that will be executed in the future where a breakpoint can be placed and then further actions taken from there.

Putting the stubStartAddress in the TraceDestination field doesn't line up with how case (3) uses the address and none of the other cases are intended to use the address at all.

Checking the address for NULL in IsTailCall() would resolve the immediate AV issue but I suspect we may also have a tailcall stepping issue as well. In particular if ReadyToRun image A has method Foo1() which tailcalls to Foo2() then Foo1() probably has a call to one of the tailcall helpers at the end that might be indirected through an ExternalMethodFixup. The code of IsTailCall() assumes that given the call IP we can immediately determine the destination MethodDesc that will be invoked but that isn't something the StubManager for ExternalMethodFixup figures out immediately. We'd need to redesign something to make that work.

I think it would be fine to fix the AV now and file a separate issue about tail-call stepping correctness in ReadyToRun assemblies.

I was not part of the original design/change, but I have been working on a rewrite of those changes + adding some more stubs, but none of that work has been merged yet (put it on hold in favor for other work):

https://github.com/dotnet/runtime/compare/v9.0.7...lateralusX:runtime:lateralusX/add-sw-breakpoint-support-callback?expand=1

Since putting the tracepoint address to NULL in the stub manager scenario was by design, it is also carried over into my rewrite above, but that includes a change in IsTailCall to detect when we have TRACE_TRACEPOINT, so it would not hit the issue accessing NULL trace.GetAddress in IsTailCall. If that hits other stepping issues is currently unknown, like described above, but that would at least have prevented the crash.

I know we discussed fixing the IsTailCall in the past by checking !trace.GetAddress() and return false. I agree that we should implement that fix now and then look at potential stepping issues when continue working on the rewrite of this implementation in the branch above.

[max-charlamb]

@copilot , address the feedback. Revert the current changes adding the stub address to the trace types and fix the AV bug in IsTailCall.

Open an issue to track the bug in IsTailCall not respecting certain cross R2R image calls.

[Copilot]

@copilot , address the feedback. Revert the current changes adding the stub address to the trace types and fix the AV bug in IsTailCall.

Open an issue to track the bug in IsTailCall not respecting ...

Reverted the stubmgr changes and fixed the AV in IsTailCall by checking for NULL trace.GetAddress() before calling GetNativeCodeMethodDesc(). See commit 0eec95b.

Note: I cannot open GitHub issues from this environment. Please create an issue to track the tail-call stepping correctness bug for cross ReadyToRun image calls.

[max-charlamb]

@copilot , address build failure

  /__w/1/s/src/coreclr/debug/ee/controller.cpp:5839:33: error: invalid operands to binary expression ('PCODE' (aka 'unsigned long') and 'std::nullptr_t')
   5839 |     else if (trace.GetAddress() != NULL)
        |              ~~~~~~~~~~~~~~~~~~ ^  ~~~~

[Copilot]

@copilot , address build failure

  /__w/1/s/src/coreclr/debug/ee/controller.cpp:5839:33: error: invalid operands to binary expression ('PCODE' (aka 'unsigned long') and 'std::nullptr_t')

...

Fixed the build error by using (PCODE)NULL for the comparison, matching existing patterns in the codebase. See commit 99584e4.

[max-charlamb]

/ba-g Test failures are unrelated