Fix OpenVirtualProcess SIGSEGV on Linux. (#17444)

Add DBI OpenVirtualProcessImpl2 that takes a module path instead of handle.

Fix assert on Windows debug.

Issue #17446

5 files changed, 143 insertions, 46 deletions

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
@@ -156,6 +156,39 @@ STDAPI OpenVirtualProcessImpl(
 };
 
 //---------------------------------------------------------------------------------------
+//
+// 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
 // We'd like a beta1 shim/VS to still be able to open dumps using a CLR v4 Beta2+ mscordbi.dll, 
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