Make nt_registry Create/OpenRegKey return a scoped object

chrome_elf/nt_registry/nt_registry.cc

Index: chrome_elf/nt_registry/nt_registry.cc
diff --git a/chrome_elf/nt_registry/nt_registry.cc b/chrome_elf/nt_registry/nt_registry.cc
index 4c65af8daff7c76ea5a9e4a0255da0225aeb58a4..b9741eb45412d99cc1440d0cb8b9d235fec5c603 100644
--- a/chrome_elf/nt_registry/nt_registry.cc
+++ b/chrome_elf/nt_registry/nt_registry.cc
@@ -565,19 +565,34 @@ NTSTATUS CreateKeyWrapper(const std::wstring& key_path,
 
 namespace nt {
 
+ScopedRegKeyHANDLE::ScopedRegKeyHANDLE() : handle_(INVALID_HANDLE_VALUE) {}
+
+ScopedRegKeyHANDLE::ScopedRegKeyHANDLE(HANDLE handle) : handle_(handle) {}
+
+ScopedRegKeyHANDLE::~ScopedRegKeyHANDLE() {
+  if (!g_initialized)

[penny, 2016/11/21 21:35:46]

Let's first add a check for is_valid().  I'd rather not do any of this if
|handle_| is INVALID_HANDLE_VALUE or NULL.

I definitely don't want to clash with any debug or safety measures related to
calling NtClose on an invalid handle.

[scottmg, 2016/11/22 22:47:36]

Good catch! Done.

+    InitNativeRegApi();
+  g_nt_close(handle_);
+}
+
+HANDLE ScopedRegKeyHANDLE::release() {
+  HANDLE result = handle_;
+  handle_ = INVALID_HANDLE_VALUE;
+  return result;
+}
+
 //------------------------------------------------------------------------------
 // Create, open, delete, close functions
 //------------------------------------------------------------------------------
 
-bool CreateRegKey(ROOT_KEY root,
-                  const wchar_t* key_path,
-                  ACCESS_MASK access,
-                  HANDLE* out_handle OPTIONAL) {
+ScopedRegKeyHANDLE CreateRegKey(ROOT_KEY root,
+                                const wchar_t* key_path,
+                                ACCESS_MASK access) {
   // |key_path| can be null or empty, but it can't be longer than
   // |g_kRegMaxPathLen| at this point.
   if (key_path != nullptr &&
       ::wcsnlen(key_path, g_kRegMaxPathLen + 1) == g_kRegMaxPathLen + 1)
-    return false;
+    return ScopedRegKeyHANDLE();
 
   if (!g_initialized)
     InitNativeRegApi();
@@ -596,14 +611,14 @@ bool CreateRegKey(ROOT_KEY root,
   std::vector<std::wstring> subkeys;
   if (!ParseFullRegPath(ConvertRootKey(root), redirected_key_path,
                         &current_path, &subkeys))
-    return false;
+    return ScopedRegKeyHANDLE();
 
   // Open the base hive first.  It should always exist already.
   HANDLE last_handle = INVALID_HANDLE_VALUE;
   NTSTATUS status =
       CreateKeyWrapper(current_path, access, &last_handle, nullptr);
   if (!NT_SUCCESS(status))
-    return false;
+    return ScopedRegKeyHANDLE();
 
   size_t subkeys_size = subkeys.size();
   if (subkeys_size != 0)
@@ -648,27 +663,20 @@ bool CreateRegKey(ROOT_KEY root,
     g_nt_close(handle);
   }
   if (!success)
-    return false;
-
-  // See if caller wants the handle left open.
-  if (out_handle)
-    *out_handle = last_handle;
-  else
-    g_nt_close(last_handle);
+    return ScopedRegKeyHANDLE();
 
-  return true;
+  return ScopedRegKeyHANDLE(last_handle);
 }
 
-bool OpenRegKey(ROOT_KEY root,
-                const wchar_t* key_path,
-                ACCESS_MASK access,
-                HANDLE* out_handle,
-                NTSTATUS* error_code OPTIONAL) {
+ScopedRegKeyHANDLE OpenRegKey(ROOT_KEY root,
+                              const wchar_t* key_path,
+                              ACCESS_MASK access,
+                              NTSTATUS* error_code OPTIONAL) {
   // |key_path| can be null or empty, but it can't be longer than

[penny, 2016/11/21 21:35:47]

Above this, let's set a default error code (for if we want to bail out before
call to NtOpenKey).

if (error_code)
  *error_code = STATUS_UNSUCCESSFUL;

[scottmg, 2016/11/22 22:47:36]

Done.

   // |g_kRegMaxPathLen| at this point.
   if (key_path != nullptr &&
       ::wcsnlen(key_path, g_kRegMaxPathLen + 1) == g_kRegMaxPathLen + 1)
-    return false;
+    return ScopedRegKeyHANDLE();
 
   if (!g_initialized)
     InitNativeRegApi();
@@ -676,7 +684,7 @@ bool OpenRegKey(ROOT_KEY root,
   NTSTATUS status = STATUS_UNSUCCESSFUL;

[penny, 2016/11/21 21:35:46]

Delete this initialization.

[scottmg, 2016/11/22 22:47:36]

Done.

   UNICODE_STRING key_path_uni = {};
   OBJECT_ATTRIBUTES obj = {};
-  *out_handle = INVALID_HANDLE_VALUE;
+  HANDLE result_handle = INVALID_HANDLE_VALUE;

[penny, 2016/11/21 21:35:47]

Move initialization of HANDLE result_handle down to just above the call to
g_nt_open_key_ex (line 704).

[scottmg, 2016/11/22 22:47:35]

Done.

 
   if (root == nt::AUTO)
     root = g_system_install ? nt::HKLM : nt::HKCU;
@@ -693,53 +701,32 @@ bool OpenRegKey(ROOT_KEY root,
   InitializeObjectAttributes(&obj, &key_path_uni, OBJ_CASE_INSENSITIVE, NULL,
                              NULL);
 
-  status = g_nt_open_key_ex(out_handle, access, &obj, 0);
+  status = g_nt_open_key_ex(&result_handle, access, &obj, 0);

[penny, 2016/11/21 21:35:47]

NTSTATUS status = ...

[scottmg, 2016/11/22 22:47:36]

Done.

   // See if caller wants the NTSTATUS.
   if (error_code)

[penny, 2016/11/21 21:35:46]

Let's move this down a tad, so that we only set |error_code| on failure.

if (!NT_SUCCESS(status)) {
  // See if caller wants the NTSTATUS.
  if (error_code)
    *error_code = status;
  return ScopedHANDLE();
}

return ScopedHANDLE(result_handle);


The above should replace line 705 to 712.

[scottmg, 2016/11/22 22:47:35]

Done.

     *error_code = status;
 
   if (NT_SUCCESS(status))
-    return true;
+    return ScopedRegKeyHANDLE(result_handle);
 
-  return false;
+  return ScopedRegKeyHANDLE();
 }
 
 bool DeleteRegKey(HANDLE key) {
   if (!g_initialized)
     InitNativeRegApi();
-
-  NTSTATUS status = STATUS_UNSUCCESSFUL;
-
-  status = g_nt_delete_key(key);
-
-  if (NT_SUCCESS(status))
-    return true;
-
-  return false;
+  return NT_SUCCESS(g_nt_delete_key(key));

[penny, 2016/11/21 21:35:47]

Do my ocd a favour and add an empty line above this return.  :D

[scottmg, 2016/11/22 22:47:36]

Ugly! If you insist.

 }
 
 // wrapper function
 bool DeleteRegKey(ROOT_KEY root,
                   WOW64_OVERRIDE wow64_override,
                   const wchar_t* key_path) {
-  HANDLE key = INVALID_HANDLE_VALUE;
-
-  if (!OpenRegKey(root, key_path, DELETE | wow64_override, &key, nullptr))
+  ScopedRegKeyHANDLE key =
+      OpenRegKey(root, key_path, DELETE | wow64_override, nullptr);
+  if (!key.is_valid())
     return false;
-
-  if (!DeleteRegKey(key)) {
-    CloseRegKey(key);
-    return false;
-  }
-
-  CloseRegKey(key);
-  return true;
-}
-
-void CloseRegKey(HANDLE key) {
-  if (!g_initialized)
-    InitNativeRegApi();
-  g_nt_close(key);
+  return DeleteRegKey(key.get());

[penny, 2016/11/21 21:35:47]

Do my ocd a favour and add an empty line above this return.  :D

[scottmg, 2016/11/22 22:47:35]

Done.

 }
 
 //------------------------------------------------------------------------------
@@ -810,18 +797,11 @@ bool QueryRegValueDWORD(ROOT_KEY root,
                         const wchar_t* key_path,
                         const wchar_t* value_name,
                         DWORD* out_dword) {
-  HANDLE key = INVALID_HANDLE_VALUE;
-
-  if (!OpenRegKey(root, key_path, KEY_QUERY_VALUE | wow64_override, &key, NULL))
-    return false;
-
-  if (!QueryRegValueDWORD(key, value_name, out_dword)) {
-    CloseRegKey(key);
+  ScopedRegKeyHANDLE key =
+      OpenRegKey(root, key_path, KEY_QUERY_VALUE | wow64_override, nullptr);
+  if (!key.is_valid())
     return false;
-  }
-
-  CloseRegKey(key);
-  return true;
+  return QueryRegValueDWORD(key.get(), value_name, out_dword);

[penny, 2016/11/21 21:35:47]

Do my ocd a favour and add an empty line above this return.  :D

[scottmg, 2016/11/22 22:47:35]

Done.

 }
 
 // wrapper function
@@ -833,8 +813,9 @@ bool QueryRegValueSZ(HANDLE key,
   ULONG type = REG_NONE;
 
   if (!QueryRegKeyValue(key, value_name, &type, &value_bytes, &ret_size) ||
-      type != REG_SZ)
+      type != REG_SZ) {
     return false;
+  }
 
   *out_sz = reinterpret_cast<wchar_t*>(value_bytes);
 
@@ -848,18 +829,11 @@ bool QueryRegValueSZ(ROOT_KEY root,
                      const wchar_t* key_path,
                      const wchar_t* value_name,
                      std::wstring* out_sz) {
-  HANDLE key = INVALID_HANDLE_VALUE;
-
-  if (!OpenRegKey(root, key_path, KEY_QUERY_VALUE | wow64_override, &key, NULL))
-    return false;
-
-  if (!QueryRegValueSZ(key, value_name, out_sz)) {
-    CloseRegKey(key);
+  ScopedRegKeyHANDLE key =
+      OpenRegKey(root, key_path, KEY_QUERY_VALUE | wow64_override, nullptr);
+  if (!key.is_valid())
     return false;
-  }
-
-  CloseRegKey(key);
-  return true;
+  return QueryRegValueSZ(key.get(), value_name, out_sz);

[penny, 2016/11/21 21:35:47]

Do my ocd a favour and add an empty line above this return.  :D

[scottmg, 2016/11/22 22:47:36]

Done.

 }
 
 // wrapper function
@@ -902,18 +876,11 @@ bool QueryRegValueMULTISZ(ROOT_KEY root,
                           const wchar_t* key_path,
                           const wchar_t* value_name,
                           std::vector<std::wstring>* out_multi_sz) {
-  HANDLE key = INVALID_HANDLE_VALUE;
-
-  if (!OpenRegKey(root, key_path, KEY_QUERY_VALUE | wow64_override, &key, NULL))
+  ScopedRegKeyHANDLE key =
+      OpenRegKey(root, key_path, KEY_QUERY_VALUE | wow64_override, nullptr);
+  if (!key.is_valid())
     return false;
-
-  if (!QueryRegValueMULTISZ(key, value_name, out_multi_sz)) {
-    CloseRegKey(key);
-    return false;
-  }
-
-  CloseRegKey(key);
-  return true;
+  return QueryRegValueMULTISZ(key.get(), value_name, out_multi_sz);

[penny, 2016/11/21 21:35:47]

Do my ocd a favour and add an empty line above this return.  :D

[scottmg, 2016/11/22 22:47:36]

Done.

 }
 
 //------------------------------------------------------------------------------
@@ -954,17 +921,11 @@ bool SetRegValueDWORD(ROOT_KEY root,
                       const wchar_t* key_path,
                       const wchar_t* value_name,
                       DWORD value) {
-  HANDLE key = INVALID_HANDLE_VALUE;
-
-  if (!OpenRegKey(root, key_path, KEY_SET_VALUE | wow64_override, &key, NULL))
-    return false;
-
-  if (!SetRegValueDWORD(key, value_name, value)) {
-    CloseRegKey(key);
+  ScopedRegKeyHANDLE key =
+      OpenRegKey(root, key_path, KEY_SET_VALUE | wow64_override, nullptr);
+  if (!key.is_valid())
     return false;
-  }
-
-  return true;
+  return SetRegValueDWORD(key.get(), value_name, value);

[penny, 2016/11/21 21:35:47]

Do my ocd a favour and add an empty line above this return.  :D

[scottmg, 2016/11/22 22:47:35]

Done.

 }
 
 // wrapper function
@@ -987,17 +948,11 @@ bool SetRegValueSZ(ROOT_KEY root,
                    const wchar_t* key_path,
                    const wchar_t* value_name,
                    const std::wstring& value) {
-  HANDLE key = INVALID_HANDLE_VALUE;
-
-  if (!OpenRegKey(root, key_path, KEY_SET_VALUE | wow64_override, &key, NULL))
-    return false;
-
-  if (!SetRegValueSZ(key, value_name, value)) {
-    CloseRegKey(key);
+  ScopedRegKeyHANDLE key =
+      OpenRegKey(root, key_path, KEY_SET_VALUE | wow64_override, nullptr);
+  if (!key.is_valid())
     return false;
-  }
-
-  return true;
+  return SetRegValueSZ(key.get(), value_name, value);

[penny, 2016/11/21 21:35:46]

Do my ocd a favour and add an empty line above this return.  :D

[scottmg, 2016/11/22 22:47:36]

Done.

 }
 
 // wrapper function
@@ -1027,7 +982,8 @@ bool SetRegValueMULTISZ(HANDLE key,
     return false;
 
   return SetRegKeyValue(
-      key, value_name, REG_MULTI_SZ, reinterpret_cast<BYTE*>(builder.data()),
+      key, value_name, REG_MULTI_SZ,
+      reinterpret_cast<BYTE*>(builder.data()),

[penny, 2016/11/21 21:35:47]

I'm just curious why this got moved down... it wasn't over 80 chars.

(I honestly prefer one argument per line, but that's not our coding standard.)

[scottmg, 2016/11/22 22:47:36]

Forgot to reformat, done.
Agreed. (Crashpad dissented in this way and has a customized clang-format file.
Shhhh!)

       (static_cast<DWORD>(builder.size()) + 1) * sizeof(wchar_t));
 }
 
@@ -1037,17 +993,11 @@ bool SetRegValueMULTISZ(ROOT_KEY root,
                         const wchar_t* key_path,
                         const wchar_t* value_name,
                         const std::vector<std::wstring>& values) {
-  HANDLE key = INVALID_HANDLE_VALUE;
-
-  if (!OpenRegKey(root, key_path, KEY_SET_VALUE | wow64_override, &key, NULL))
+  ScopedRegKeyHANDLE key =
+      OpenRegKey(root, key_path, KEY_SET_VALUE | wow64_override, nullptr);
+  if (!key.is_valid())
     return false;
-
-  if (!SetRegValueMULTISZ(key, value_name, values)) {
-    CloseRegKey(key);
-    return false;
-  }
-
-  return true;
+  return SetRegValueMULTISZ(key.get(), value_name, values);

[penny, 2016/11/21 21:35:46]

Do my ocd a favour and add an empty line above this return.  :D

[scottmg, 2016/11/22 22:47:35]

Done.

 }
 
 //------------------------------------------------------------------------------