From 6a4fedabede1dd00955c45e11591cfa397df79fe Mon Sep 17 00:00:00 2001 From: Aaron R Robinson Date: Wed, 18 Feb 2026 16:23:12 -0800 Subject: [PATCH 1/5] Fix P/Invoke IL stub race in DoPrestub Re-enable ILStubCache lookup/insert for forward P/Invoke stubs to prevent racing threads from independently creating and JIT-ing duplicate IL stubs for the same target method. PR #117901 made IsSharedStubScenario return false for forward P/Invokes, which disabled the cache. Multiple threads could then each create their own DynamicMethodDesc and JIT it, producing different PCODE values that violated the assert in DoPrestub. The fix: - Remove the forward P/Invoke exclusion from IsSharedStubScenario so these stubs use the ILStubCache for de-duplication. - In CreateHashBlob, create a minimal hash blob containing just the target MethodDesc pointer for forward P/Invoke stubs, so different P/Invoke methods get distinct cache entries while racing threads for the same method converge on the same DynamicMethodDesc. Fixes #124530 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/coreclr/vm/dllimport.cpp | 68 ++++++++++++++++++++++-------------- 1 file changed, 41 insertions(+), 27 deletions(-) diff --git a/src/coreclr/vm/dllimport.cpp b/src/coreclr/vm/dllimport.cpp index afa5b7a2401458..58c264be6f2d14 100644 --- a/src/coreclr/vm/dllimport.cpp +++ b/src/coreclr/vm/dllimport.cpp @@ -229,10 +229,10 @@ static bool IsSharedStubScenario(DWORD dwStubFlags) return false; } - if (SF_IsForwardPInvokeStub(dwStubFlags) && !SF_IsCALLIStub(dwStubFlags) && !SF_IsVarArgStub(dwStubFlags)) - { - return false; - } + // Forward P/Invoke stubs (non-CALLI, non-vararg) are not shared across different + // target methods, but they still use the ILStubCache for de-duplication when + // multiple threads race to create a stub for the same target. The hash blob for + // these stubs is keyed by target MethodDesc rather than by signature. return true; } @@ -4081,10 +4081,32 @@ namespace PCCOR_SIGNATURE pvNativeType; }; - ILStubHashBlob* CreateHashBlob(PInvokeStubParameters* pParams) + ILStubHashBlob* CreateHashBlob(PInvokeStubParameters* pParams, MethodDesc* pTargetMD) { STANDARD_VM_CONTRACT; + DWORD dwStubFlags = pParams->m_dwStubFlags; + + // The target MethodDesc may be NULL for field marshalling. + // Forward P/Invoke stubs (non-CALLI, non-vararg) each target a specific method, + // so the hash blob is just the target MethodDesc pointer. This ensures racing + // threads for the same P/Invoke converge on the same DynamicMethodDesc in the + // ILStubCache, while different P/Invoke methods get distinct cache entries. + if (pTargetMD != NULL + && SF_IsForwardPInvokeStub(dwStubFlags) + && !SF_IsCALLIStub(dwStubFlags) + && !SF_IsVarArgStub(dwStubFlags)) + { + size_t blobSize = sizeof(ILStubHashBlobBase) + sizeof(MethodDesc*); + NewArrayHolder pBytes = new BYTE[blobSize]; + ZeroMemory(pBytes, blobSize); + ILStubHashBlob* pBlob = (ILStubHashBlob*)(BYTE*)pBytes; + pBlob->m_cbSizeOfBlob = blobSize; + memcpy(pBlob->m_rgbBlobData, &pTargetMD, sizeof(pTargetMD)); + pBytes.SuppressRelease(); + return pBlob; + } + PInvokeStubHashBlob* pBlob; IMDInternalImport* pInternalImport = pParams->m_pModule->GetMDImport(); @@ -4795,16 +4817,14 @@ namespace class ILStubCreatorHelper { public: - ILStubCreatorHelper(MethodDesc *pTargetMD, - PInvokeStubParameters* pParams - ) : - m_pTargetMD(pTargetMD), - m_pParams(pParams), - m_pStubMD(NULL), - m_bILStubCreator(false) + ILStubCreatorHelper(PInvokeStubParameters* pParams, MethodDesc *pTargetMD) + : m_pParams(pParams) + , m_pTargetMD(pTargetMD) + , m_pStubMD(NULL) + , m_bILStubCreator(false) { STANDARD_VM_CONTRACT; - m_pHashParams = CreateHashBlob(m_pParams); + m_pHashParams = CreateHashBlob(m_pParams, m_pTargetMD); } ~ILStubCreatorHelper() @@ -4866,11 +4886,10 @@ namespace } private: - MethodDesc* m_pTargetMD; PInvokeStubParameters* m_pParams; - NewArrayHolder m_pHashParams; - AllocMemTracker* m_pAmTracker; + MethodDesc* m_pTargetMD; MethodDesc* m_pStubMD; + NewArrayHolder m_pHashParams; AllocMemTracker m_amTracker; bool m_bILStubCreator; // Only the creator can remove the ILStub from the Cache }; //ILStubCreatorHelper @@ -4966,7 +4985,7 @@ namespace // remove it from cache if OOM occurs { - ILStubCreatorHelper ilStubCreatorHelper(pTargetMD, ¶ms); + ILStubCreatorHelper ilStubCreatorHelper(¶ms, pTargetMD); // take the domain level lock ListLockHolder pILStubLock(AppDomain::GetCurrentDomain()->GetILStubGenLock()); @@ -5686,16 +5705,11 @@ PCODE PInvoke::GetStubForILStub(PInvokeMethodDesc* pNMD, MethodDesc** ppStubMD, } // - // NOTE: there is a race in updating this MethodDesc. We depend on all - // threads getting back the same DynamicMethodDesc for a particular - // PInvokeMethodDesc, in that case, the locking around the actual JIT - // operation will prevent the code from being jitted more than once. - // By the time we get here, all threads get the same address of code - // back from the JIT operation and they all just fill in the same value - // here. - // - // In the NGEN case, all threads will get the same preimplemented code - // address much like the JIT case. + // NOTE: For P/Invoke stubs, racing threads will resolve to the same + // DynamicMethodDesc via the ILStubCache (keyed by target MethodDesc), so the + // locking around the JIT operation prevents the code from being jitted more + // than once, and all threads get back the same PCODE. See CreateHashBlob() + // for the hashing logic. // return pStub; From 4a8c7456423ea751b140a4e70cb6e8f4d64b6d4e Mon Sep 17 00:00:00 2001 From: Aaron R Robinson Date: Wed, 18 Feb 2026 17:10:08 -0800 Subject: [PATCH 2/5] Feedback. --- src/coreclr/vm/dllimport.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/vm/dllimport.cpp b/src/coreclr/vm/dllimport.cpp index 58c264be6f2d14..0d4e08c15534ac 100644 --- a/src/coreclr/vm/dllimport.cpp +++ b/src/coreclr/vm/dllimport.cpp @@ -4097,7 +4097,7 @@ namespace && !SF_IsCALLIStub(dwStubFlags) && !SF_IsVarArgStub(dwStubFlags)) { - size_t blobSize = sizeof(ILStubHashBlobBase) + sizeof(MethodDesc*); + size_t blobSize = sizeof(ILStubHashBlobBase) + sizeof(pTargetMD); NewArrayHolder pBytes = new BYTE[blobSize]; ZeroMemory(pBytes, blobSize); ILStubHashBlob* pBlob = (ILStubHashBlob*)(BYTE*)pBytes; From 95d8981fd5334d14ab705eeda447e13b879b9e08 Mon Sep 17 00:00:00 2001 From: Aaron R Robinson Date: Thu, 19 Feb 2026 13:32:11 -0800 Subject: [PATCH 3/5] Enhance P/Invoke stub handling by ensuring unique hash blob sizes and clarifying comments on thread safety during JIT operations --- src/coreclr/vm/dllimport.cpp | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/src/coreclr/vm/dllimport.cpp b/src/coreclr/vm/dllimport.cpp index 0d4e08c15534ac..68aec52fd682af 100644 --- a/src/coreclr/vm/dllimport.cpp +++ b/src/coreclr/vm/dllimport.cpp @@ -4085,6 +4085,11 @@ namespace { STANDARD_VM_CONTRACT; + // The hash blob for a forward P/Invoke stub is just the target MethodDesc pointer. + // In order to help avoid collisions, ensure various blob sizes are different. + // See asserts below. + constexpr size_t forwardPInvokeHashBlobSize = sizeof(ILStubHashBlobBase) + sizeof(MethodDesc*); + DWORD dwStubFlags = pParams->m_dwStubFlags; // The target MethodDesc may be NULL for field marshalling. @@ -4097,7 +4102,7 @@ namespace && !SF_IsCALLIStub(dwStubFlags) && !SF_IsVarArgStub(dwStubFlags)) { - size_t blobSize = sizeof(ILStubHashBlobBase) + sizeof(pTargetMD); + const size_t blobSize = forwardPInvokeHashBlobSize; NewArrayHolder pBytes = new BYTE[blobSize]; ZeroMemory(pBytes, blobSize); ILStubHashBlob* pBlob = (ILStubHashBlob*)(BYTE*)pBytes; @@ -4161,6 +4166,8 @@ namespace if (cbSizeOfBlob.IsOverflow()) COMPlusThrowHR(COR_E_OVERFLOW); + _ASSERTE(cbSizeOfBlob.Value() != forwardPInvokeHashBlobSize); + static_assert(nltMaxValue <= 0xFF); static_assert(nlfMaxValue <= 0xFF); static_assert(pmMaxValue <= 0xFFFF); @@ -5704,13 +5711,12 @@ PCODE PInvoke::GetStubForILStub(PInvokeMethodDesc* pNMD, MethodDesc** ppStubMD, PInvokeLink(pNMD); } - // - // NOTE: For P/Invoke stubs, racing threads will resolve to the same - // DynamicMethodDesc via the ILStubCache (keyed by target MethodDesc), so the - // locking around the JIT operation prevents the code from being jitted more - // than once, and all threads get back the same PCODE. See CreateHashBlob() - // for the hashing logic. - // + // NOTE: For IL-stub-backed P/Invoke stubs (i.e., when *ppStubMD is non-null + // and the stub is obtained via JitILStub/ILStubCache), racing threads will + // resolve to the same DynamicMethodDesc via the ILStubCache (keyed by the + // target MethodDesc). The locking around the JIT operation prevents the + // code from being jitted more than once, and all threads get back the same + // PCODE. See CreateHashBlob() for the hashing logic. return pStub; } From 12f8f2f79baa0b0f9b24ba8625ef708de6477ace Mon Sep 17 00:00:00 2001 From: Aaron R Robinson Date: Thu, 19 Feb 2026 16:32:54 -0800 Subject: [PATCH 4/5] Update CrstType definitions and adjust CrstILStubGen level for improved thread safety --- src/coreclr/inc/CrstTypes.def | 2 +- src/coreclr/inc/crsttypes_generated.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/coreclr/inc/CrstTypes.def b/src/coreclr/inc/CrstTypes.def index 8cd820022658cd..20851e4738642f 100644 --- a/src/coreclr/inc/CrstTypes.def +++ b/src/coreclr/inc/CrstTypes.def @@ -236,7 +236,7 @@ Crst IJWHash End Crst ILStubGen - AcquiredBefore DeadlockDetection UniqueStack + AcquiredBefore DeadlockDetection UniqueStack UnresolvedClassLock FusionAppCtx End Crst InstMethodHashTable diff --git a/src/coreclr/inc/crsttypes_generated.h b/src/coreclr/inc/crsttypes_generated.h index 713f6bd5b33f33..994ee8b2c86f33 100644 --- a/src/coreclr/inc/crsttypes_generated.h +++ b/src/coreclr/inc/crsttypes_generated.h @@ -170,7 +170,7 @@ int g_rgCrstLevelMap[] = 1, // CrstHandleTable 7, // CrstIJWFixupData 0, // CrstIJWHash - 6, // CrstILStubGen + 11, // CrstILStubGen 0, // CrstInlineTrackingMap 18, // CrstInstMethodHashTable 0, // CrstInterfaceDispatchGlobalLists From fa1c8d797a86b0be8081741e6cc5b414d0be1d6c Mon Sep 17 00:00:00 2001 From: Aaron R Robinson Date: Thu, 19 Feb 2026 19:03:05 -0800 Subject: [PATCH 5/5] Update CrstILStubGen dependencies and adjust level for improved thread safety --- src/coreclr/inc/CrstTypes.def | 2 +- src/coreclr/inc/crsttypes_generated.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/coreclr/inc/CrstTypes.def b/src/coreclr/inc/CrstTypes.def index 20851e4738642f..e3ae5275951f87 100644 --- a/src/coreclr/inc/CrstTypes.def +++ b/src/coreclr/inc/CrstTypes.def @@ -236,7 +236,7 @@ Crst IJWHash End Crst ILStubGen - AcquiredBefore DeadlockDetection UniqueStack UnresolvedClassLock FusionAppCtx + AcquiredBefore DeadlockDetection UniqueStack UnresolvedClassLock FusionAppCtx AssemblyLoader End Crst InstMethodHashTable diff --git a/src/coreclr/inc/crsttypes_generated.h b/src/coreclr/inc/crsttypes_generated.h index 994ee8b2c86f33..28eb23c9e267d4 100644 --- a/src/coreclr/inc/crsttypes_generated.h +++ b/src/coreclr/inc/crsttypes_generated.h @@ -170,7 +170,7 @@ int g_rgCrstLevelMap[] = 1, // CrstHandleTable 7, // CrstIJWFixupData 0, // CrstIJWHash - 11, // CrstILStubGen + 14, // CrstILStubGen 0, // CrstInlineTrackingMap 18, // CrstInstMethodHashTable 0, // CrstInterfaceDispatchGlobalLists