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

chrome/installer/util/shell_util.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.
 //
 // This file defines functions that integrate Chrome in Windows shell. These
 // functions can be used by Chrome as well as Chrome installer. All of the
 // work is done by the local functions defined in anonymous namespace in
 // this class.
 
 #include "chrome/installer/util/shell_util.h"
@@ skipping 10 matching lines @@
 #include "base/memory/scoped_ptr.h"
 #include "base/path_service.h"
 #include "base/stl_util.h"
 #include "base/string_number_conversions.h"
 #include "base/string_split.h"
 #include "base/string_util.h"
 #include "base/utf_string_conversions.h"
 #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"
 #include "chrome/installer/util/browser_distribution.h"
 #include "chrome/installer/util/install_util.h"
 #include "chrome/installer/util/master_preferences.h"
 #include "chrome/installer/util/master_preferences_constants.h"
 
 using base::win::RegKey;
 
@@ skipping 37 matching lines @@
   VER_SET_CONDITION(condition_mask, VER_BUILDNUMBER, VER_GREATER_EQUAL);
   VER_SET_CONDITION(condition_mask, VER_SERVICEPACKMAJOR, VER_GREATER_EQUAL);
   VER_SET_CONDITION(condition_mask, VER_SERVICEPACKMINOR, VER_GREATER_EQUAL);
 
   DWORD type_mask = VER_MAJORVERSION | VER_MINORVERSION | VER_BUILDNUMBER |
       VER_SERVICEPACKMAJOR | VER_SERVICEPACKMINOR;
 
   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).
+  // Note: since |new_style_suffix| will always be the same for this user, only
+  // initialize it once, re-use the derived value in subsequent calls.
+  static string16 new_style_suffix;

[grt (UTC plus 2), 2012/06/27 15:47:32]

this isn't threadsafe and it adds an exit-time dtor.  if you don't care about
the former, use CR_DEFINE_STATIC_LOCAL.  otherwise, use a leaky lazy instance.

why not do the caching at the level of GetNewUserSpecificRegistrySuffix?  i
don't see why this caller alone should benefit from not recomputing it.  you
could do something clean for the suffix that would allow it to be cached and for
the cache to be overridden in tests if that was helpful.

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

Done.

+  if ((!new_style_suffix.empty() ||
+       ShellUtil::GetNewUserSpecificRegistrySuffix(&new_style_suffix)) &&
+      suffix.compare(new_style_suffix) == 0 && chrome_html.length() > 39) {
+    NOTREACHED();
+    chrome_html.erase(39);
+  }
+  return chrome_html;
+}
+
 // 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
 // of this class type can be obtained only by calling a static method of this
 // class.
 class RegistryEntry {
  public:
   // A bit-field enum of places to look for this key in the Windows registry.
   enum LookForIn {
     LOOK_IN_HKCU = 1 << 0,
@@ skipping 76 matching lines @@
         // <root hkey>\Software\Classes\<app_id>\.exe\shell\<verb>\command
         entries->push_front(new RegistryEntry(sub_path, delegate_command));
         entries->push_front(new RegistryEntry(
             sub_path, ShellUtil::kRegDelegateExecute, delegate_guid));
       }
     }
 
     // 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(
         chrome_html_prog_id, ShellUtil::kRegUrlProtocol, L""));
     entries->push_front(new RegistryEntry(
         chrome_html_prog_id + ShellUtil::kRegDefaultIcon, icon_path));
     entries->push_front(new RegistryEntry(
         chrome_html_prog_id + ShellUtil::kRegShellOpen, open_cmd));
     if (set_delegate_execute) {
       entries->push_front(new RegistryEntry(
@@ skipping 31 matching lines @@
   }
 
   // This method returns a list of the registry entries needed to declare a
   // capability of handling a protocol on Windows.
   static bool GetProtocolCapabilityEntries(BrowserDistribution* dist,
                                            const string16& suffix,
                                            const string16& protocol,
                                            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;
   }
 
   // This method returns a list of all the registry entries required to fully
   // integrate Chrome with Windows (i.e. StartMenuInternet, Default Programs,
   // AppPaths, etc.). This entries need to be registered in HKLM prior to Win8.
   static bool GetShellIntegrationEntries(BrowserDistribution* dist,
                                          const string16& chrome_exe,
                                          const string16& suffix,
                                          std::list<RegistryEntry*>* entries) {
@@ skipping 40 matching lines @@
         dist->GetLongAppDescription()));
     entries->push_front(new RegistryEntry(
         capabilities, ShellUtil::kRegApplicationIcon, icon_path));
     entries->push_front(new RegistryEntry(
         capabilities, ShellUtil::kRegApplicationName,
         dist->GetAppShortCutName()));
 
     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",
           ShellUtil::kFileAssociations[i], html_prog_id));
     }
     for (int i = 0; ShellUtil::kPotentialProtocolAssociations[i] != NULL;
         i++) {
       entries->push_front(new RegistryEntry(
           capabilities + L"\\URLAssociations",
           ShellUtil::kPotentialProtocolAssociations[i], html_prog_id));
@@ skipping 48 matching lines @@
     return true;
   }
 
   // This method returns a list of all the user level registry entries that
   // are needed to make Chromium default browser.
   static bool GetUserEntries(BrowserDistribution* dist,
                              const string16& chrome_exe,
                              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]);
       ext_key.append(ShellUtil::kFileAssociations[i]);
       entries->push_front(new RegistryEntry(ext_key, html_prog_id));
     }
 
     // Protocols associations.
     string16 chrome_open = ShellUtil::GetChromeShellOpenCmd(chrome_exe);
     string16 chrome_icon = ShellUtil::GetChromeIcon(dist, chrome_exe);
@@ skipping 290 matching lines @@
 
     // <root hkey>\Software\Classes\<app_id>
     string16 key(ShellUtil::kRegClasses);
     key.push_back(FilePath::kSeparators[0]);
     key.append(app_id);
     InstallUtil::DeleteRegistryKey(root_key, key);
 
     // <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);
   }
 }
 
 // Returns true if the current install's |chrome_exe| has been registered with
 // |suffix|.
 // |confirmation_level| is the level of verification desired as described in
 // the RegistrationConfirmationLevel enum above.
@@ skipping 48 matching lines @@
   // Assert that |reg_key| points to |chrome_exe| in HKLM.
   const RegKey key_hklm(HKEY_LOCAL_MACHINE, reg_key.c_str(), KEY_QUERY_VALUE);
   string16 hklm_value;
   if (key_hklm.ReadValue(L"", &hklm_value) == ERROR_SUCCESS) {
     return InstallUtil::ProgramCompare(
         FilePath(chrome_exe)).Evaluate(hklm_value);
   }
   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.
+// Sets |suffix| to a 27 characters string that is specific to this user on

[grt (UTC plus 2), 2012/06/27 15:47:32]

characters -> character

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

Done.

+// this machine, on user-level installs, preferably.

[grt (UTC plus 2), 2012/06/27 15:47:32]

i don't understand "on user-level installs, preferably."  what does that mean?

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

I meant that this is not the only outcome, but the one we "prefer". I agree it's
confusing, removed as the rest of the comment already details this out.

 // 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.
+// |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.
 bool GetInstallationSpecificSuffix(BrowserDistribution* dist,
                                    const string16& chrome_exe,
                                    string16* suffix) {
   if (!InstallUtil::IsPerUserInstall(chrome_exe.c_str()) ||
       QuickIsChromeRegistered(dist, chrome_exe, string16(),
                               CONFIRM_SHELL_REGISTRATION)) {
     // No suffix on system-level installs and user-level installs already
     // registered with no suffix.
     suffix->clear();
     return true;
-  } else {
-    return GetUserSpecificRegistrySuffix(suffix);
   }
+
+  string16 old_suffix;

[grt (UTC plus 2), 2012/06/27 15:47:32]

why not use the caller-provided string here rather than introducing a new one?

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

Done.

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

Done.

+  // Get the |old_suffix| for the check below.
+  if (!ShellUtil::GetOldUserSpecificRegistrySuffix(&old_suffix)) {
+    NOTREACHED();
+    return false;
+  }
+  if (QuickIsChromeRegistered(dist, chrome_exe, old_suffix,
+                              CONFIRM_SHELL_REGISTRATION)) {
+    // Username suffix for installs that are suffixed as per the old-style.
+    suffix->assign(old_suffix);
+    return true;
+  }
+
+  return ShellUtil::GetNewUserSpecificRegistrySuffix(suffix);
 }
 
 // Returns the root registry key (HKLM or HKCU) into which shell integration
 // registration for default protocols must be placed. As of Windows 8 everything
 // can go in HKCU for per-user installs.
 HKEY DetermineShellIntegrationRoot(bool is_per_user) {
   return is_per_user && base::win::GetVersion() >= base::win::VERSION_WIN8 ?
       HKEY_CURRENT_USER : HKEY_LOCAL_MACHINE;
 }
 
@@ skipping 255 matching lines @@
           key.ReadValue(kReinstallCommand, &command) == ERROR_SUCCESS &&
           !command.empty()) {
         (*browsers)[name] = command;
       }
     }
   }
 }
 
 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,
-                               CONFIRM_PROGID_REGISTRATION)) {
-    return string16();
+  if (InstallUtil::IsPerUserInstall(chrome_exe.c_str()) &&
+      (!GetNewUserSpecificRegistrySuffix(&tested_suffix) ||
+       !QuickIsChromeRegistered(dist, chrome_exe, tested_suffix,
+                                CONFIRM_PROGID_REGISTRATION)) &&
+      (!GetOldUserSpecificRegistrySuffix(&tested_suffix) ||
+       !QuickIsChromeRegistered(dist, chrome_exe, tested_suffix,
+                                CONFIRM_PROGID_REGISTRATION)) &&
+      (tested_suffix.clear(),

[grt (UTC plus 2), 2012/06/27 15:47:32]

ugh, comma.  how about just:

!QuickIsChromeRegistered(dist, chrome_exe, tested_suffix.erase(),
CONFIRM_PROGID_REGISTRATION)

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

Oh nice, erase()! I had looked at .clear(), but it returns void() :(... Thought
I was clever with my comma operator, but that's even better :)!

+       !QuickIsChromeRegistered(dist, chrome_exe, tested_suffix,
+                                CONFIRM_PROGID_REGISTRATION))) {
+    // 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 (!GetNewUserSpecificRegistrySuffix(&tested_suffix))
+      NOTREACHED();
   }
   return tested_suffix;
 }
 
 string16 ShellUtil::GetApplicationName(BrowserDistribution* dist,
                                        const string16& chrome_exe) {
   string16 app_name = dist->GetBaseAppName();
   app_name += GetCurrentInstallationSuffix(dist, chrome_exe);
   return app_name;
 }
 
 string16 ShellUtil::GetBrowserModelId(BrowserDistribution* dist,
                                       const string16& chrome_exe) {
   string16 app_id(dist->GetBaseAppId());
   string16 suffix;
   if (InstallUtil::IsPerUserInstall(chrome_exe.c_str()) &&
-      !GetUserSpecificRegistrySuffix(&suffix)) {
+      !GetNewUserSpecificRegistrySuffix(&suffix)) {
     NOTREACHED();
   }
   // There is only one component (i.e. the suffixed appid) in this case, but it
   // is still necessary to go through the appid constructor to make sure the
   // returned appid is truncated if necessary.
   std::vector<string16> components(1, app_id.append(suffix));
   return BuildAppModelId(components);
 }
 
 string16 ShellUtil::BuildAppModelId(
@@ skipping 418 matching lines @@
       chrome_exe.c_str(),
       shortcut.c_str(),
       chrome_path.value().c_str(),
       arguments.c_str(),
       description.c_str(),
       icon_path.c_str(),
       icon_index,
       app_id.c_str(),
       ConvertShellUtilShortcutOptionsToFileUtil(options));
 }
+
+// Sets |suffix| to the 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 suffix is then meant to be added to all registration that may conflict
+// with another user-level Chrome install.
+// Note that prior to Chrome 21, the suffix registered used to be the user's
+// username (see GetOldUserSpecificRegistrySuffix() below). We still honor old
+// installs registered that way, but it was wrong because some of the characters
+// allowed in a username are not allowed in a ProgId.
+// Returns true unless the OS call to retrieve the username fails.
+bool ShellUtil::GetNewUserSpecificRegistrySuffix(string16* suffix) {

[grt (UTC plus 2), 2012/06/27 15:47:32]

I'm not in love with "New" and "Old" in the method names.  On the plus side:
it's a bit easier for a caller to know which one to use.  On the minus side:
it's opaque.  What do you think about at least removing "New" from this one?

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

Done.

+  wchar_t user_name[256];
+  DWORD size = arraysize(user_name);
+  if (::GetUserName(user_name, &size) == 0 || size < 1) {
+    NOTREACHED();
+    return false;
+  }
+  base::MD5Digest md5_digest;
+  base::MD5Sum(user_name, size, &md5_digest);
+  const string16 base32_md5(ShellUtil::MD5DigestToBase32(md5_digest));
+  // 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(27);

[grt (UTC plus 2), 2012/06/27 15:47:32]

27 -> base32_md5.size() + 1

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

Done.

+  suffix->assign(1, L'.');
+  suffix->append(base32_md5);
+  return true;
+}
+
+// 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

[grt (UTC plus 2), 2012/06/27 15:47:32]

this comment is incorrect, no?  this suffix shouldn't be used to disambiguate,
it should only be used for "legacy" installs.

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

Done.

+// user-level Chrome install.
+// Returns true unless the OS call to retrieve the username fails.
+bool ShellUtil::GetOldUserSpecificRegistrySuffix(string16* suffix) {
+  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::MD5DigestToBase32(const base::MD5Digest& digest) {

[grt (UTC plus 2), 2012/06/27 15:47:32]

why is this function needed?  GetNewUserSpecificRegistrySuffix does its DCHECK,
so i don't see the added value of it.  removing it gets rid of the #include
"base/md5.h" from the header, which would be a good thing IMO.

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

Good point, moved the very little bit logic remaining in this function to
GetUserSpecificRegistrySuffix().

+  COMPILE_ASSERT(sizeof(base::MD5Digest) == 16, size_of_MD5_not_as_expected_);
+  string16 encoding(ByteArrayToBase32(digest.a, 16));

[grt (UTC plus 2), 2012/06/27 15:47:32]

16 -> arraysize(digest.a)

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

Done.

+  // This must always remain 26 characters as callers which write to solid
+  // state depend on this.
+  DCHECK_EQ(encoding.length(), 26U);
+  return encoding;
+}
+
+string16 ShellUtil::ByteArrayToBase32(const unsigned char bytes[], int size) {

[grt (UTC plus 2), 2012/06/27 15:47:32]

const unsigned char bytes[] -> const uint8* bytes
style guide says to use size_t for "a count of objects or bytes in memory",
which describes |size|.

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

I thought style-guide also said to avoid using size_t, but ok.

Done.

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

it says to avoid using unsigned types just for the sake of representing values
that shouldn't be negative (prefer int), but do use size_t for things that truly
are sizes or counts of bytes or elements in a container.

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

Ah ok got it, thanks for clarifying :)!

+  static char const kEncoding[] = "ABCDEFGHIJKLMNOPQRSTUVWXYZ234567";

[grt (UTC plus 2), 2012/06/27 15:47:32]

char const -> const char

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

Done.

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

Done.

+
+  // 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;
+  }

[grt (UTC plus 2), 2012/06/27 15:47:32]

else if (size >= std::numeric_limits<size_t>::max() / 8) {
  // error: the calculation of encoded_length would overflow.
}

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

Done.

+
+  // 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 int kEncodedLength = (size * 8 + 4) / 5;

[grt (UTC plus 2), 2012/06/27 15:47:32]

kEncodedLength -> encoded_length (it isn't a compile-time constant)

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

Done.

+
+  string16 ret;
+  ret.reserve(kEncodedLength);
+
+  // 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];
+  int 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(), static_cast<size_t>(kEncodedLength));
+  return ret;
+}