Skip to content

Fix minidump generation when there is a NDirect method in the stack #116391

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

thaystg
Copy link
Member

@thaystg thaystg commented Jun 6, 2025

Only get the il from PE if it's not nDirect Method because the IL of a NDirect is dynamically generated so it does not make sense to get it from the PE.

@thaystg thaystg requested review from tommcdon and Copilot June 6, 2025 20:22
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR aims to fix minidump generation by avoiding retrieval of IL from the PE for NDirect methods, since their IL is dynamically generated.

  • Added an extra check (!IsNDirect()) before attempting to retrieve the IL header.
  • Ensures that the IL header is only fetched for non-NDirect methods.
Comments suppressed due to low confidence (1)

src/coreclr/vm/method.cpp:1211

  • [nitpick] Consider adding a brief comment explaining that NDirect methods generate dynamic IL and therefore do not require IL header retrieval from the PE. This could enhance code clarity for future maintainers.
if (pIL == (TADDR)NULL && !IsNDirect())

@github-actions github-actions bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Jun 6, 2025
@tommcdon tommcdon added area-Diagnostics-coreclr and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Jun 6, 2025
Copy link
Contributor

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

@jkotas
Copy link
Member

jkotas commented Jun 7, 2025

Only get the il from PE if it's not nDirect Method because the IL of a NDirect is dynamically generated so it does not make sense to get it from the PE.

Could you please share stacktrace for the crash that this is fixing?

I do not see the problem with the current code. GetRVA should return 0 for (NDirect) P/Invoke methods. Module::GetIL has early our for RVA == 0.

If there is a problem with this path, there are likely more types of methods that we need to check here (e.g. FCalls, etc.).

@@ -1208,7 +1208,7 @@ COR_ILMETHOD* MethodDesc::GetILHeader()
// Always pickup overrides like reflection emit, EnC, etc.
Copy link
Member

Choose a reason for hiding this comment

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

Notice that this method has a PRECONDITION(IsIL()). It means that it is not expected to be called on IsNDirect method in the first place.

I think the correct fix is to add if (IsIL()) check around the whole CodeVersionManager related block in MethodDesc::EnumMemoryRegions.

@jkotas
Copy link
Member

jkotas commented Jun 7, 2025

GetRVA should return 0 for (NDirect) P/Invoke methods. Module::GetIL has early our for RVA == 0.

One possible explanation is that this is IJW module (are you able to confirm it with the customer?) and GetRVA returns the address of native code in that case. We try to interpret the native code as IL and crash.

@jkotas jkotas closed this Jun 7, 2025
@jkotas jkotas reopened this Jun 7, 2025
@tommcdon
Copy link
Member

tommcdon commented Jun 7, 2025

Could you please share stacktrace for the crash that this is fixing?

mscordaccore_AMD64_AMD64.8.0.1224.60305.dll!DacError(HRESULT err) Line 102
mscordaccore_AMD64_AMD64.8.0.1224.60305.dll!DacInstantiateTypeByAddressHelper(unsigned __int64 addr, unsigned int size, bool t...
[Inlined Frame] mscordaccore_AMD64_AMD64.8.0.1224.60305.dll!DacInstantiateTypeByAddress(unsigned __int64 addr) Line 449
[Inlined Frame] mscordaccore_AMD64_AMD64.8.0.1224.60305.dll!_DPtr<tag<COR_ILMETHOD_SECT_SMALL>::operator->() Line 1202
mscordaccore_AMD64_AMD64.8.0.1224.60305.dll!PEDecoder::ComputeILMethodSize(unsigned __int64 pIL) Line 2303
[Inlined Frame] mscordaccore_AMD64_AMD64.8.0.1224.60305.dll!DacGetIlMethod(unsigned __int64) Line 1325
mscordaccore_AMD64_AMD64.8.0.1224.60305.dll!MethodDesc::GetILHeader(int fAllowOverwrites) Line 1100
mscordaccore_AMD64_AMD64.8.0.1224.60305.dll!ILCodeVersion::GetIL() Line 923
mscordaccore_AMD64_AMD64.8.0.1224.60305.dll!MethodDesc::EnumMemoryRegions(CLRDataEnumMemoryFlags flags) Line 3799

One possible explanation is that this is IJW module (are you able to confirm it with the customer?) and GetRVA returns the address of native code in that case. We try to interpret the native code as IL and crash.

Confirmed that it is an IJW app

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants