Add holder for registry handle to ensure cleanup of native resources. (#1711)

Add a holder for the registry key to ensure it is always cleaned up when it goes out of scope.
Exceptions should be caught as const.

samples/integration/cmake-fetch-content/src/main.cpp

@@ -39,9 +39,9 @@ int main(int argc, char* argv[])
       std::cout << blob.Name << std::endl;
     }
   }
-  catch (std::exception& e)
+  catch (const std::exception& ex)
   {
-    std::cout << e.what();
+    std::cout << ex.what();
     return 1;
   }
 

sdk/core/azure-core/CMakeLists.txt

@@ -51,6 +51,7 @@ set(
     inc/azure/core/http/policy.hpp
     inc/azure/core/http/transport.hpp
     inc/azure/core/internal/contract.hpp
+    inc/azure/core/internal/hkeyholder.hpp
     inc/azure/core/internal/http/pipeline.hpp
     inc/azure/core/internal/json_serializable.hpp
     inc/azure/core/internal/json.hpp

sdk/core/azure-core/inc/azure/core/internal/hkeyholder.hpp

@@ -0,0 +1,58 @@
+// Copyright (c) Microsoft Corporation. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+/**
+ * @file
+ * @brief Internal HKEY holder.
+ *
+ */
+#pragma once
+
+#if defined(AZ_PLATFORM_WINDOWS)
+#if !defined(WIN32_LEAN_AND_MEAN)
+#define WIN32_LEAN_AND_MEAN
+#endif
+#if !defined(NOMINMAX)
+#define NOMINMAX
+#endif
+
+#include <windows.h>
+
+namespace Azure { namespace Core { namespace Internal {
+
+  /**
+   * @brief HKEYHolder ensures native handle resource is released.
+   */
+  class HKEYHolder {
+  private:
+    HKEY m_value = nullptr;
+
+  public:
+    explicit HKEYHolder() noexcept : m_value(nullptr){};
+
+    ~HKEYHolder() noexcept
+    {
+      if (m_value != nullptr)
+      {
+        ::RegCloseKey(m_value);
+      }
+    }
+
+    void operator=(HKEY p) noexcept
+    {
+      if (p != nullptr)
+      {
+        m_value = p;
+      }
+    }
+
+    operator HKEY() noexcept { return m_value; }
+
+    operator HKEY*() noexcept { return &m_value; }
+
+    HKEY* operator&() noexcept { return &m_value; }
+  };
+
+}}} // namespace Azure::Core::Internal
+
+#endif

sdk/core/azure-core/src/http/curl/curl.cpp

@@ -466,9 +466,9 @@ void CurlSession::ParseChunkSize(Context const& context)
         {
           this->m_chunkSize = static_cast<int64_t>(std::stoull(strChunkSize, nullptr, 16));
         }
-        catch (std::invalid_argument& err)
+        catch (const std::invalid_argument& ex)
         {
-          (void)err;
+          (void)ex;
           // Server can return something like `\n\r\n` for a chunk of zero length data. This is
           // allowed by RFC. `stoull` will throw invalid_argument if there is not at least one hex
           // digit to be parsed. For those cases, we consider the response as zero-length.

sdk/core/azure-core/src/http/retry_policy.cpp

@@ -150,7 +150,7 @@ std::unique_ptr<RawResponse> Azure::Core::Http::RetryPolicy::Send(
         return response;
       }
     }
-    catch (TransportException const&)
+    catch (const TransportException&)
     {
       if (!ShouldRetryOnTransportFailure(m_retryOptions, attempt, retryAfter))
       {

sdk/core/azure-core/src/http/telemetry_policy.cpp

@@ -16,6 +16,9 @@
 #endif
 
 #include <windows.h>
+
+#include "azure/core/internal/hkeyholder.hpp"
+
 #elif defined(AZ_PLATFORM_POSIX)
 #include <sys/utsname.h>
 #endif
@@ -29,52 +32,35 @@ std::string GetOSVersion()
 #if !defined(WINAPI_PARTITION_DESKTOP) \
     || WINAPI_PARTITION_DESKTOP // See azure/core/platform.hpp for explanation.
   {
-    HKEY regKey{};
-    auto regKeyOpened = false;
-    try
+    Azure::Core::Internal::HKEYHolder regKey;
+    if (RegOpenKeyExA(
+            HKEY_LOCAL_MACHINE,
+            "SOFTWARE\\Microsoft\\Windows NT\\CurrentVersion",
+            0,
+            KEY_READ,
+            &regKey)
+        == ERROR_SUCCESS)
     {
-      if (RegOpenKeyExA(
-              HKEY_LOCAL_MACHINE,
-              "SOFTWARE\\Microsoft\\Windows NT\\CurrentVersion",
-              0,
-              KEY_READ,
-              &regKey)
-          == ERROR_SUCCESS)
+      auto first = true;
+      static constexpr char const* regValues[]{
+          "ProductName", "CurrentVersion", "CurrentBuildNumber", "BuildLabEx"};
+      for (auto regValue : regValues)
       {
-        regKeyOpened = true;
+        char valueBuf[200] = {};
+        DWORD valueBufSize = sizeof(valueBuf);
 
-        auto first = true;
-        static constexpr char const* regValues[]{
-            "ProductName", "CurrentVersion", "CurrentBuildNumber", "BuildLabEx"};
-        for (auto regValue : regValues)
+        if (RegQueryValueExA(regKey, regValue, NULL, NULL, (LPBYTE)valueBuf, &valueBufSize)
+            == ERROR_SUCCESS)
         {
-          char valueBuf[200] = {};
-          DWORD valueBufSize = sizeof(valueBuf);
-
-          if (RegQueryValueExA(regKey, regValue, NULL, NULL, (LPBYTE)valueBuf, &valueBufSize)
-              == ERROR_SUCCESS)
+          if (valueBufSize > 0)
           {
-            if (valueBufSize > 0)
-            {
-              osVersionInfo << (first ? "" : " ")
-                            << std::string(valueBuf, valueBuf + (valueBufSize - 1));
-              first = false;
-            }
+            osVersionInfo << (first ? "" : " ")
+                          << std::string(valueBuf, valueBuf + (valueBufSize - 1));
+            first = false;
           }
         }
-
-        RegCloseKey(regKey);
       }
     }
-    catch (...)
-    {
-      if (regKeyOpened)
-      {
-        RegCloseKey(regKey);
-      }
-
-      throw;
-    }
   }
 #else
   {

sdk/core/azure-core/test/ut/policy.cpp

@@ -57,9 +57,9 @@ TEST(Policy, throwWhenNoTransportPolicyMessage)
   {
     pipeline.Send(Azure::Core::GetApplicationContext(), request);
   }
-  catch (std::invalid_argument& err)
+  catch (const std::invalid_argument& ex)
   {
-    EXPECT_STREQ("Invalid pipeline. No transport policy found. Endless policy.", err.what());
+    EXPECT_STREQ("Invalid pipeline. No transport policy found. Endless policy.", ex.what());
   }
 }