Skip to content

Unify StubLinker stub memory management with JIT code allocator - #1

Open
anurag6569201 wants to merge 1 commit into
qa/agent-dotnet-runtime/pr-01-131903/basefrom
qa/agent-dotnet-runtime/pr-01-131903/head
Open

anurag6569201 wants to merge 1 commit into
qa/agent-dotnet-runtime/pr-01-131903/basefrom
qa/agent-dotnet-runtime/pr-01-131903/head

Conversation

@anurag6569201

Copy link
Copy Markdown

StubLinker-generated stubs used a dedicated LoaderAllocator::m_pStubHeap, separate from the executable memory infrastructure shared by JITed code and other stub kinds. This PR unifies the two paths.

  • LoaderAllocator: removed m_pStubHeap and its init/terminate/DAC/size-reporting plumbing.
  • StubLinker/Stub: StubLinker::Link and Stub::NewStub now take a LoaderAllocator* instead of a LoaderHeap*, allocating stub memory through ExecutionManager::GetEEJitManager()->AllocCodeFragmentBlock.
  • StubCodeBlockKind: added STUB_CODE_BLOCK_STUBLINK and STUB_CODE_BLOCK_SHUFFLE_THUNK to distinguish these stubs in the code-fragment allocator.
  • StubLinkStubManager: removed its own range list; RangeSectionStubManager now recognizes the new stub kinds and forwards stub identification/tracing to StubLinkStubManager (mirroring the existing pattern for jump stubs and call-counting stubs).
  • DAC/SOS: updated datadescriptor.inc and debug/daccess/request.cpp to drop the removed stub heap field.
  • Call sites: updated stubcache.cpp, comdelegate.cpp, prestub.cpp, assembly.cpp, appdomain.hpp/.inl, and class.h to pass LoaderAllocator* instead of a stub LoaderHeap*.
// Before
Stub *Link(LoaderHeap *heap, DWORD flags, const char *stubType);

// After
Stub *Link(LoaderAllocator *pLoaderAllocator, DWORD flags, const char *stubType);

Source merge-base: c36f112e7ad108b8f52e5d7028d2ecc09a4015f2
Source head: bb70c03b305a8f424e9924bb2defc66bcd7424b1

@shipwright-agent

Copy link
Copy Markdown

⛔ Shipwright · Blocked

Recommendation: do not merge PR #1 · Tier T3
Checks: 0 total · 0 needing attention

Next step: resolve the blocking findings before merge.

Findings (11)

  • CRITICAL The StubCodeBlockKind enum values are renumbered (e.g. · src/coreclr/vm/codeman.h:98
    • Fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
  • CRITICAL In SetupShuffleThunk, the InterlockedCompareExchangeT now stores a raw PCODE and on contention the losing thread simply discards its newly created thunk without any DecRef or clean · src/coreclr/vm/comdelegate.cpp:931
    • Fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
  • CRITICAL StubLinkStubManager::CheckIsStub_Internal now unconditionally returns FALSE, and DoEnumMemoryRegions no longer enumerates the range list. · src/coreclr/vm/stubmgr.cpp:1259
    • Fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
  • CRITICAL GetStubTargetMethod reads a PTR_MethodDesc from immediately before the stub start address (pStubData - sizeof(PTR_MethodDesc)) without any validation that the memory layout actuall · src/coreclr/vm/stubmgr.cpp:1268
    • Fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
  • CRITICAL The removal of StubHolder and the Stub class's refcount-based lifetime management means all call sites that previously relied on automatic DecRef on scope exit now leak or double-f · src/coreclr/inc/holder.h:1128
    • Fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
  • CRITICAL The change from Stub::RecoverStub to GetStubTargetMethod removes the debug-only signature validation (kUsedStub) and the assertion that the entry point matches the stub. · src/coreclr/vm/stubmgr.cpp:1268
    • Fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
  • CRITICAL The removal of the range list enumeration in StubLinkStubManager::DoEnumMemoryRegions means DAC memory enumeration no longer includes stub ranges. · src/coreclr/vm/stubmgr.cpp:2267
    • Fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
  • HIGH The enum renumbering in StubCodeBlockKind is not accompanied by a comment explaining why the values changed or whether the numeric values are part of a serialized contract. · src/coreclr/vm/codeman.h:98
    • Fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
  • …and 3 more findings in the check details.

Fireworks usage: 59,680 input · 1,408 output · 61,088 total tokens · $0.0141 · 23s · 0 fix iteration(s)

Open the Shipwright check for full evidence and the audit bundle. Use /shipwright rerun to verify again.

}

typedef DPTR(PTR_MethodDesc) PTR_PTR_MethodDesc;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Shipwright · CRITICAL

GetStubTargetMethod reads a PTR_MethodDesc from immediately before the stub start address (pStubData - sizeof(PTR_MethodDesc)) without any validation that the memory layout actuall

Impact: GetStubTargetMethod reads a PTR_MethodDesc from immediately before the stub start address (pStubData - sizeof(PTR_MethodDesc)) without any validation that the memory layout actually contains a MethodDesc pointer. If the stub kind is not WRAPPER_STUB or the allocation layout differs, this dereferences arbitrary memory and can crash or corrupt state.

Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.

}

typedef DPTR(PTR_MethodDesc) PTR_PTR_MethodDesc;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Shipwright · CRITICAL

The change from Stub::RecoverStub to GetStubTargetMethod removes the debug-only signature validation (kUsedStub) and the assertion that the entry point matches the stub.

Impact: The change from Stub::RecoverStub to GetStubTargetMethod removes the debug-only signature validation (kUsedStub) and the assertion that the entry point matches the stub. In DAC builds, the old code explicitly checked m_signature and GetEntryPoint before dereferencing. The new code blindly reads a pointer before the stub address, creating a memory-safety vulnerability where a crafted or corrupted dump can cause arbit…

Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.


extern "C" void STDCALL JIT_PatchedCodeStart();
extern "C" void STDCALL JIT_PatchedCodeLast();
#ifndef TARGET_X86

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Shipwright · HIGH

The new GetCodeSize helper uses a ptrdiff_t subtraction and asserts codeSize > 0, but the callers pass symbol addresses that may not be ordered correctly on all platforms or build

Impact: The new GetCodeSize helper uses a ptrdiff_t subtraction and asserts codeSize > 0, but the callers pass symbol addresses that may not be ordered correctly on all platforms or build configurations. If the linker places JIT_WriteBarrier_End before JIT_WriteBarrier, the assert fires in release builds or the size is silently wrong.

Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant