Make nt_registry Create/OpenRegKey return a scoped object

chrome_elf/nt_registry/nt_registry.h

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 // This API is a usability layer for direct registry access via NTDLL.
 // It allows for "advapi32-free" registry access, which is especially
 // useful for accessing registy from DllMain (holding loader lock),
 // or if a dependency on/linkage of ADVAPI32.dll is not desired.
 
 // The implementation of this API should only use ntdll and kernel32 system
@@ skipping 26 matching lines @@
 // Use HKLM or HKCU to override.
 typedef enum _ROOT_KEY { AUTO = 0, HKLM, HKCU } ROOT_KEY;
 
 // Used with wrapper functions to request registry redirection override.
 // Maps to KEY_WOW64_32KEY and KEY_WOW64_64KEY access flags.
 enum WOW64_OVERRIDE {
   NONE = 0L,
   WOW6432 = KEY_WOW64_32KEY,
   WOW6464 = KEY_WOW64_64KEY
 };
 

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

Please add a small class comment.

"
// ScopedHANDLE wraps a generic Windows HANDLE, allowing it to be
// closed automatically (via NtClose) when the object goes out of scope.
//
// - Use is_valid() function to check validity of internal HANDLE.
// - Use release() function to take ownership of the raw internal HANDLE. 
"

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

Done.

+class ScopedRegKeyHANDLE {

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

Let's make this class generic ScopedHANDLE.  Nothing about a Windows HANDLE is
specific to registry.  It would be easy in the future to re-use this for any
HANDLE usage.

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

The INVALID_HANDLE_VALUE vs nullptr was why I didn't do that. I think it's
better to keep File-y handles separate from kernel handles. How about
ScopedFileHANDLE?

+ public:
+  ScopedRegKeyHANDLE();
+  explicit ScopedRegKeyHANDLE(HANDLE handle);

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

Just for my own c++ learnin':  why did you think to add explicit only to this
constructor (and not the others... or not at all)?  Is it because we want to
force handling of the raw HANDLE passed in?  Or is this just to stop a caller
from doing cross-type assignment like "ScopedHANDLE a = HANDLE win_handle"?  <--
Wouldn't the compiler scream?

From what I understand, the two constructors below will behave as converting
constructors - which lets one "copy initialize" with it?

e.g. 
ScopedHANDLE b(HANDLE win_handle);
ScopedHANDLE a = b;

^^ which will delete b during construction of a?

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

Yeah, it's for this. Otherwise, any old HANDLE could magically convert into a
ScopedHANDLE which we don't really want. This way it has to be explicit, either
in a constructor statement, like one of these:

ScopedHandle x(value);
ScopedHandle x = ScopedHandle(SomeFunctionThatReturnsAHANDLE());

But really I'm just obeying
https://google.github.io/styleguide/cppguide.html#Implicit_Conversions :). 
I'd actually never thought of that! I think you can't really put explicit on
copy/move ctors because they wouldn't be able to do their magic.
Yes, that's definitely why we don't want to allow operator=.

+  ScopedRegKeyHANDLE(ScopedRegKeyHANDLE&& rvalue) : handle_(rvalue.release()) {}

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

Why did you have to release and then set in this constructor and not the ones
below?  I'm assuming it's because the ones below do implicit copying of the
object contents (including |handle_|) before deleting the old, while implicit
copying of a pointer to an object does not copy the contents??

Could I actually use this like:
ScopedHANDLE b(HANDLE win_handle);
ScopedHANDLE a = &b;  // <--- seriously?

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

The ones below are =delete, so they're not implemented. This one is a move
constructor, so the rvalue reference (&&) that's coming in is being moved into
this object. So we don't want it to own the lifetime of the HANDLE it used to
contain, so we're moving that value into the one that's being constructed here.

+  ScopedRegKeyHANDLE(const ScopedRegKeyHANDLE&) = delete;

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

Did you need to add one with "const"?  Is that just so this constructor can take
both const or non-const argument?  And we didn't add "const" in the above
constructor because it's a pointer to an object, which would probably never be
const?

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

You kind of lost me by this point. But the operator= is a typo. (Thanks!) That
one should have a const. The idea is that the const ones can accept both const
and non-const, so we only need the const.

+  void operator=(ScopedRegKeyHANDLE&) = delete;
+  ~ScopedRegKeyHANDLE();
+
+  HANDLE get() { return handle_; }
+  bool is_valid() const { return handle_ != INVALID_HANDLE_VALUE; }

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

I think I'd like is_valid to *also* check for NULL.  Just to be extra careful.

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

How about I assert for non-null? I'd prefer to keep file handle and kernel
handle separate.

+  HANDLE release();

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

Please add tiny comment like:

"
// Caller is responsible for closing the returned HANDLE if !=
INVALID_HANDLE_VALUE.
"

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

Done. Also made it private for now, since we don't need to expose it.

+
+ private:
+  HANDLE handle_;
+};
+
 //------------------------------------------------------------------------------
 // Create, open, delete, close functions
 //------------------------------------------------------------------------------
 
 // Create and/or open a registry key.
 // - This function will recursively create multiple sub-keys if required for
 //   |key_path|.
-// - If the key doesn't need to be left open, pass in nullptr for |out_handle|.
 // - This function will happily succeed if the key already exists.
 // - Optional |out_handle|.  If nullptr, function will close handle when done.

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

Change this comment to be:

// - On success, the returned object will hold the open handle to the
//   deepest subkey.

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

Done.

 //   Otherwise, will hold the open handle to the deepest subkey.
-// - Caller must call CloseRegKey on returned handle (on success).
-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);
 
 // Open existing registry key.
-// - Caller must call CloseRegKey on returned handle (on success).
-// - Optional error code can be returned on failure for extra detail.
-bool OpenRegKey(ROOT_KEY root,
-                const wchar_t* key_path,
-                ACCESS_MASK access,
-                HANDLE* out_handle,
-                NTSTATUS* error_code OPTIONAL);
+// - Optional error code can be returned on failure for extra detail. This will
+// be set if the returned ScopedRegKeyHANDLE.is_valid() == false.
+ScopedRegKeyHANDLE OpenRegKey(ROOT_KEY root,
+                              const wchar_t* key_path,
+                              ACCESS_MASK access,
+                              NTSTATUS* error_code OPTIONAL);
 
 // Delete a registry key.
-// - Caller must still call CloseRegKey after the delete.
-bool DeleteRegKey(HANDLE key);
+bool DeleteRegKey(HANDLE handle);

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

I was fine with the name change, but then I realized that all the HANDLE
arguments further down are called |key| as well.

So let's change it back to "key" for consistency.

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

Oops, sorry, unintentional. I was fiddling a bit more than this CL, but then
walked it back and forgot to return this to what it was. Done.

 
 // Delete a registry key.
 // - WRAPPER: Function opens and closes the target key for caller.
 // - Use |wow64_override| to force redirection behaviour, or pass nt::NONE.
 bool DeleteRegKey(ROOT_KEY root,
                   WOW64_OVERRIDE wow64_override,
                   const wchar_t* key_path);
 
-// Close a registry key handle that was opened with CreateRegKey or OpenRegKey.
-void CloseRegKey(HANDLE key);
-
 //------------------------------------------------------------------------------
 // Getter functions
 //------------------------------------------------------------------------------
 
 // Main function to query a registry value.
 // - Key handle should have been opened with CreateRegKey or OpenRegKey.
 // - Types defined in winnt.h.  E.g.: REG_DWORD, REG_SZ.
 // - Caller is responsible for calling "delete[] *out_buffer" (on success).
 bool QueryRegKeyValue(HANDLE key,
                       const wchar_t* value_name,
                       ULONG* out_type,
                       BYTE** out_buffer,
                       DWORD* out_size);
 
 // Query DWORD value.
 // - WRAPPER: Function works with DWORD data type.
 // - Key handle should have been opened with CreateRegKey or OpenRegKey.
-// - Handle will be left open.  Caller must still call CloseRegKey when done.
 bool QueryRegValueDWORD(HANDLE key,
                         const wchar_t* value_name,
                         DWORD* out_dword);
 
 // Query DWORD value.
 // - WRAPPER: Function opens and closes the target key for caller, and works
 // with DWORD data type.
 // - Use |wow64_override| to force redirection behaviour, or pass nt::NONE.
 bool QueryRegValueDWORD(ROOT_KEY root,
                         WOW64_OVERRIDE wow64_override,
                         const wchar_t* key_path,
                         const wchar_t* value_name,
                         DWORD* out_dword);
 
 // Query SZ (string) value.
 // - WRAPPER: Function works with SZ data type.
 // - Key handle should have been opened with CreateRegKey or OpenRegKey.
-// - Handle will be left open.  Caller must still call CloseRegKey when done.
 bool QueryRegValueSZ(HANDLE key,
                      const wchar_t* value_name,
                      std::wstring* out_sz);
 
 // Query SZ (string) value.
 // - WRAPPER: Function opens and closes the target key for caller, and works
 // with SZ data type.
 // - Use |wow64_override| to force redirection behaviour, or pass nt::NONE.
 bool QueryRegValueSZ(ROOT_KEY root,
                      WOW64_OVERRIDE wow64_override,
                      const wchar_t* key_path,
                      const wchar_t* value_name,
                      std::wstring* out_sz);
 
 // Query MULTI_SZ (multiple strings) value.
 // - WRAPPER: Function works with MULTI_SZ data type.
 // - Key handle should have been opened with CreateRegKey or OpenRegKey.
-// - Handle will be left open.  Caller must still call CloseRegKey when done.
 bool QueryRegValueMULTISZ(HANDLE key,
                           const wchar_t* value_name,
                           std::vector<std::wstring>* out_multi_sz);
 
 // Query MULTI_SZ (multiple strings) value.
 // - WRAPPER: Function opens and closes the target key for caller, and works
 // with MULTI_SZ data type.
 // - Use |wow64_override| to force redirection behaviour, or pass nt::NONE.
 bool QueryRegValueMULTISZ(ROOT_KEY root,
                           WOW64_OVERRIDE wow64_override,
@@ skipping 10 matching lines @@
 // - Types defined in winnt.h.  E.g.: REG_DWORD, REG_SZ.
 bool SetRegKeyValue(HANDLE key,
                     const wchar_t* value_name,
                     ULONG type,
                     const BYTE* data,
                     DWORD data_size);
 
 // Set DWORD value.
 // - WRAPPER: Function works with DWORD data type.
 // - Key handle should have been opened with CreateRegKey or OpenRegKey.
-// - Handle will be left open.  Caller must still call CloseRegKey when done.
 bool SetRegValueDWORD(HANDLE key, const wchar_t* value_name, DWORD value);
 
 // Set DWORD value.
 // - WRAPPER: Function opens and closes the target key for caller, and works
 // with DWORD data type.
 // - Use |wow64_override| to force redirection behaviour, or pass nt::NONE.
 bool SetRegValueDWORD(ROOT_KEY root,
                       WOW64_OVERRIDE wow64_override,
                       const wchar_t* key_path,
                       const wchar_t* value_name,
                       DWORD value);
 
 // Set SZ (string) value.
 // - WRAPPER: Function works with SZ data type.
 // - Key handle should have been opened with CreateRegKey or OpenRegKey.
-// - Handle will be left open.  Caller must still call CloseRegKey when done.
 bool SetRegValueSZ(HANDLE key,
                    const wchar_t* value_name,
                    const std::wstring& value);
 
 // Set SZ (string) value.
 // - WRAPPER: Function opens and closes the target key for caller, and works
 // with SZ data type.
 // - Use |wow64_override| to force redirection behaviour, or pass nt::NONE.
 bool SetRegValueSZ(ROOT_KEY root,
                    WOW64_OVERRIDE wow64_override,
                    const wchar_t* key_path,
                    const wchar_t* value_name,
                    const std::wstring& value);
 
 // Set MULTI_SZ (multiple strings) value.
 // - WRAPPER: Function works with MULTI_SZ data type.
 // - Key handle should have been opened with CreateRegKey or OpenRegKey.
-// - Handle will be left open.  Caller must still call CloseRegKey when done.
 bool SetRegValueMULTISZ(HANDLE key,
                         const wchar_t* value_name,
                         const std::vector<std::wstring>& values);
 
 // Set MULTI_SZ (multiple strings) value.
 // - WRAPPER: Function opens and closes the target key for caller, and works
 // with MULTI_SZ data type.
 // - Use |wow64_override| to force redirection behaviour, or pass nt::NONE.
 bool SetRegValueMULTISZ(ROOT_KEY root,
                         WOW64_OVERRIDE wow64_override,
@@ skipping 13 matching lines @@
 
 // Setter function for test suites that use reg redirection.
 bool SetTestingOverride(ROOT_KEY root, const std::wstring& new_path);
 
 // Getter function for test suites that use reg redirection.
 std::wstring GetTestingOverride(ROOT_KEY root);
 
 };  // namespace nt
 
 #endif  // CHROME_ELF_NT_REGISTRY_NT_REGISTRY_H_