Cleanup where.exe invocation (#958)

src/AspNetCoreModuleV2/CommonLib/CommonLib.vcxproj

@@ -197,6 +197,7 @@
     <ClInclude Include="exceptions.h" />
     <ClInclude Include="GlobalVersionUtility.h" />
     <ClInclude Include="fx_ver.h" />
+    <ClInclude Include="HandleWrapper.h" />
     <ClInclude Include="hostfxroptions.h" />
     <ClInclude Include="hostfxr_utility.h" />
     <ClInclude Include="iapplication.h" />

src/AspNetCoreModuleV2/CommonLib/HandleWrapper.h

@@ -0,0 +1,52 @@
+// Copyright (c) .NET Foundation. All rights reserved.
+// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
+
+#pragma once
+
+#include <Windows.h>
+
+struct InvalidHandleTraits
+{
+    using HandleType = HANDLE;
+    static const HANDLE DefaultHandle;
+    static void Close(HANDLE handle) { CloseHandle(handle); }
+};
+
+// Workaround for VC++ bug https://developercommunity.visualstudio.com/content/problem/33928/constexpr-failing-on-nullptr-v141-compiler-regress.html
+const HANDLE InvalidHandleTraits::DefaultHandle = INVALID_HANDLE_VALUE;
+
+struct NullHandleTraits
+{
+    using HandleType = HANDLE;
+    static constexpr HANDLE DefaultHandle = NULL;
+    static void Close(HANDLE handle) { CloseHandle(handle); }
+};
+
+template<typename traits>
+class HandleWrapper
+{
+public:
+    using HandleType = typename traits::HandleType;
+
+    HandleWrapper(HandleType handle = traits::DefaultHandle) : m_handle(handle) { }
+    ~HandleWrapper()
+    {
+        if (m_handle != traits::DefaultHandle)
+        {
+            traits::Close(m_handle);
+        }
+    }
+
+    operator HandleType() { return m_handle; }
+    HandleWrapper& operator =(HandleType value)
+    {
+        DBG_ASSERT(m_handle == traits::DefaultHandle);
+        m_handle = value;
+        return *this;
+    }
+
+    HandleType* operator&() { return &m_handle; }
+
+private:
+    HandleType m_handle;
+};

src/AspNetCoreModuleV2/CommonLib/exceptions.h

@@ -26,12 +26,21 @@
 #endif
 
 #define OBSERVE_CAUGHT_EXCEPTION()                              CaughtExceptionHResult(LOCATION_INFO);
-#define RETURN_CAUGHT_EXCEPTION()                               return CaughtExceptionHResult(LOCATION_INFO);
-#define CATCH_RETURN()                                          catch (...) { RETURN_CAUGHT_EXCEPTION(); }
-#define THROW_IF_NULL_ALLOC(ptr)                                Throw_IfNullAlloc(ptr)
 #define RETURN_IF_FAILED(hr)                                    do { HRESULT __hrRet = hr; if (FAILED(__hrRet)) { LogHResultFailed(LOCATION_INFO, __hrRet); return __hrRet; }} while (0, 0)
+#define RETURN_CAUGHT_EXCEPTION()                               return CaughtExceptionHResult(LOCATION_INFO);
+#define RETURN_LAST_ERROR_IF(condition)                         do { if (condition) { return LogLastError(LOCATION_INFO); }} while (0, 0)
+#define RETURN_LAST_ERROR_IF_NULL(ptr)                          do { if ((ptr) == nullptr) { return LogLastError(LOCATION_INFO); }} while (0, 0)
+
 #define FINISHED_IF_FAILED(hrr)                                 do { HRESULT __hrRet = hrr; if (FAILED(__hrRet)) { LogHResultFailed(LOCATION_INFO, __hrRet); hr = __hrRet; goto Finished; }} while (0, 0)
+#define FINISHED_IF_NULL_ALLOC(ptr)                             do { if ((ptr) == nullptr) { hr = LogHResultFailed(LOCATION_INFO, E_OUTOFMEMORY); goto Finished; }} while (0, 0)
+#define FINISHED_LAST_ERROR_IF(condition)                       do { if (condition) { hr = LogLastError(LOCATION_INFO); goto Finished; }} while (0, 0)
+#define FINISHED_LAST_ERROR_IF_NULL(ptr)                        do { if ((ptr) == nullptr) { hr = LogLastError(LOCATION_INFO); goto Finished; }} while (0, 0)
+
+#define THROW_IF_NULL_ALLOC(ptr)                                Throw_IfNullAlloc(ptr)
+
+#define CATCH_RETURN()                                          catch (...) { RETURN_CAUGHT_EXCEPTION(); }
 #define LOG_IF_FAILED(hr)                                       LogHResultFailed(LOCATION_INFO, hr)
+#define LOG_LAST_ERROR_IF(condition)                            LogLastErrorIf(LOCATION_INFO, condition)
 #define SUCCEEDED_LOG(hr)                                       SUCCEEDED(LOG_IF_FAILED(hr))
 #define FAILED_LOG(hr)                                          FAILED(LOG_IF_FAILED(hr))
 
@@ -40,6 +49,26 @@
     DebugPrintf(ASPNETCORE_DEBUG_FLAG_ERROR, LOCATION_FORMAT "Unhandled non-standard exception", LOCATION_CALL_ONLY);
 }
 
+ __declspec(noinline) inline HRESULT LogLastError(LOCATION_ARGUMENTS_ONLY)
+{
+    const auto lastError = GetLastError();
+    const auto hr = HRESULT_FROM_WIN32(lastError);
+
+    DebugPrintf(ASPNETCORE_DEBUG_FLAG_ERROR, LOCATION_FORMAT "Operation failed with LastError: %d HR: 0x%x", LOCATION_CALL lastError, hr);
+
+    return hr;
+}
+
+ __declspec(noinline) inline bool LogLastErrorIf(LOCATION_ARGUMENTS_ONLY, bool condition)
+{
+    if (condition)
+    {
+        LogLastError(LOCATION_CALL_ONLY);
+    }
+
+    return condition;
+}
+
  __declspec(noinline) inline VOID ReportException(LOCATION_ARGUMENTS std::exception& exception)
 {
     DebugPrintf(ASPNETCORE_DEBUG_FLAG_ERROR, LOCATION_FORMAT "Unhandled exception: %s", LOCATION_CALL exception.what());

src/AspNetCoreModuleV2/CommonLib/hostfxr_utility.cpp

@@ -4,11 +4,13 @@
 #include "hostfxr_utility.h"
 
 #include <string>
+#include <atlcomcli.h>
 #include "EventLog.h"
 #include "ntassert.h"
 #include "fx_ver.h"
 #include "debugutil.h"
 #include "exceptions.h"
+#include "HandleWrapper.h"
 
 namespace fs = std::experimental::filesystem;
 
@@ -440,9 +442,11 @@ HOSTFXR_UTILITY::InvokeWhereToFindDotnet()
     SECURITY_ATTRIBUTES securityAttributes;
 
     CHAR                pzFileContents[READ_BUFFER_SIZE];
-    HANDLE              hStdOutReadPipe = INVALID_HANDLE_VALUE;
+    HandleWrapper<InvalidHandleTraits>     hStdOutReadPipe;
-    HANDLE              hStdOutWritePipe = INVALID_HANDLE_VALUE;
+    HandleWrapper<InvalidHandleTraits>     hStdOutWritePipe;
-    LPWSTR              pwzDotnetName = NULL;
+    HandleWrapper<InvalidHandleTraits>     hProcess;
+    HandleWrapper<InvalidHandleTraits>     hThread;
+    CComBSTR            pwzDotnetName = NULL;
     DWORD               dwFilePointer;
     BOOL                fIsWow64Process;
     BOOL                fIsCurrentProcess64Bit;
@@ -453,8 +457,6 @@ HOSTFXR_UTILITY::InvokeWhereToFindDotnet()
     DWORD               dwBinaryType;
     INT                 index = 0;
     INT                 prevIndex = 0;
-    BOOL                fProcessCreationResult = FALSE;
-    BOOL                fResult = FALSE;
     std::optional<fs::path> result;
 
     // Set the security attributes for the read/write pipe
@@ -462,17 +464,11 @@ HOSTFXR_UTILITY::InvokeWhereToFindDotnet()
     securityAttributes.lpSecurityDescriptor = NULL;
     securityAttributes.bInheritHandle = TRUE;
 
+    LOG_INFO("Invoking where.exe to find dotnet.exe");
+
     // Create a read/write pipe that will be used for reading the result of where.exe
-    if (!CreatePipe(&hStdOutReadPipe, &hStdOutWritePipe, &securityAttributes, 0))
+    FINISHED_LAST_ERROR_IF(!CreatePipe(&hStdOutReadPipe, &hStdOutWritePipe, &securityAttributes, 0));
-    {
+    FINISHED_LAST_ERROR_IF(!SetHandleInformation(hStdOutReadPipe, HANDLE_FLAG_INHERIT, 0));
-        hr = HRESULT_FROM_WIN32(GetLastError());
-        goto Finished;
-    }
-    if (!SetHandleInformation(hStdOutReadPipe, HANDLE_FLAG_INHERIT, 0))
-    {
-        hr = ERROR_FILE_INVALID;
-        goto Finished;
-    }
 
     // Set the stdout and err pipe to the write pipes.
     startupInfo.cb = sizeof(startupInfo);
@@ -482,14 +478,10 @@ HOSTFXR_UTILITY::InvokeWhereToFindDotnet()
 
     // CreateProcess requires a mutable string to be passed to commandline
     // See https://blogs.msdn.microsoft.com/oldnewthing/20090601-00/?p=18083/
-    pwzDotnetName = SysAllocString(L"\"where.exe\" dotnet.exe");
+    pwzDotnetName = L"\"where.exe\" dotnet.exe";
-    if (pwzDotnetName == NULL)
-    {
-        goto Finished;
-    }
 
     // Create a process to invoke where.exe
-    fProcessCreationResult = CreateProcessW(NULL,
+    FINISHED_LAST_ERROR_IF(!CreateProcessW(NULL,
         pwzDotnetName,
         NULL,
         NULL,
@@ -499,20 +491,18 @@ HOSTFXR_UTILITY::InvokeWhereToFindDotnet()
         NULL,
         &startupInfo,
         &processInformation
-    );
+    ));
 
-    if (!fProcessCreationResult)
+    // Store handles into wrapper so they get closed automatically
-    {
+    hProcess = processInformation.hProcess;
-        goto Finished;
+    hThread = processInformation.hThread;
-    }
 
     // Wait for where.exe to return, waiting 2 seconds.
-    if (WaitForSingleObject(processInformation.hProcess, 2000) != WAIT_OBJECT_0)
+    if (LOG_LAST_ERROR_IF(WaitForSingleObject(processInformation.hProcess, 2000) != WAIT_OBJECT_0))
     {
         // Timeout occured, terminate the where.exe process and return.
-        TerminateProcess(processInformation.hProcess, 2);
+        FINISHED_LAST_ERROR_IF(!TerminateProcess(processInformation.hProcess, 2));
-        hr = HRESULT_FROM_WIN32(ERROR_TIMEOUT);
+        FINISHED_IF_FAILED(HRESULT_FROM_WIN32(ERROR_TIMEOUT));
-        goto Finished;
     }
 
     //
@@ -520,10 +510,7 @@ HOSTFXR_UTILITY::InvokeWhereToFindDotnet()
     // and 2 if there was an error. Check if the exit code is 1 and set
     // a new hr result saying it couldn't find dotnet.exe
     //
-    if (!GetExitCodeProcess(processInformation.hProcess, &dwExitCode))
+    FINISHED_LAST_ERROR_IF (!GetExitCodeProcess(processInformation.hProcess, &dwExitCode));
-    {
-        goto Finished;
-    }
 
     //
     // In this block, if anything fails, we will goto our fallback of
@@ -531,7 +518,7 @@ HOSTFXR_UTILITY::InvokeWhereToFindDotnet()
     //
     if (dwExitCode != 0)
     {
-        goto Finished;
+        FINISHED_IF_FAILED(E_FAIL);
     }
 
     // Where succeeded.
@@ -539,38 +526,30 @@ HOSTFXR_UTILITY::InvokeWhereToFindDotnet()
     dwFilePointer = SetFilePointer(hStdOutReadPipe, 0, NULL, FILE_BEGIN);
     if (dwFilePointer == INVALID_SET_FILE_POINTER)
     {
-        goto Finished;
+        FINISHED_IF_FAILED(E_FAIL);
     }
 
     //
     // As the call to where.exe succeeded (dotnet.exe was found), ReadFile should not hang.
     // TODO consider putting ReadFile in a separate thread with a timeout to guarantee it doesn't block.
     //
-    if (!ReadFile(hStdOutReadPipe, pzFileContents, READ_BUFFER_SIZE, &dwNumBytesRead, NULL))
+    FINISHED_LAST_ERROR_IF (!ReadFile(hStdOutReadPipe, pzFileContents, READ_BUFFER_SIZE, &dwNumBytesRead, NULL));
-    {
-        goto Finished;
-    }
 
     if (dwNumBytesRead >= READ_BUFFER_SIZE)
     {
         // This shouldn't ever be this large. We could continue to call ReadFile in a loop,
         // however if someone had this many dotnet.exes on their machine.
-        goto Finished;
+        FINISHED_IF_FAILED(E_FAIL);
     }
 
-    hr = HRESULT_FROM_WIN32(GetLastError());
+    FINISHED_IF_FAILED(struDotnetLocationsString.CopyA(pzFileContents, dwNumBytesRead));
-    if (FAILED(hr = struDotnetLocationsString.CopyA(pzFileContents, dwNumBytesRead)))
+
-    {
+    WLOG_INFOF(L"where.exe invocation returned: %s", struDotnetLocationsString.QueryStr());
-        goto Finished;
-    }
 
     // Check the bitness of the currently running process
     // matches the dotnet.exe found.
-    if (!IsWow64Process(GetCurrentProcess(), &fIsWow64Process))
+    FINISHED_LAST_ERROR_IF (!IsWow64Process(GetCurrentProcess(), &fIsWow64Process));
-    {
+
-        // Calling IsWow64Process failed
-        goto Finished;
-    }
     if (fIsWow64Process)
     {
         // 32 bit mode
@@ -584,6 +563,8 @@ HOSTFXR_UTILITY::InvokeWhereToFindDotnet()
         fIsCurrentProcess64Bit = systemInfo.wProcessorArchitecture == PROCESSOR_ARCHITECTURE_AMD64;
     }
 
+    WLOG_INFOF(L"Current process bitness type detected as isX64=%d", fIsCurrentProcess64Bit);
+
     while (TRUE)
     {
         index = struDotnetLocationsString.IndexOf(L"\r\n", prevIndex);
@@ -591,46 +572,28 @@ HOSTFXR_UTILITY::InvokeWhereToFindDotnet()
         {
             break;
         }
-        if (FAILED(hr = struDotnetSubstring.Copy(&struDotnetLocationsString.QueryStr()[prevIndex], index - prevIndex)))
+
-        {
+        FINISHED_IF_FAILED(struDotnetSubstring.Copy(&struDotnetLocationsString.QueryStr()[prevIndex], index - prevIndex));
-            goto Finished;
-        }
         // \r\n is two wchars, so add 2 here.
         prevIndex = index + 2;
 
-        if (GetBinaryTypeW(struDotnetSubstring.QueryStr(), &dwBinaryType) &&
+        WLOG_INFOF(L"Processing entry %s", struDotnetSubstring.QueryStr());
-            fIsCurrentProcess64Bit == (dwBinaryType == SCS_64BIT_BINARY))
+
+        if (LOG_LAST_ERROR_IF(!GetBinaryTypeW(struDotnetSubstring.QueryStr(), &dwBinaryType)))
+        {
+            continue;
+        }
+
+        WLOG_INFOF(L"Binary type %d", dwBinaryType);
+
+        if (fIsCurrentProcess64Bit == (dwBinaryType == SCS_64BIT_BINARY))
         {
             // The bitness of dotnet matched with the current worker process bitness.
-            result = std::make_optional(struDotnetSubstring.QueryStr());
+            return std::make_optional(struDotnetSubstring.QueryStr());
-            fResult = TRUE;
-            break;
         }
     }
 
-Finished:
+    Finished:
-
-    if (hStdOutReadPipe != INVALID_HANDLE_VALUE)
-    {
-        CloseHandle(hStdOutReadPipe);
-    }
-    if (hStdOutWritePipe != INVALID_HANDLE_VALUE)
-    {
-        CloseHandle(hStdOutWritePipe);
-    }
-    if (processInformation.hProcess != INVALID_HANDLE_VALUE)
-    {
-        CloseHandle(processInformation.hProcess);
-    }
-    if (processInformation.hThread != INVALID_HANDLE_VALUE)
-    {
-        CloseHandle(processInformation.hThread);
-    }
-    if (pwzDotnetName != NULL)
-    {
-        SysFreeString(pwzDotnetName);
-    }
-
     return result;
 }