Make nt_registry Create/OpenRegKey return a scoped object

chrome_elf/nt_registry/nt_registry.h

Index: chrome_elf/nt_registry/nt_registry.h
diff --git a/chrome_elf/nt_registry/nt_registry.h b/chrome_elf/nt_registry/nt_registry.h
index 5115410bcd24359e7186b2d67f3635c663ff372f..b1de0463745d955d4943f266d8caf76e79e01391 100644
--- a/chrome_elf/nt_registry/nt_registry.h
+++ b/chrome_elf/nt_registry/nt_registry.h
@@ -45,6 +45,23 @@ enum WOW64_OVERRIDE {
   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
 //------------------------------------------------------------------------------
@@ -52,28 +69,23 @@ enum WOW64_OVERRIDE {
 // 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.
@@ -82,9 +94,6 @@ 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
 //------------------------------------------------------------------------------
@@ -102,7 +111,6 @@ bool QueryRegKeyValue(HANDLE key,
 // 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);
@@ -120,7 +128,6 @@ bool QueryRegValueDWORD(ROOT_KEY root,
 // 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);
@@ -138,7 +145,6 @@ bool QueryRegValueSZ(ROOT_KEY root,
 // 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);
@@ -169,7 +175,6 @@ bool SetRegKeyValue(HANDLE key,
 // 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.
@@ -185,7 +190,6 @@ bool SetRegValueDWORD(ROOT_KEY root,
 // 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);
@@ -203,7 +207,6 @@ bool SetRegValueSZ(ROOT_KEY root,
 // 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);