Add WOW64 support and DeleteEmptyKey to base::win::registry.

base/win/registry.cc

 // Copyright (c) 2012 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.
 
 #include "base/win/registry.h"
 
 #include <shlwapi.h>
 #include <algorithm>
 
 #include "base/logging.h"
 #include "base/strings/string_util.h"
 #include "base/threading/thread_restrictions.h"
-
-#pragma comment(lib, "shlwapi.lib")  // for SHDeleteKey
+#include "base/win/windows_version.h"
 
 namespace base {
 namespace win {
 
 namespace {
 
 // RegEnumValue() reports the number of characters from the name that were
 // written to the buffer, not how many there are. This constant is the maximum
 // name size, such that a buffer with this size should read any name.
 const DWORD MAX_REGISTRY_NAME_SIZE = 16384;
 
 // Registry values are read as BYTE* but can have wchar_t* data whose last
 // wchar_t is truncated. This function converts the reported |byte_size| to
 // a size in wchar_t that can store a truncated wchar_t if necessary.
 inline DWORD to_wchar_size(DWORD byte_size) {
   return (byte_size + sizeof(wchar_t) - 1) / sizeof(wchar_t);
 }
 
+// Mask to pull WOW64 access flags out of REGSAM access.
+const REGSAM kWow64AccessMask = KEY_WOW64_32KEY | KEY_WOW64_64KEY;
+
 }  // namespace
 
 // RegKey ----------------------------------------------------------------------
 
 RegKey::RegKey()
     : key_(NULL),
       watch_event_(0) {

[grt (UTC plus 2), 2014/05/17 12:54:33]

wow64access_(0)

[Will Harris, 2014/05/19 21:49:03]

Done.

 }
 
 RegKey::RegKey(HKEY key)
     : key_(key),
       watch_event_(0) {

[grt (UTC plus 2), 2014/05/17 12:54:33]

this ctor needs to take the wow64 bits of |key| as a parameter, right? if that's
the way to go, and those are the only two bits that should ever be set, i would
also suggest a:
  DCHECK_EQ(0, (access & ~kWow64AccessMask));
to make sure no other bits are set

[Will Harris, 2014/05/19 21:49:03]

HEKY is not RegKey - no way to change/modify access here.

 }
 
 RegKey::RegKey(HKEY rootkey, const wchar_t* subkey, REGSAM access)

[grt (UTC plus 2), 2014/05/17 12:54:33]

wow64access_(0)

what does it mean for rootkey and subkey to both be 0/NULL? does anyone call
this that way? maybe this should be just after the DCHECK on line 58:
    wow64access_ = access & kWow64AccessMask;

[Will Harris, 2014/05/19 21:49:03]

yes, good spot, I looked through the code and can't see any callers to this
path, but it's best to be safe here.

     : key_(NULL),
       watch_event_(0) {
   if (rootkey) {
     if (access & (KEY_SET_VALUE | KEY_CREATE_SUB_KEY | KEY_CREATE_LINK))
       Create(rootkey, subkey, access);
     else
       Open(rootkey, subkey, access);
   } else {
     DCHECK(!subkey);
   }
@@ skipping 11 matching lines @@
 LONG RegKey::CreateWithDisposition(HKEY rootkey, const wchar_t* subkey,
                                    DWORD* disposition, REGSAM access) {
   DCHECK(rootkey && subkey && access && disposition);
   HKEY subhkey = NULL;
   LONG result = RegCreateKeyEx(rootkey, subkey, 0, NULL,
                                REG_OPTION_NON_VOLATILE, access, NULL, &subhkey,
                                disposition);
   if (result == ERROR_SUCCESS) {
     Close();
     key_ = subhkey;
+    wow64access_ = access & kWow64AccessMask;
   }
 
   return result;
 }
 
 LONG RegKey::CreateKey(const wchar_t* name, REGSAM access) {

[grt (UTC plus 2), 2014/05/17 12:54:33]

msdn says "After the application has accessed an alternate registry view using
one of the flags, all subsequent operations (create, delete, or open) on child
registry keys must explicitly use the same flag." it seems that this function
should enforce that. one way would be to have something like:

  if ((access & wow64access_) != wow64access_)
    return ERROR_INVALID_PARAMETER;

[cpu_(ooo_6.6-7.5), 2014/05/19 19:21:13]

very good catch.

[Will Harris, 2014/05/19 21:49:03]

Done.

   DCHECK(name && access);
   HKEY subkey = NULL;
   LONG result = RegCreateKeyEx(key_, name, 0, NULL, REG_OPTION_NON_VOLATILE,
                                access, NULL, &subkey, NULL);
- if (result == ERROR_SUCCESS) {
-   Close();
-
-   key_ = subkey;
+  if (result == ERROR_SUCCESS) {
+    Close();
+    key_ = subkey;
+    wow64access_ = access & kWow64AccessMask;
   }
 
   return result;
 }
 
 LONG RegKey::Open(HKEY rootkey, const wchar_t* subkey, REGSAM access) {
   DCHECK(rootkey && subkey && access);
   HKEY subhkey = NULL;
 
   LONG result = RegOpenKeyEx(rootkey, subkey, 0, access, &subhkey);
   if (result == ERROR_SUCCESS) {
     Close();
     key_ = subhkey;
+    wow64access_ = access & kWow64AccessMask;
   }
 
   return result;
 }
 
 LONG RegKey::OpenKey(const wchar_t* relative_key_name, REGSAM access) {

[grt (UTC plus 2), 2014/05/17 12:54:33]

same comment here about enforcing that |access| contains the same bits as
wow64access_

[Will Harris, 2014/05/19 21:49:03]

Done.

   DCHECK(relative_key_name && access);
   HKEY subkey = NULL;
   LONG result = RegOpenKeyEx(key_, relative_key_name, 0, access, &subkey);
 
   // We have to close the current opened key before replacing it with the new
   // one.
   if (result == ERROR_SUCCESS) {
     Close();
     key_ = subkey;
+    wow64access_ = access & kWow64AccessMask;
   }
   return result;
 }
 
 void RegKey::Close() {
   StopWatching();
   if (key_) {
     ::RegCloseKey(key_);
     key_ = NULL;

[grt (UTC plus 2), 2014/05/17 12:54:33]

wow64access_ = 0;

[Will Harris, 2014/05/19 21:49:03]

Done.

   }
 }
 
 void RegKey::Set(HKEY key) {

[grt (UTC plus 2), 2014/05/17 12:54:33]

this needs to take a REGSAM for the wow64 bits, no? (with a DCHECK_EQ(0, (access
& ~kWow64AccessMask));)?

[Will Harris, 2014/05/19 21:49:03]

it's not possible to check that key was opened with a particular access mask,
and thus check that the access mask being passed here is correct.  Either we
have to change Take() to return both key_ and wow64access_ so it can be passed
into Set(), or we just deny a Take() if the wow64access_ is specified.  There's
still nothing to stop someone from passing a key opened with a WOW64 flag into
Set() here.

   if (key_ != key) {
     Close();
     key_ = key;
   }
 }
 
 HKEY RegKey::Take() {
   StopWatching();
   HKEY key = key_;
   key_ = NULL;

[grt (UTC plus 2), 2014/05/17 12:54:33]

wow64access_ = 0;

[Will Harris, 2014/05/19 21:49:03]

Done.

   return key;
 }
 
 bool RegKey::HasValue(const wchar_t* name) const {
   return RegQueryValueEx(key_, name, 0, NULL, NULL, NULL) == ERROR_SUCCESS;
 }
 
 DWORD RegKey::GetValueCount() const {
   DWORD count = 0;
   LONG result = RegQueryInfoKey(key_, NULL, 0, NULL, NULL, NULL, NULL, &count,
                                 NULL, NULL, NULL, NULL);
   return (result == ERROR_SUCCESS) ? count : 0;
 }
 
 LONG RegKey::GetValueNameAt(int index, std::wstring* name) const {
   wchar_t buf[256];
   DWORD bufsize = arraysize(buf);
   LONG r = ::RegEnumValue(key_, index, buf, &bufsize, NULL, NULL, NULL, NULL);
   if (r == ERROR_SUCCESS)
     *name = buf;
 
   return r;
 }
 
 LONG RegKey::DeleteKey(const wchar_t* name) {
   DCHECK(key_);
   DCHECK(name);
-  LONG result = SHDeleteKey(key_, name);
-  return result;
+  HKEY subkey;
+
+  // Verify the key exists before attempting delete to replicate previous
+  // behavior.
+  LONG result =
+      RegOpenKeyEx(key_, name, 0, READ_CONTROL | wow64access_, &subkey);
+  if (result != ERROR_SUCCESS)
+    return result;
+  RegCloseKey(subkey);
+  std::wstring keyname(name);

[grt (UTC plus 2), 2014/05/17 12:54:33]

nit: put this inline with the recursive call:
  return RegDelRecurse(key_, std::wstring(name), wow64access_);

[Will Harris, 2014/05/19 21:49:03]

Done.

+
+  return RegDelRecurse(key_, keyname, wow64access_);
 }
 
 LONG RegKey::DeleteValue(const wchar_t* value_name) {
   DCHECK(key_);
   LONG result = RegDeleteValue(key_, value_name);
   return result;
 }
 
 LONG RegKey::ReadValueDW(const wchar_t* name, DWORD* out_value) const {
   DCHECK(out_value);
@@ skipping 152 matching lines @@
 LONG RegKey::StopWatching() {
   LONG result = ERROR_INVALID_HANDLE;
   if (watch_event_) {
     CloseHandle(watch_event_);
     watch_event_ = 0;
     result = ERROR_SUCCESS;
   }
   return result;
 }
 
+// static
+LONG RegKey::RegDeleteKeyExWrapper(HKEY hKey,
+                                   const wchar_t* lpSubKey,
+                                   REGSAM samDesired,
+                                   DWORD Reserved) {
+  typedef LSTATUS(WINAPI * RegDeleteKeyExPtr)(

[grt (UTC plus 2), 2014/05/17 12:54:33]

WINAPI*

[Will Harris, 2014/05/19 21:49:03]

this was git gl format :)

[grt (UTC plus 2), 2014/05/20 15:28:52]

Oh. How 'bout that. May as well do as it says, although it looks out of place to
me given the normal formatting. Feel free to go with git cl format and ignore
me.

+      HKEY hKey, LPCWSTR lpSubKey, REGSAM samDesired, DWORD Reserved);

[grt (UTC plus 2), 2014/05/17 12:54:33]

nit: remove the parameter names

[Will Harris, 2014/05/19 21:49:03]

Done.

+
+  RegDeleteKeyExPtr reg_delete_key_ex_func =
+      reinterpret_cast<RegDeleteKeyExPtr>(
+          GetProcAddress(GetModuleHandleA("advapi32.dll"), "RegDeleteKeyExW"));
+
+  if (reg_delete_key_ex_func)
+    return reg_delete_key_ex_func(hKey, lpSubKey, samDesired, Reserved);
+
+  // Windows XP does not support RegDeleteKeyEx, so fallback to RegDeleteKey.
+  return RegDeleteKey(hKey, lpSubKey);
+}
+
+// static
+LONG RegKey::RegDelRecurse(HKEY root_key,
+                           std::wstring const& name,

[grt (UTC plus 2), 2014/05/17 12:54:33]

const std::wstring&

[Will Harris, 2014/05/19 21:49:03]

Done.

+                           REGSAM access) {
+  LONG result;
+  HKEY target_key;
+
+  size_t name_length = name.length();

[grt (UTC plus 2), 2014/05/17 12:54:33]

remove this and change line 388 to if (name.empty())

[Will Harris, 2014/05/19 21:49:03]

Done.

+
+  if (name_length == 0) {
+    NOTREACHED();
+    return ERROR_INVALID_PARAMETER;
+  }
+
+  // First, see if the key can be deleted without having to recurse.
+  result = RegDeleteKeyExWrapper(root_key, name.c_str(), access, 0);

[grt (UTC plus 2), 2014/05/17 12:54:33]

the style guide says to define local vars "as close to the first use as
possible"
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Local_Variables),
so move the definition of result here:
  LONG result = ...

[Will Harris, 2014/05/19 21:49:03]

Done.

+
+  if (result == ERROR_SUCCESS)
+    return result;
+
+  result = RegOpenKeyEx(

[grt (UTC plus 2), 2014/05/17 12:54:33]

put the definition of target_key just above this line:
  HKEY target_key = NULL;

[Will Harris, 2014/05/19 21:49:03]

Done.

+      root_key, name.c_str(), 0, KEY_ENUMERATE_SUB_KEYS | access, &target_key);
+
+  if (result == ERROR_FILE_NOT_FOUND)
+    return ERROR_SUCCESS;
+  if (result != ERROR_SUCCESS)
+    return result;
+
+  // Copy string before modifying it.
+  std::wstring subkey_name(name);
+
+  // Check for an ending slash and add one if it is missing.
+  if (subkey_name[name_length - 1] != L'\\') {
+    name_length++;
+    subkey_name += L"\\";
+  }
+
+  // Enumerate the keys
+  result = ERROR_SUCCESS;
+  DWORD kMaxKeyNameLength = MAX_PATH;

[grt (UTC plus 2), 2014/05/17 12:54:33]

  const DWORD

[Will Harris, 2014/05/19 21:49:03]

Done.

+

[grt (UTC plus 2), 2014/05/17 12:54:33]

define this here:
  const size_t base_key_lenght = subkey_name.length();

[Will Harris, 2014/05/19 21:49:03]

Done.

+  while (result == ERROR_SUCCESS) {
+    std::wstring key_name;

[grt (UTC plus 2), 2014/05/17 12:54:33]

move the definition of key_name out of the loop.

[Will Harris, 2014/05/19 21:49:03]

Done.

+    DWORD key_size = kMaxKeyNameLength;
+    result = RegEnumKeyEx(target_key,
+                          0,
+                          WriteInto(&key_name, kMaxKeyNameLength),
+                          &key_size,
+                          NULL,
+                          NULL,
+                          NULL,
+                          NULL);
+
+    if (result != ERROR_SUCCESS)
+      break;
+
+    key_name.resize(key_size);
+    subkey_name.resize(name_length);

[grt (UTC plus 2), 2014/05/17 12:54:33]

name_length -> base_key_lenght

[Will Harris, 2014/05/19 21:49:03]

Done.

+    subkey_name += key_name;
+
+    if (RegDelRecurse(root_key, subkey_name, access) != ERROR_SUCCESS)
+      break;
+  }
+
+  RegCloseKey(target_key);
+
+  // Try again to delete the key.
+  result = RegDeleteKeyExWrapper(root_key, name.c_str(), access, 0);
+
+  return result;
+}
+
 // RegistryValueIterator ------------------------------------------------------
 
 RegistryValueIterator::RegistryValueIterator(HKEY root_key,
                                              const wchar_t* folder_key)
     : name_(MAX_PATH, L'\0'),
       value_(MAX_PATH, L'\0') {
   LONG result = RegOpenKeyEx(root_key, folder_key, 0, KEY_READ, &key_);
   if (result != ERROR_SUCCESS) {
     key_ = NULL;
   } else {
@@ skipping 132 matching lines @@
     if (ERROR_SUCCESS == r)
       return true;
   }
 
   name_[0] = '\0';
   return false;
 }
 
 }  // namespace win
 }  // namespace base