From 08285b16f8dfbd3b0ee4e31cfdfa15aa8aedf31b Mon Sep 17 00:00:00 2001 From: Jan Vorlicek Date: Fri, 24 Apr 2020 17:39:34 +0200 Subject: [PATCH] Fix null reference handling in VSD stub for x86 (#35331) * Fix null reference handling in VSD stub for x86 The regression test that I've added recently to accompany a fix for null reference handling in VSD stub for x64 is failing for x86. The problem is that the null reference handling in VSD dispatch and resolve stubs was broken in another way due to the x86 calling convention. When the call went through a shuffle thunk that removes one stack argument due to the shuffle, the manual unwinding in AdjustContextForVirtualStub was getting an ESP that was off by one stack slot and exception handling wasn't able to correctly unwind from that location to the caller. This change fixes it by letting the AdjustContextForVirtualStub manually unwind to the instruction after the call to the shuffle thunk / VSD stub and updating the ESP according to the number of stack arguments of the target method. I have also modified the regression test. One change was to make a call with multiple parameters to verify that the logic to get stack arguments size is working correctly. Another change was to make sure that both dispatch and resolve stub cases are tested. * Replace MethodTable in the ResolveStub by size of stack args This prevents issues in case the type represented by the MethodTable got unloaded. * Make the stack arguments size stuff Windows specific On Unix x86, the stack is cleaned up by the caller, not the callee. --- src/coreclr/src/vm/i386/excepx86.cpp | 36 +++++++++-- src/coreclr/src/vm/i386/virtualcallstubcpu.hpp | 52 +++++++++------- src/coreclr/src/vm/virtualcallstub.cpp | 27 ++++++++- src/coreclr/src/vm/virtualcallstub.h | 6 +- .../Regressions/coreclr/GitHub_35000/test35000.cs | 70 ++++++++++------------ 5 files changed, 124 insertions(+), 67 deletions(-) diff --git a/src/coreclr/src/vm/i386/excepx86.cpp b/src/coreclr/src/vm/i386/excepx86.cpp index 85f18d9..218b3d4 100644 --- a/src/coreclr/src/vm/i386/excepx86.cpp +++ b/src/coreclr/src/vm/i386/excepx86.cpp @@ -3631,7 +3631,6 @@ LONG CLRNoCatchHandler(EXCEPTION_POINTERS* pExceptionInfo, PVOID pv) return EXCEPTION_CONTINUE_SEARCH; #endif // !FEATURE_EH_FUNCLETS } -#endif // !DACCESS_COMPILE // Returns TRUE if caller should resume execution. BOOL @@ -3653,7 +3652,7 @@ AdjustContextForVirtualStub( PCODE f_IP = GetIP(pContext); VirtualCallStubManager::StubKind sk; - /* VirtualCallStubManager *pMgr = */ VirtualCallStubManager::FindStubManager(f_IP, &sk); + VirtualCallStubManager *pMgr = VirtualCallStubManager::FindStubManager(f_IP, &sk); if (sk == VirtualCallStubManager::SK_DISPATCH) { @@ -3679,11 +3678,12 @@ AdjustContextForVirtualStub( return FALSE; } - PCODE callsite = GetAdjustedCallAddress(*dac_cast(GetSP(pContext))); + PCODE callsite = *dac_cast(GetSP(pContext)); if (pExceptionRecord != NULL) { pExceptionRecord->ExceptionAddress = (PVOID)callsite; } + SetIP(pContext, callsite); #if defined(GCCOVER_TOLERATE_SPURIOUS_AV) @@ -3693,7 +3693,35 @@ AdjustContextForVirtualStub( #endif // defined(GCCOVER_TOLERATE_SPURIOUS_AV) // put ESP back to what it was before the call. - SetSP(pContext, dac_cast(dac_cast(GetSP(pContext)) + sizeof(void*))); + TADDR sp = GetSP(pContext) + sizeof(void*); + +#ifndef UNIX_X86_ABI + // set the ESP to what it would be after the call (remove pushed arguments) + + size_t stackArgumentsSize; + if (sk == VirtualCallStubManager::SK_DISPATCH) + { + ENABLE_FORBID_GC_LOADER_USE_IN_THIS_SCOPE(); + + DispatchHolder *holder = DispatchHolder::FromDispatchEntry(f_IP); + MethodTable *pMT = (MethodTable*)holder->stub()->expectedMT(); + DispatchToken token(VirtualCallStubManager::GetTokenFromStubQuick(pMgr, f_IP, sk)); + MethodDesc* pMD = VirtualCallStubManager::GetRepresentativeMethodDescFromToken(token, pMT); + stackArgumentsSize = pMD->SizeOfArgStack(); + } + else + { + // Compute the stub entry address from the address of failure (location of dereferencing of "this" pointer) + ResolveHolder *holder = ResolveHolder::FromResolveEntry(f_IP - ResolveStub::offsetOfThisDeref()); + stackArgumentsSize = holder->stub()->stackArgumentsSize(); + } + + sp += stackArgumentsSize; +#endif // UNIX_X86_ABI + + SetSP(pContext, dac_cast(dac_cast(sp))); return TRUE; } + +#endif // !DACCESS_COMPILE diff --git a/src/coreclr/src/vm/i386/virtualcallstubcpu.hpp b/src/coreclr/src/vm/i386/virtualcallstubcpu.hpp index 979c961..ab46a53 100644 --- a/src/coreclr/src/vm/i386/virtualcallstubcpu.hpp +++ b/src/coreclr/src/vm/i386/virtualcallstubcpu.hpp @@ -258,7 +258,7 @@ is important. */ struct ResolveStub { inline PCODE failEntryPoint() { LIMITED_METHOD_CONTRACT; return (PCODE)&_failEntryPoint[0]; } - inline PCODE resolveEntryPoint() { LIMITED_METHOD_CONTRACT; return (PCODE)&_resolveEntryPoint[0]; } + inline PCODE resolveEntryPoint() { LIMITED_METHOD_CONTRACT; return (PCODE)&_resolveEntryPoint; } inline PCODE slowEntryPoint() { LIMITED_METHOD_CONTRACT; return (PCODE)&_slowEntryPoint[0]; } inline INT32* pCounter() { LIMITED_METHOD_CONTRACT; return _pCounter; } @@ -266,6 +266,10 @@ struct ResolveStub inline size_t cacheAddress() { LIMITED_METHOD_CONTRACT; return _cacheAddress; } inline size_t token() { LIMITED_METHOD_CONTRACT; return _token; } inline size_t size() { LIMITED_METHOD_CONTRACT; return sizeof(ResolveStub); } +#ifndef UNIX_X86_ABI + inline static size_t offsetOfThisDeref(){ LIMITED_METHOD_CONTRACT; return offsetof(ResolveStub, part1) - offsetof(ResolveStub, _resolveEntryPoint); } + inline size_t stackArgumentsSize() { LIMITED_METHOD_CONTRACT; return _stackArgumentsSize; } +#endif private: friend struct ResolveHolder; @@ -283,13 +287,13 @@ private: // ResolveStub::_resolveEntryPoint expects: // ecx: object (the "this" pointer) // eax: siteAddrForRegisterIndirect if this is a RegisterIndirect dispatch call - BYTE _resolveEntryPoint[6]; // 50 push eax ;save siteAddrForRegisterIndirect - this may be an indirect call - // 8b 01 mov eax,[ecx] ;get the method table from the "this" pointer. This is the place + BYTE _resolveEntryPoint; // 50 push eax ;save siteAddrForRegisterIndirect - this may be an indirect call + BYTE part1 [11]; // 8b 01 mov eax,[ecx] ;get the method table from the "this" pointer. This is the place // ; where we are going to fault on null this. If you change it, // ; change also AdjustContextForVirtualStub in excep.cpp!!! // 52 push edx // 8b d0 mov edx, eax - BYTE part1 [6]; // c1 e8 0C shr eax,12 ;we are adding upper bits into lower bits of mt + // c1 e8 0C shr eax,12 ;we are adding upper bits into lower bits of mt // 03 c2 add eax,edx // 35 xor eax, UINT32 _hashedToken; // xx xx xx xx hashedToken ;along with pre-hashed token @@ -329,6 +333,9 @@ private: DISPL _backpatcherDispl; // xx xx xx xx backpatcherWorker == BackPatchWorkerAsmStub BYTE part11 [1]; // eb jmp BYTE toResolveStub; // xx resolveStub, i.e. go back to _resolveEntryPoint +#ifndef UNIX_X86_ABI + size_t _stackArgumentsSize; // xx xx xx xx +#endif }; /* ResolveHolders are the containers for ResolveStubs, They provide @@ -343,7 +350,8 @@ struct ResolveHolder void Initialize(PCODE resolveWorkerTarget, PCODE patcherTarget, size_t dispatchToken, UINT32 hashedToken, - void * cacheAddr, INT32 * counterAddr); + void * cacheAddr, INT32 * counterAddr, + size_t stackArgumentsSize); ResolveStub* stub() { LIMITED_METHOD_CONTRACT; return &_stub; } @@ -839,21 +847,19 @@ void ResolveHolder::InitializeStatic() resolveInit.toPatcher = (offsetof(ResolveStub, patch) - (offsetof(ResolveStub, toPatcher) + 1)) & 0xFF; - resolveInit._resolveEntryPoint [0] = 0x50; - resolveInit._resolveEntryPoint [1] = 0x8b; - resolveInit._resolveEntryPoint [2] = 0x01; - resolveInit._resolveEntryPoint [3] = 0x52; - resolveInit._resolveEntryPoint [4] = 0x8b; - resolveInit._resolveEntryPoint [5] = 0xd0; - static_assert_no_msg(sizeof(resolveInit._resolveEntryPoint) == 6); - - resolveInit.part1 [0] = 0xc1; - resolveInit.part1 [1] = 0xe8; - resolveInit.part1 [2] = CALL_STUB_CACHE_NUM_BITS; - resolveInit.part1 [3] = 0x03; - resolveInit.part1 [4] = 0xc2; - resolveInit.part1 [5] = 0x35; - static_assert_no_msg(sizeof(resolveInit.part1) == 6); + resolveInit._resolveEntryPoint = 0x50; + resolveInit.part1 [0] = 0x8b; + resolveInit.part1 [1] = 0x01; + resolveInit.part1 [2] = 0x52; + resolveInit.part1 [3] = 0x8b; + resolveInit.part1 [4] = 0xd0; + resolveInit.part1 [5] = 0xc1; + resolveInit.part1 [6] = 0xe8; + resolveInit.part1 [7] = CALL_STUB_CACHE_NUM_BITS; + resolveInit.part1 [8] = 0x03; + resolveInit.part1 [9] = 0xc2; + resolveInit.part1 [10] = 0x35; + static_assert_no_msg(sizeof(resolveInit.part1) == 11); resolveInit._hashedToken = 0xcccccccc; resolveInit.part2 [0] = 0x25; @@ -932,7 +938,8 @@ void ResolveHolder::InitializeStatic() void ResolveHolder::Initialize(PCODE resolveWorkerTarget, PCODE patcherTarget, size_t dispatchToken, UINT32 hashedToken, - void * cacheAddr, INT32 * counterAddr) + void * cacheAddr, INT32 * counterAddr, + size_t stackArgumentsSize) { _stub = resolveInit; @@ -945,6 +952,9 @@ void ResolveHolder::Initialize(PCODE resolveWorkerTarget, PCODE patcherTarget, _stub._tokenPush = dispatchToken; _stub._resolveWorkerDispl = resolveWorkerTarget - ((PCODE) &_stub._resolveWorkerDispl + sizeof(DISPL)); _stub._backpatcherDispl = patcherTarget - ((PCODE) &_stub._backpatcherDispl + sizeof(DISPL)); +#ifndef UNIX_X86_ABI + _stub._stackArgumentsSize = stackArgumentsSize; +#endif } ResolveHolder* ResolveHolder::FromFailEntry(PCODE failEntry) diff --git a/src/coreclr/src/vm/virtualcallstub.cpp b/src/coreclr/src/vm/virtualcallstub.cpp index 580e17d..a124ed7 100644 --- a/src/coreclr/src/vm/virtualcallstub.cpp +++ b/src/coreclr/src/vm/virtualcallstub.cpp @@ -1912,9 +1912,22 @@ PCODE VirtualCallStubManager::ResolveWorker(StubCallSite* pCallSite, PCODE addrOfResolver = (PCODE)(resolvers->Find(&probeR)); if (addrOfResolver == CALL_STUB_EMPTY_ENTRY) { +#if defined(TARGET_X86) && !defined(UNIX_X86_ABI) + MethodDesc* pMD = VirtualCallStubManager::GetRepresentativeMethodDescFromToken(token, objectType); + size_t stackArgumentsSize; + { + ENABLE_FORBID_GC_LOADER_USE_IN_THIS_SCOPE(); + stackArgumentsSize = pMD->SizeOfArgStack(); + } +#endif // TARGET_X86 && !UNIX_X86_ABI + pResolveHolder = GenerateResolveStub(pResolverFcn, pBackPatchFcn, - token.To_SIZE_T()); + token.To_SIZE_T() +#if defined(TARGET_X86) && !defined(UNIX_X86_ABI) + , stackArgumentsSize +#endif + ); // Add the resolve entrypoint into the cache. //@TODO: Can we store a pointer to the holder rather than the entrypoint? @@ -2848,7 +2861,11 @@ addrOfPatcher is who to call if the fail piece is being called too often by disp */ ResolveHolder *VirtualCallStubManager::GenerateResolveStub(PCODE addrOfResolver, PCODE addrOfPatcher, - size_t dispatchToken) + size_t dispatchToken +#if defined(TARGET_X86) && !defined(UNIX_X86_ABI) + , size_t stackArgumentsSize +#endif + ) { CONTRACT (ResolveHolder*) { THROWS; @@ -2912,7 +2929,11 @@ ResolveHolder *VirtualCallStubManager::GenerateResolveStub(PCODE addr holder->Initialize(addrOfResolver, addrOfPatcher, dispatchToken, DispatchCache::HashToken(dispatchToken), - g_resolveCache->GetCacheBaseAddr(), counterAddr); + g_resolveCache->GetCacheBaseAddr(), counterAddr +#if defined(TARGET_X86) && !defined(UNIX_X86_ABI) + , stackArgumentsSize +#endif + ); ClrFlushInstructionCache(holder->stub(), holder->stub()->size()); AddToCollectibleVSDRangeList(holder); diff --git a/src/coreclr/src/vm/virtualcallstub.h b/src/coreclr/src/vm/virtualcallstub.h index cca8df3..9f2e280 100644 --- a/src/coreclr/src/vm/virtualcallstub.h +++ b/src/coreclr/src/vm/virtualcallstub.h @@ -510,7 +510,11 @@ private: ResolveHolder *GenerateResolveStub(PCODE addrOfResolver, PCODE addrOfPatcher, - size_t dispatchToken); + size_t dispatchToken +#if defined(TARGET_X86) && !defined(UNIX_X86_ABI) + , size_t stackArgumentsSize +#endif + ); LookupHolder *GenerateLookupStub(PCODE addrOfResolver, size_t dispatchToken); diff --git a/src/coreclr/tests/src/Regressions/coreclr/GitHub_35000/test35000.cs b/src/coreclr/tests/src/Regressions/coreclr/GitHub_35000/test35000.cs index 53027c1..30e0324 100644 --- a/src/coreclr/tests/src/Regressions/coreclr/GitHub_35000/test35000.cs +++ b/src/coreclr/tests/src/Regressions/coreclr/GitHub_35000/test35000.cs @@ -4,55 +4,49 @@ using System; using System.Reflection; -public class TestData -{ - public virtual Guid InputGuid { get; set; } -} - class Test35000 { - static int Main(string[] args) + public class TestData0 { - bool success = false; + public virtual object MyMethod(int a, int b, int c, int d, int e, int f, int g, int h) { return null; } + } - PropertyInfo property = typeof(TestData).GetProperty(nameof(TestData.InputGuid), - BindingFlags.Instance | BindingFlags.Public); + public class TestData1 : TestData0 + { + public override object MyMethod(int a, int b, int c, int d, int e, int f, int g, int h) { return null; } + } - Func func = GetFunc(property); + static int Main(string[] args) + { + var method = typeof(TestData0).GetMethod(nameof(TestData0.MyMethod)); + var func = (Func)Delegate.CreateDelegate(typeof(Func), null, method); - TestData data = new TestData(); + TestData0 data = new TestData0(); + TestData0 data1 = new TestData1(); - data.InputGuid = Guid.NewGuid(); + int nullRefCount = 0; - object value1 = func(data); - object value3 = func(data); + const int LoopCount = 10; - try + for (int j = 0; j < LoopCount; j++) { - object value2 = func(null); + for (int i = 0; i < 50; i++) + { + func(data, 1, 2, 3, 4, 5, 6, 7, 8); + func(data1, 1, 2, 3, 4, 5, 6, 7, 8); + } + + try + { + func(null, 1, 2, 3, 4, 5, 6, 7, 8); + } + catch (NullReferenceException e) + { + nullRefCount++; + Console.WriteLine(e); + } } - catch (NullReferenceException e) - { - Console.WriteLine(e); - success = true; - } - - return success ? 100 : 101; - } - - public static Func GetFunc(PropertyInfo propertyInfo) - { - var method = typeof(Test35000).GetMethod(nameof(CreateFunc)); - - return (Func)method - .MakeGenericMethod(propertyInfo.DeclaringType, propertyInfo.PropertyType) - .Invoke(null, new object[] { propertyInfo.GetMethod }); - } - - public static Func CreateFunc(MethodInfo methodInfo) - { - var func = (Func)Delegate.CreateDelegate(typeof(Func), null, methodInfo); - return (object o) => func((TTarget)o); + return (nullRefCount == LoopCount) ? 100 : 101; } } -- 2.7.4