[Chrome ELF] New NT registry API.

chrome_elf/chrome_elf_reg.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.

[robertshield, 2016/04/20 16:55:53]

Suggest having at the top of this file some explanation for why it exists
"advapi32-free registry access" and some notes about usage (e.g. the HANDLEs
passed to many of the GetXXX functions should be obtained via Open/CreateRegKey
in this file.

[penny, 2016/05/28 01:34:23]

Done.  Comments about HANDLEs have been added to the function documentation.

+
+#ifndef CHROME_ELF_CHROME_ELF_REG_H_
+#define CHROME_ELF_CHROME_ELF_REG_H_
+
+#include <vector>
+
+#include "base/strings/string16.h"
+#include "sandbox/win/src/nt_internals.h"  // NTSTATUS
+
+namespace nt {
+
+extern base::string16 HKLM_override;
+extern base::string16 HKCU_override;
+
+// AUTO will choose depending on system install or not.
+// Use HKLM or HKCU to override.
+typedef enum _ROOT_KEY { AUTO = 0, HKLM, HKCU } ROOT_KEY, *PROOT_KEY;

[robertshield, 2016/04/20 16:55:53]

We don't usually explicitly typedef for a pointer-to-type, I don't think you
need the PROOT_KEY bit.

[penny, 2016/05/28 01:34:23]

Done.  I just always do that as an old habit.

+
+// If you don't need the new handle left open, pass in nullptr.
+// Also, will happily succeed if key already exists.
+bool CreateRegKey(ROOT_KEY root,
+                  const wchar_t* key_path,
+                  ACCESS_MASK access,
+                  HANDLE* out_handle OPTIONAL);
+
+// If key handle is passed in, caller must still call CloseRegKey
+// after the delete.
+bool DeleteRegKey(HANDLE key);

[robertshield, 2016/04/20 16:55:53]

please add separate comments for each function.

[penny, 2016/05/28 01:34:23]

Done.

+bool DeleteRegKey(ROOT_KEY root, const wchar_t* key_path);
+
+// Caller must call CloseRegKey on returned handle (on success).
+bool OpenRegKey(ROOT_KEY root,
+                const wchar_t* key_path,
+                ACCESS_MASK access,
+                HANDLE* out_handle,
+                NTSTATUS* error_code OPTIONAL);
+
+void CloseRegKey(HANDLE key);
+
+//------------------------------------------------------------------------------
+// Getter functions
+//------------------------------------------------------------------------------
+
+// Caller responsible for calling "delete[] *out_buffer".
+// Types defined in winnt.h.  E.g.: REG_DWORD, REG_SZ.
+bool QueryRegKeyValue(HANDLE key,

[robertshield, 2016/04/20 16:55:53]

For all of the functions in this file that take a HANDLE, it looks like it is
important to have obtained that handle from OpenRegKey or CreateRegKey in order
for the override stuff to take effect. 

It would be worth having a comment somewhere to this effect.

[robertshield, 2016/04/20 16:55:53]

Why is this function called QueryXXX when the others are called GetXXX? Consider
renaming it for consistency.

[penny, 2016/05/28 01:34:23]

Done.

[penny, 2016/05/28 01:34:23]

Done.  I've changed to always use "Query".  This main query function is named
after the NTDLL API, which uses the term "query".  I'd like to keep this
consistent with the NTDLL function names.  I was using "Get" only for wrapper
functions (that call QueryRegKeyValue underneath).  But I'm happy to make them
"Query" for consistency.  It's a fair point.

[penny, 2016/05/28 01:34:23]

Done.

+                      const wchar_t* value_name,
+                      ULONG* out_type,
+                      BYTE** out_buffer,
+                      DWORD* out_size);
+
+// If key handle is passed in, it will be left open.  Otherwise,
+// the registry key will be opened, queried, then closed.

[robertshield, 2016/04/20 16:55:53]

Please attach comments to the individual functions, the semantics of these two
are different enough to make it worth separating them.

micro nit: word wrapping looks off. If you're using Sublime, Wrap Plus is handy,
otherwise git cl format should take care of it.

[penny, 2016/05/28 01:34:23]

Done.  I always use "git cl format" before uploading.  I wonder if it doesn't
fix comment lines that are too short?  Maybe it only handles lines that are too
long?

+bool GetRegValue_DWORD(HANDLE key, const wchar_t* value_name, DWORD* out_dword);
+bool GetRegValue_DWORD(ROOT_KEY root,
+                       const wchar_t* key_path,
+                       const wchar_t* value_name,
+                       DWORD* out_dword);
+
+bool GetRegValue_SZ(HANDLE key,

[robertshield, 2016/04/20 15:45:47]

Minor style nit: base's registry functions are named without underscores, I'd
mildly prefer the form GetRegValueSZ, GetRegValueMULTISZ, GetRegValueDWORD, etc.

[penny, 2016/05/28 01:34:23]

Done.

+                    const wchar_t* value_name,
+                    base::string16* out_sz);
+bool GetRegValue_SZ(ROOT_KEY root,
+                    const wchar_t* key_path,
+                    const wchar_t* value_name,
+                    base::string16* out_sz);
+
+bool GetRegValue_MULTI_SZ(HANDLE key,
+                          const wchar_t* value_name,
+                          std::vector<base::string16>* out_multi_sz);
+bool GetRegValue_MULTI_SZ(ROOT_KEY root,
+                          const wchar_t* key_path,
+                          const wchar_t* value_name,
+                          std::vector<base::string16>* out_multi_sz);
+
+//------------------------------------------------------------------------------
+// Setter functions
+//------------------------------------------------------------------------------
+
+// Types defined in winnt.h.  E.g.: REG_DWORD, REG_SZ.
+bool SetRegKeyValue(HANDLE key,
+                    const wchar_t* value_name,
+                    ULONG type,
+                    BYTE* data,
+                    DWORD data_size);
+
+// If key handle is passed in, it will be left open.  Otherwise,
+// the registry key will be opened, set, then closed.
+bool SetRegValue_DWORD(HANDLE key, const wchar_t* value_name, DWORD value);
+bool SetRegValue_DWORD(ROOT_KEY root,
+                       const wchar_t* key_path,
+                       const wchar_t* value_name,
+                       DWORD value);
+
+// Passing in pointer to string for efficiency.

[robertshield, 2016/04/20 16:55:53]

Use const ref instead of pointer here and everywhere else non POD types are
being passed as input values. Don't need the efficiency comment here. 

[penny, 2016/05/28 01:34:23]

Done.  I think done.  I'm new to the strange '&' refs in c++.  I've adjusted the
base::string16s and std::vectors that are being passed into
SetRegValueSZ/MULTISZ to be const refs.  Oh, and the BYTE* is now const for
SetRegKeyValue.

+bool SetRegValue_SZ(HANDLE key,
+                    const wchar_t* value_name,
+                    base::string16* value);
+bool SetRegValue_SZ(ROOT_KEY root,
+                    const wchar_t* key_path,
+                    const wchar_t* value_name,
+                    base::string16* value);
+
+// Passing in pointer to vector for efficiency.
+bool SetRegValue_MULTI_SZ(HANDLE key,
+                          const wchar_t* value_name,
+                          std::vector<base::string16>* values);
+bool SetRegValue_MULTI_SZ(ROOT_KEY root,
+                          const wchar_t* key_path,
+                          const wchar_t* value_name,
+                          std::vector<base::string16>* values);
+
+//------------------------------------------------------------------------------
+// Utils
+//------------------------------------------------------------------------------
+
+// Handy function to get the current user sid in string form.
+// Does NOT use advapi32, only ntdll.

[robertshield, 2016/04/20 16:55:53]

Suggest:

Returns the current user SID in string form.

The other part of the comment isn't likely to be useful to future readers.

[penny, 2016/05/28 01:34:23]

Done.

+base::string16 GetCurrentUserSidString();
+
+};  // namespace nt
+
+#endif  // CHROME_ELF_CHROME_ELF_REG_H_