From 0f0320e58fd006a02cdecf7ae45426f54da333e5 Mon Sep 17 00:00:00 2001 From: Mike McLaughlin Date: Sat, 7 Apr 2018 14:21:46 -0700 Subject: Fix OpenVirtualProcess SIGSEGV on Linux. (#17444) Add DBI OpenVirtualProcessImpl2 that takes a module path instead of handle. Fix assert on Windows debug. Issue #17446 --- src/debug/di/process.cpp | 33 +++++++ src/debug/shim/debugshim.cpp | 143 ++++++++++++++++++++--------- src/dlls/dbgshim/dbgshim.cpp | 9 +- src/dlls/mscordbi/mscordbi.src | 1 + src/dlls/mscordbi/mscordbi_unixexports.src | 3 +- 5 files changed, 143 insertions(+), 46 deletions(-) (limited to 'src') diff --git a/src/debug/di/process.cpp b/src/debug/di/process.cpp index b869a0184f..9a123c4d5e 100644 --- a/src/debug/di/process.cpp +++ b/src/debug/di/process.cpp @@ -155,6 +155,39 @@ STDAPI OpenVirtualProcessImpl( return hr; }; +//--------------------------------------------------------------------------------------- +// +// OpenVirtualProcessImpl2 method called by the dbgshim to get an ICorDebugProcess4 instance +// +// Arguments: +// clrInstanceId - target pointer identifying which CLR in the Target to debug. +// pDataTarget - data target abstraction. +// pDacModulePath - the module path of the appropriate DAC dll for this runtime +// riid - interface ID to query for. +// ppProcessOut - new object for target, interface ID matches riid. +// ppFlagsOut - currently only has 1 bit to indicate whether or not this runtime +// instance will send a managed event after attach +// +// Return Value: +// S_OK on success. Else failure +//--------------------------------------------------------------------------------------- +STDAPI OpenVirtualProcessImpl2( + ULONG64 clrInstanceId, + IUnknown * pDataTarget, + LPCWSTR pDacModulePath, + CLR_DEBUGGING_VERSION * pMaxDebuggerSupportedVersion, + REFIID riid, + IUnknown ** ppInstance, + CLR_DEBUGGING_PROCESS_FLAGS* pFlagsOut) +{ + HMODULE hDac = LoadLibraryW(pDacModulePath); + if (hDac == NULL) + { + return HRESULT_FROM_WIN32(GetLastError()); + } + return OpenVirtualProcessImpl(clrInstanceId, pDataTarget, hDac, pMaxDebuggerSupportedVersion, riid, ppInstance, pFlagsOut); +} + //--------------------------------------------------------------------------------------- // DEPRECATED - use OpenVirtualProcessImpl // OpenVirtualProcess method used by the shim in CLR v4 Beta1 diff --git a/src/debug/shim/debugshim.cpp b/src/debug/shim/debugshim.cpp index b4e709adcf..828cbfeefa 100644 --- a/src/debug/shim/debugshim.cpp +++ b/src/debug/shim/debugshim.cpp @@ -38,6 +38,14 @@ // CLRDebuggingImpl implementation (ICLRDebugging) //***************************************************************************** +typedef HRESULT (STDAPICALLTYPE *OpenVirtualProcessImpl2FnPtr)(ULONG64 clrInstanceId, + IUnknown * pDataTarget, + LPCWSTR pDacModulePath, + CLR_DEBUGGING_VERSION * pMaxDebuggerSupportedVersion, + REFIID riid, + IUnknown ** ppInstance, + CLR_DEBUGGING_PROCESS_FLAGS * pdwFlags); + typedef HRESULT (STDAPICALLTYPE *OpenVirtualProcessImplFnPtr)(ULONG64 clrInstanceId, IUnknown * pDataTarget, HMODULE hDacDll, @@ -53,6 +61,8 @@ typedef HRESULT (STDAPICALLTYPE *OpenVirtualProcess2FnPtr)(ULONG64 clrInstanceI IUnknown ** ppInstance, CLR_DEBUGGING_PROCESS_FLAGS * pdwFlags); +typedef HMODULE (STDAPICALLTYPE *LoadLibraryWFnPtr)(LPCWSTR lpLibFileName); + // Implementation of ICLRDebugging::OpenVirtualProcess // // Arguments: @@ -81,63 +91,64 @@ STDMETHODIMP CLRDebuggingImpl::OpenVirtualProcess( ICorDebugDataTarget * pDt = NULL; HMODULE hDbi = NULL; HMODULE hDac = NULL; + LPWSTR pDacModulePath = NULL; + LPWSTR pDbiModulePath = NULL; DWORD dbiTimestamp; DWORD dbiSizeOfImage; - WCHAR dbiName[MAX_PATH_FNAME] = {0}; + WCHAR dbiName[MAX_PATH_FNAME] = { 0 }; DWORD dacTimestamp; DWORD dacSizeOfImage; - WCHAR dacName[MAX_PATH_FNAME] = {0}; + WCHAR dacName[MAX_PATH_FNAME] = { 0 }; CLR_DEBUGGING_VERSION version; BOOL versionSupportedByCaller = FALSE; - + // argument checking - if( (ppProcess != NULL || pFlags != NULL) && pLibraryProvider == NULL) + if ((ppProcess != NULL || pFlags != NULL) && pLibraryProvider == NULL) { hr = E_POINTER; // the library provider must be specified if either // ppProcess or pFlags is non-NULL } - else if( (ppProcess != NULL || pFlags != NULL) && pMaxDebuggerSupportedVersion == NULL) + else if ((ppProcess != NULL || pFlags != NULL) && pMaxDebuggerSupportedVersion == NULL) { hr = E_POINTER; // the max supported version must be specified if either // ppProcess or pFlags is non-NULL } - else if(pVersion != NULL && pVersion->wStructVersion != 0) + else if (pVersion != NULL && pVersion->wStructVersion != 0) { hr = CORDBG_E_UNSUPPORTED_VERSION_STRUCT; } - else if(FAILED(pDataTarget->QueryInterface(__uuidof(ICorDebugDataTarget), (void**) &pDt))) + else if (FAILED(pDataTarget->QueryInterface(__uuidof(ICorDebugDataTarget), (void**)&pDt))) { hr = CORDBG_E_MISSING_DATA_TARGET_INTERFACE; } - if(SUCCEEDED(hr)) + if (SUCCEEDED(hr)) { // get CLR version // The expectation is that new versions of the CLR will continue to use the same GUID // (unless there's a reason to hide them from older shims), but debuggers will tell us the // CLR version they're designed for and mscordbi.dll can decide whether or not to accept it. version.wStructVersion = 0; - hr = GetCLRInfo(pDt, - moduleBaseAddress, - &version, - &dbiTimestamp, - &dbiSizeOfImage, - dbiName, - MAX_PATH_FNAME, - &dacTimestamp, - &dacSizeOfImage, - dacName, - MAX_PATH_FNAME); + hr = GetCLRInfo(pDt, + moduleBaseAddress, + &version, + &dbiTimestamp, + &dbiSizeOfImage, + dbiName, + MAX_PATH_FNAME, + &dacTimestamp, + &dacSizeOfImage, + dacName, + MAX_PATH_FNAME); } // If we need to fetch either the process info or the flags info then we need to find // mscordbi and DAC and do the version specific OVP work - if(SUCCEEDED(hr) && (ppProcess != NULL || pFlags != NULL)) + if (SUCCEEDED(hr) && (ppProcess != NULL || pFlags != NULL)) { ICLRDebuggingLibraryProvider2* pLibraryProvider2; if (SUCCEEDED(pLibraryProvider->QueryInterface(__uuidof(ICLRDebuggingLibraryProvider2), (void**)&pLibraryProvider2))) { - LPWSTR pDbiModulePath; if (FAILED(pLibraryProvider2->ProvideLibrary2(dbiName, dbiTimestamp, dbiSizeOfImage, &pDbiModulePath)) || pDbiModulePath == NULL) { @@ -151,11 +162,6 @@ STDMETHODIMP CLRDebuggingImpl::OpenVirtualProcess( { hr = HRESULT_FROM_WIN32(GetLastError()); } -#ifdef FEATURE_PAL - free(pDbiModulePath); -#else - CoTaskMemFree(pDbiModulePath); -#endif } if (SUCCEEDED(hr)) @@ -163,8 +169,7 @@ STDMETHODIMP CLRDebuggingImpl::OpenVirtualProcess( // Adjust the timestamp and size of image if this DAC is a known buggy version and needs to be retargeted RetargetDacIfNeeded(&dacTimestamp, &dacSizeOfImage); - // ask library provider for dac - LPWSTR pDacModulePath; + // Ask library provider for dac if (FAILED(pLibraryProvider2->ProvideLibrary2(dacName, dacTimestamp, dacSizeOfImage, &pDacModulePath)) || pDacModulePath == NULL) { @@ -178,18 +183,13 @@ STDMETHODIMP CLRDebuggingImpl::OpenVirtualProcess( { hr = HRESULT_FROM_WIN32(GetLastError()); } -#ifdef FEATURE_PAL - free(pDacModulePath); -#else - CoTaskMemFree(pDacModulePath); -#endif } } pLibraryProvider2->Release(); } else { - // ask library provider for dbi + // Ask library provider for dbi if (FAILED(pLibraryProvider->ProvideLibrary(dbiName, dbiTimestamp, dbiSizeOfImage, &hDbi)) || hDbi == NULL) { @@ -210,14 +210,53 @@ STDMETHODIMP CLRDebuggingImpl::OpenVirtualProcess( } } - if(SUCCEEDED(hr)) + *ppProcess = NULL; + + if (SUCCEEDED(hr) && pDacModulePath != NULL) + { + // Get access to the latest OVP implementation and call it + OpenVirtualProcessImpl2FnPtr ovpFn = (OpenVirtualProcessImpl2FnPtr)GetProcAddress(hDbi, "OpenVirtualProcessImpl2"); + if (ovpFn != NULL) + { + hr = ovpFn(moduleBaseAddress, pDataTarget, pDacModulePath, pMaxDebuggerSupportedVersion, riidProcess, ppProcess, pFlags); + if (FAILED(hr)) + { + _ASSERTE(ppProcess == NULL || *ppProcess == NULL); + _ASSERTE(pFlags == NULL || *pFlags == 0); + } + } +#ifdef FEATURE_PAL + else + { + // On Linux/MacOS the DAC module handle needs to be re-created using the DAC PAL instance + // before being passed to DBI's OpenVirtualProcess* implementation. The DBI and DAC share + // the same PAL where dbgshim has it's own. + LoadLibraryWFnPtr loadLibraryWFn = (LoadLibraryWFnPtr)GetProcAddress(hDac, "LoadLibraryW"); + if (loadLibraryWFn != NULL) + { + hDac = loadLibraryWFn(pDacModulePath); + if (hDac == NULL) + { + hr = E_HANDLE; + } + } + else + { + hr = E_HANDLE; + } + } +#endif // FEATURE_PAL + } + + // If no errors so far and "OpenVirtualProcessImpl2" doesn't exist + if (SUCCEEDED(hr) && *ppProcess == NULL) { - // get access to OVP and call it - OpenVirtualProcessImplFnPtr ovpFn = (OpenVirtualProcessImplFnPtr) GetProcAddress(hDbi, "OpenVirtualProcessImpl"); - if(ovpFn == NULL) + // Get access to OVP and call it + OpenVirtualProcessImplFnPtr ovpFn = (OpenVirtualProcessImplFnPtr)GetProcAddress(hDbi, "OpenVirtualProcessImpl"); + if (ovpFn == NULL) { // Fallback to CLR v4 Beta1 path, but skip some of the checking we'd normally do (maxSupportedVersion, etc.) - OpenVirtualProcess2FnPtr ovp2Fn = (OpenVirtualProcess2FnPtr) GetProcAddress(hDbi, "OpenVirtualProcess2"); + OpenVirtualProcess2FnPtr ovp2Fn = (OpenVirtualProcess2FnPtr)GetProcAddress(hDbi, "OpenVirtualProcess2"); if (ovp2Fn == NULL) { hr = CORDBG_E_LIBRARY_PROVIDER_ERROR; @@ -231,7 +270,7 @@ STDMETHODIMP CLRDebuggingImpl::OpenVirtualProcess( { // Have a CLR v4 Beta2+ DBI, call it and let it do the version check hr = ovpFn(moduleBaseAddress, pDataTarget, hDac, pMaxDebuggerSupportedVersion, riidProcess, ppProcess, pFlags); - if(FAILED(hr)) + if (FAILED(hr)) { _ASSERTE(ppProcess == NULL || *ppProcess == NULL); _ASSERTE(pFlags == NULL || *pFlags == 0); @@ -241,16 +280,34 @@ STDMETHODIMP CLRDebuggingImpl::OpenVirtualProcess( } //version is still valid in some failure cases - if(pVersion != NULL && + if (pVersion != NULL && (SUCCEEDED(hr) || (hr == CORDBG_E_UNSUPPORTED_DEBUGGING_MODEL) || - (hr == CORDBG_E_UNSUPPORTED_FORWARD_COMPAT))) + (hr == CORDBG_E_UNSUPPORTED_FORWARD_COMPAT))) { memcpy(pVersion, &version, sizeof(CLR_DEBUGGING_VERSION)); } + if (pDacModulePath != NULL) + { +#ifdef FEATURE_PAL + free(pDacModulePath); +#else + CoTaskMemFree(pDacModulePath); +#endif + } + + if (pDbiModulePath != NULL) + { +#ifdef FEATURE_PAL + free(pDbiModulePath); +#else + CoTaskMemFree(pDbiModulePath); +#endif + } + // free the data target we QI'ed earlier - if(pDt != NULL) + if (pDt != NULL) { pDt->Release(); } diff --git a/src/dlls/dbgshim/dbgshim.cpp b/src/dlls/dbgshim/dbgshim.cpp index 39c966a3ce..f096b20431 100644 --- a/src/dlls/dbgshim/dbgshim.cpp +++ b/src/dlls/dbgshim/dbgshim.cpp @@ -1808,6 +1808,8 @@ CLRCreateInstance( REFIID riid, LPVOID *ppInterface) { + PUBLIC_CONTRACT; + #if defined(FEATURE_CORESYSTEM) if (ppInterface == NULL) @@ -1822,9 +1824,12 @@ CLRCreateInstance( GUID skuId = CLR_ID_CORECLR; #endif - CLRDebuggingImpl *pDebuggingImpl = new CLRDebuggingImpl(skuId); + CLRDebuggingImpl *pDebuggingImpl = new (nothrow) CLRDebuggingImpl(skuId); + if (NULL == pDebuggingImpl) + return E_OUTOFMEMORY; + return pDebuggingImpl->QueryInterface(riid, ppInterface); #else return E_NOTIMPL; #endif -} \ No newline at end of file +} diff --git a/src/dlls/mscordbi/mscordbi.src b/src/dlls/mscordbi/mscordbi.src index 3b1f37720d..0e14701243 100644 --- a/src/dlls/mscordbi/mscordbi.src +++ b/src/dlls/mscordbi/mscordbi.src @@ -13,6 +13,7 @@ EXPORTS // Out-of-proc creation path from the shim - ICLRDebugging OpenVirtualProcessImpl + OpenVirtualProcessImpl2 // DEPRECATED - use OpenVirtualProcessImpl OpenVirtualProcess private diff --git a/src/dlls/mscordbi/mscordbi_unixexports.src b/src/dlls/mscordbi/mscordbi_unixexports.src index b4704aef63..88fa9cdab1 100644 --- a/src/dlls/mscordbi/mscordbi_unixexports.src +++ b/src/dlls/mscordbi/mscordbi_unixexports.src @@ -11,8 +11,9 @@ CoreCLRCreateCordbObject ; Out-of-proc creation path from the shim - ICLRDebugging OpenVirtualProcessImpl +OpenVirtualProcessImpl2 ; PAL module registration DllMain PAL_RegisterModule -PAL_UnregisterModule \ No newline at end of file +PAL_UnregisterModule -- cgit v1.2.3