Use a better registration suffix that will always be unique while respecting the MSDN rules (2nd tr…

chrome/installer/util/shell_util.cc

Index: chrome/installer/util/shell_util.cc
diff --git a/chrome/installer/util/shell_util.cc b/chrome/installer/util/shell_util.cc
index dc084360b859861d5a43b0032382e1ae7ebd3aab..f8d7225116d9f04d9a159749bd09f83a9ae6de8e 100644
--- a/chrome/installer/util/shell_util.cc
+++ b/chrome/installer/util/shell_util.cc
@@ -12,12 +12,15 @@
 #include <windows.h>
 #include <shlobj.h>
 
+#include <limits>
 #include <list>
 
 #include "base/command_line.h"
 #include "base/file_path.h"
 #include "base/file_util.h"
+#include "base/lazy_instance.h"
 #include "base/logging.h"
+#include "base/md5.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/path_service.h"
 #include "base/stl_util.h"
@@ -28,6 +31,7 @@
 #include "base/values.h"
 #include "base/win/registry.h"
 #include "base/win/scoped_comptr.h"
+#include "base/win/win_util.h"
 #include "base/win/windows_version.h"
 #include "chrome/common/chrome_constants.h"
 #include "chrome/common/chrome_switches.h"
@@ -85,6 +89,75 @@ bool IsChromeMetroSupported() {
   return VerifyVersionInfo(&min_version_info, type_mask, condition_mask) != 0;
 }
 
+// Returns the current (or installed) browser's ProgId (e.g.
+// "ChromeHTML|suffix|").
+// |suffix| can be the empty string.
+string16 GetBrowserProgId(const string16& suffix) {
+  string16 chrome_html(ShellUtil::kChromeHTMLProgId);
+  chrome_html.append(suffix);
+
+  // ProgIds cannot be longer than 39 characters.
+  // Ref: http://msdn.microsoft.com/en-us/library/aa911706.aspx.
+  // Make all new registrations comply with this requirement (existing
+  // registrations must be preserved).
+  string16 new_style_suffix;
+  if (ShellUtil::GetUserSpecificRegistrySuffix(&new_style_suffix) &&
+      suffix == new_style_suffix && chrome_html.length() > 39) {
+    NOTREACHED();
+    chrome_html.erase(39);
+  }
+  return chrome_html;
+}
+
+// This class is used to initialize and cache a base 32 encoding of the md5 hash
+// of this user's username preceded by a dot.
+// This is guaranteed to be unique on the machine and 27 characters long
+// (including the '.').
+// This is then meant to be used as a suffix on all registrations that may
+// conflict with another user-level Chrome install.
+class UserSpecificRegistrySuffix {
+ public:
+  UserSpecificRegistrySuffix() {

[grt (UTC plus 2), 2012/07/05 20:00:19]

style dictates not inlining like this even though it's convenient
(http://www.chromium.org/developers/coding-style#TOC-Inline-functions)

[gab, 2012/07/05 21:19:53]

Oh interesting, I didn't know the compiler interpreted this differently (i.e.
automatically inlined it when declared this way).

More C++ knowledge woohoo!

+    // Do all the initialization in the constructor to be able to build the
+    // suffix in a thread-safe manner when used in conjunction with a
+    // LazyInstance.
+    wchar_t user_name[256];
+    DWORD size = arraysize(user_name);
+    if (::GetUserName(user_name, &size) == 0 || size < 1) {
+      NOTREACHED();
+      return;
+    }
+    COMPILE_ASSERT(sizeof(base::MD5Digest) == 16, size_of_MD5_not_as_expected_);
+    base::MD5Digest md5_digest;
+    base::MD5Sum(user_name, size, &md5_digest);

[grt (UTC plus 2), 2012/07/05 20:00:19]

this includes the string terminator in the hash.  please either switch to size-1
or mention in the comment above that the terminator is included.

[gab, 2012/07/05 21:19:53]

Changed to sid string.

+    const string16 base32_md5(
+        ShellUtil::ByteArrayToBase32(md5_digest.a, arraysize(md5_digest.a)));
+    // The value returned by the base32 algorithm above must never change and
+    // must always be 26 characters long (i.e. if someone ever moves this to
+    // base and implements the full base32 algorithm (i.e. with appended '='
+    // signs in the output), they must provide a flag to allow this method to
+    // still request the output with no appended '=' signs).
+    DCHECK_EQ(base32_md5.length(), 26U);
+    suffix_.reserve(base32_md5.length() + 1);
+    suffix_.assign(1, L'.');
+    suffix_.append(base32_md5);
+  }
+
+  bool GetSuffix(string16* suffix) {
+    if (suffix_.empty()) {
+      NOTREACHED();
+      return false;
+    }
+    suffix->assign(suffix_);
+    return true;
+  }
+
+ private:
+  string16 suffix_;
+
+  DISALLOW_COPY_AND_ASSIGN(UserSpecificRegistrySuffix);
+};  // class UserSpecificRegistrySuffix
+
 // This class represents a single registry entry. The objective is to
 // encapsulate all the registry entries required for registering Chrome at one
 // place. This class can not be instantiated outside the class and the objects
@@ -181,8 +254,7 @@ class RegistryEntry {
     // File association ProgId
     string16 chrome_html_prog_id(ShellUtil::kRegClasses);
     chrome_html_prog_id.push_back(FilePath::kSeparators[0]);
-    chrome_html_prog_id.append(ShellUtil::kChromeHTMLProgId);
-    chrome_html_prog_id.append(suffix);
+    chrome_html_prog_id.append(GetBrowserProgId(suffix));
     entries->push_front(new RegistryEntry(
         chrome_html_prog_id, ShellUtil::kChromeHTMLProgIdDesc));
     entries->push_front(new RegistryEntry(
@@ -234,7 +306,7 @@ class RegistryEntry {
                                            std::list<RegistryEntry*>* entries) {
     entries->push_front(new RegistryEntry(
         GetCapabilitiesKey(dist, suffix).append(L"\\URLAssociations"),
-        protocol, string16(ShellUtil::kChromeHTMLProgId).append(suffix)));
+        protocol, GetBrowserProgId(suffix)));
     return true;
   }
 
@@ -295,8 +367,7 @@ class RegistryEntry {
     entries->push_front(new RegistryEntry(capabilities + L"\\Startmenu",
         L"StartMenuInternet", reg_app_name));
 
-    string16 html_prog_id(ShellUtil::kChromeHTMLProgId);
-    html_prog_id.append(suffix);
+    string16 html_prog_id(GetBrowserProgId(suffix));
     for (int i = 0; ShellUtil::kFileAssociations[i] != NULL; i++) {
       entries->push_front(new RegistryEntry(
           capabilities + L"\\FileAssociations",
@@ -365,8 +436,7 @@ class RegistryEntry {
                              const string16& suffix,
                              std::list<RegistryEntry*>* entries) {
     // File extension associations.
-    string16 html_prog_id(ShellUtil::kChromeHTMLProgId);
-    html_prog_id.append(suffix);
+    string16 html_prog_id(GetBrowserProgId(suffix));
     for (int i = 0; ShellUtil::kFileAssociations[i] != NULL; i++) {
       string16 ext_key(ShellUtil::kRegClasses);
       ext_key.push_back(FilePath::kSeparators[0]);
@@ -677,8 +747,7 @@ void RemoveBadWindows8RegistrationIfNeeded(
     // <root hkey>\Software\Classes\ChromiumHTML[.user]\shell\open\command
     key = ShellUtil::kRegClasses;
     key.push_back(FilePath::kSeparators[0]);
-    key.append(ShellUtil::kChromeHTMLProgId);
-    key.append(installation_suffix);
+    key.append(GetBrowserProgId(installation_suffix));
     key.append(ShellUtil::kRegShellOpen);
     InstallUtil::DeleteRegistryValue(root_key, key,
                                      ShellUtil::kRegDelegateExecute);
@@ -747,28 +816,13 @@ bool QuickIsChromeRegistered(BrowserDistribution* dist,
   return false;
 }
 
-// Sets |suffix| to this user's username preceded by a dot. This suffix is then
-// meant to be added to all registration that may conflict with another
-// user-level Chrome install.
-// Returns true unless the OS call to retrieve the username fails.
-bool GetUserSpecificRegistrySuffix(string16* suffix) {
-  wchar_t user_name[256];
-  DWORD size = arraysize(user_name);
-  if (::GetUserName(user_name, &size) == 0 || size < 1) {
-    PLOG(DFATAL) << "GetUserName failed";
-    return false;
-  }
-  suffix->reserve(size);
-  suffix->assign(1, L'.');
-  suffix->append(user_name, size - 1);
-  return true;
-}
-
-// Sets |suffix| to the current user's username, preceded by a dot, on
-// user-level installs.
-// To support old-style user-level installs however, |suffix| is cleared if
-// the user currently owns the non-suffixed HKLM registrations.
-// |suffix| is also cleared on system-level installs.
+// Sets |suffix| to a 27 character string that is specific to this user on this
+// machine (on user-level installs only).
+// To support old-style user-level installs however, |suffix| is cleared if the
+// user currently owns the non-suffixed HKLM registrations.
+// |suffix| can also be set to the user's username if the current install is
+// suffixed as per the old-style registrations.
+// |suffix| is cleared on system-level installs.
 // |suffix| should then be appended to all Chrome properties that may conflict
 // with other Chrome user-level installs.
 // Returns true unless one of the underlying calls fails.
@@ -782,9 +836,20 @@ bool GetInstallationSpecificSuffix(BrowserDistribution* dist,
     // registered with no suffix.
     suffix->clear();
     return true;
-  } else {
-    return GetUserSpecificRegistrySuffix(suffix);
   }
+
+  // Get the old suffix for the check below.
+  if (!ShellUtil::GetOldUserSpecificRegistrySuffix(suffix)) {
+    NOTREACHED();
+    return false;
+  }
+  if (QuickIsChromeRegistered(dist, chrome_exe, *suffix,
+                              CONFIRM_SHELL_REGISTRATION)) {
+    // Username suffix for installs that are suffixed as per the old-style.
+    return true;
+  }
+
+  return ShellUtil::GetUserSpecificRegistrySuffix(suffix);
 }
 
 // Returns the root registry key (HKLM or HKCU) into which shell integration
@@ -1060,12 +1125,29 @@ void ShellUtil::GetRegisteredBrowsers(
 
 string16 ShellUtil::GetCurrentInstallationSuffix(BrowserDistribution* dist,
                                                  const string16& chrome_exe) {
+  // This method is somewhat the opposite of GetInstallationSpecificSuffix().
+  // In this case we are not trying to determine the current suffix for the
+  // upcoming installation (i.e. not trying to stick to a currently bad
+  // registration style if one is present).
+  // Here we want to determine which suffix we should use at run-time.
+  // In order of preference, we prefer (for user-level installs):
+  //   1) Base 32 encoding of the md5 hash of the username (new-style).
+  //   2) Username (old-style).
+  //   3) Unsuffixed (even worse).
   string16 tested_suffix;
-  if (!InstallUtil::IsPerUserInstall(chrome_exe.c_str()) ||
-      !GetUserSpecificRegistrySuffix(&tested_suffix) ||
-      !QuickIsChromeRegistered(dist, chrome_exe, tested_suffix,
+  if (InstallUtil::IsPerUserInstall(chrome_exe.c_str()) &&
+      (!GetUserSpecificRegistrySuffix(&tested_suffix) ||
+       !QuickIsChromeRegistered(dist, chrome_exe, tested_suffix,
+                                CONFIRM_PROGID_REGISTRATION)) &&
+      (!GetOldUserSpecificRegistrySuffix(&tested_suffix) ||
+       !QuickIsChromeRegistered(dist, chrome_exe, tested_suffix,
+                                CONFIRM_PROGID_REGISTRATION)) &&
+      !QuickIsChromeRegistered(dist, chrome_exe, tested_suffix.erase(),
                                CONFIRM_PROGID_REGISTRATION)) {
-    return string16();
+    // If Chrome is not registered under any of the possible suffixes (e.g.
+    // tests, Canary, etc.): use the new-style suffix at run-time.
+    if (!GetUserSpecificRegistrySuffix(&tested_suffix))
+      NOTREACHED();
   }
   return tested_suffix;
 }
@@ -1521,3 +1603,71 @@ bool ShellUtil::UpdateChromeShortcut(BrowserDistribution* dist,
       app_id.c_str(),
       ConvertShellUtilShortcutOptionsToFileUtil(options));
 }
+
+bool ShellUtil::GetUserSpecificRegistrySuffix(string16* suffix) {
+  // Use a thread-safe cache for the user's suffix.
+  static base::LazyInstance<UserSpecificRegistrySuffix>::Leaky suffix_instance =
+      LAZY_INSTANCE_INITIALIZER;
+  return suffix_instance.Get().GetSuffix(suffix);
+}
+
+bool ShellUtil::GetOldUserSpecificRegistrySuffix(string16* suffix) {

[gab, 2012/07/03 22:36:07]

I thought of also caching this one in the UserSpecificRegistrySuffix class, but
it would require that the LazyInstance be in the unnamed namespace (which
probably means it always exists as opposed to only be created on the first call
with a local static?) (or have 2 of them, but I like that even less).

Let me know what you think.

[grt (UTC plus 2), 2012/07/05 20:00:19]

It could be done with a single class/instance owned by a private helper function
in the unnamed namespace that returns the lazy-initialized value for use by the
two Getters.
I think it's fine to this to stay as-is.  I don't know that GetUserName is
particularly heavy-weight.

[gab, 2012/07/05 21:19:53]

Right, GetUserName is probably very low-height (and potentially even kept in
memory by the OS). Plus now that I use the SID, the old one is no longer and
intermediate state of the new one...

+  wchar_t user_name[256];
+  DWORD size = arraysize(user_name);
+  if (::GetUserName(user_name, &size) == 0 || size < 1) {
+    NOTREACHED();
+    return false;
+  }
+  suffix->reserve(size);
+  suffix->assign(1, L'.');
+  suffix->append(user_name, size - 1);
+  return true;
+}
+
+string16 ShellUtil::ByteArrayToBase32(const uint8* bytes, size_t size) {
+  static const char kEncoding[] = "ABCDEFGHIJKLMNOPQRSTUVWXYZ234567";
+
+  // Eliminate special cases first.
+  if (size == 0) {
+    return string16();
+  } else if (size == 1) {
+    string16 ret;
+    ret.push_back(kEncoding[(bytes[0] & 0xf8) >> 3]);
+    ret.push_back(kEncoding[(bytes[0] & 0x07) << 2]);
+    return ret;
+  } else if (size >= std::numeric_limits<size_t>::max() / 8) {
+    // If |size| is too big, the calculation of |encoded_length| below will
+    // overflow.
+    NOTREACHED();
+    return string16();
+  }
+
+  // Overestimate the number of bits in the string by 4 so that dividing by 5
+  // is the equivalent of rounding up the actual number of bits divided by 5.
+  const size_t encoded_length = (size * 8 + 4) / 5;
+
+  string16 ret;
+  ret.reserve(encoded_length);
+
+  // A bit stream which will be read from the left and appended to from the
+  // right as it's emptied.
+  uint16 bit_stream = (bytes[0] << 8) + bytes[1];
+  size_t next_byte_index = 2;
+  int free_bits = 0;
+  while (free_bits < 16) {
+    // Extract the 5 leftmost bits in the stream
+    ret.push_back(kEncoding[(bit_stream & 0xf800) >> 11]);
+    bit_stream <<= 5;
+    free_bits += 5;
+
+    // If there is enough room in the bit stream, inject another byte (if there
+    // are any left...).
+    if (free_bits >= 8 && next_byte_index < size) {
+      free_bits -= 8;
+      bit_stream += bytes[next_byte_index++] << free_bits;
+    }
+  }
+
+  DCHECK_EQ(ret.length(), encoded_length);
+  return ret;
+}