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

[NativeAOT] Adding CET support #102680

Merged
merged 18 commits into from
Jul 2, 2024
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Allow hijacked returns that land in assembly thunks
  • Loading branch information
VSadov committed Jul 1, 2024
commit 0f0c3b6bb93f69e5a0b35011b772f332ae97674d
28 changes: 26 additions & 2 deletions src/coreclr/nativeaot/Runtime/StackFrameIterator.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -498,8 +498,9 @@ void StackFrameIterator::InternalInit(Thread * pThreadToWalk, NATIVE_CONTEXT* pC
// properly walk it in parallel.
ResetNextExInfoForSP(pCtx->GetSp());

// This codepath is used by the hijack stackwalk. The IP must be in managed code.
ASSERT(m_pInstance->IsManaged(dac_cast<PTR_VOID>(pCtx->GetIp())));
// This codepath is used by the hijack stackwalk. The IP must be in managed code
// or in a conservatively reported assembly thunk.
ASSERT(IsValidReturnAddress((void*)pCtx->GetIp()));

//
// control state
Expand Down Expand Up @@ -616,6 +617,29 @@ void StackFrameIterator::InternalInit(Thread * pThreadToWalk, NATIVE_CONTEXT* pC
#endif // TARGET_ARM

#undef PTR_TO_REG

// This function guarantees that the final initialized context will refer to a managed
// frame. In the rare case where the PC does not refer to managed code (and refers to an
// assembly thunk instead), unwind through the thunk sequence to find the nearest managed
// frame.
// NOTE: When thunks are present, the thunk sequence may report a conservative GC reporting
// lower bound that must be applied when processing the managed frame.

ReturnAddressCategory category = CategorizeUnadjustedReturnAddress(m_ControlPC);
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure how this is related to the CET stuff

Copy link
Member Author

Choose a reason for hiding this comment

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

InternalInit is supposed to leave the iterator in managed code. Until this change the InternalInit that takes NATIVE_CONTEXT was always starting from a location that is already in managed code. (unlike the one that takes PInvokeTransitionFrame)

Now, when we receive hijack exception, we pop to the caller and perform suspension in the caller's context.
Typically managed method that is not a reverse PInvoke must only be called by another managed method, so that would require no changes here. There are rare cases though in NativeAOT when ordinary managed methods are called from asm thunks. So it becomes possible for this code to see a location in a thunk. That is handled in the same way as in the other InternalInit method. (i.e. via UnwindNonEHThunkSequence)

Perhaps I should unify this code into a helper to not duplicate it in both InternalInit methods.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've unified the adjustment for asm thunks


if (category == InManagedCode)
{
ASSERT(m_pInstance->IsManaged(m_ControlPC));
}
else if (IsNonEHThunk(category))
{
UnwindNonEHThunkSequence();
ASSERT(m_pInstance->IsManaged(m_ControlPC));
}
else
{
FAILFAST_OR_DAC_FAIL_UNCONDITIONALLY("NATIVE_CONTEXT PC points to an unexpected assembly thunk kind.");
}
}

PTR_VOID StackFrameIterator::HandleExCollide(PTR_ExInfo pExInfo)
Expand Down