Make Crashpad use the user data dir, rather than always default location

chrome/install_static/install_util.cc

 // Copyright 2014 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 "chrome/install_static/install_util.h"
 
 #include <windows.h>
 #include <assert.h>
 #include <algorithm>
 #include <iostream>
 #include <iterator>
 #include <memory>
 #include <sstream>
 
+#include "chrome/install_static/policy_path_parser.h"
 #include "chrome_elf/nt_registry/nt_registry.h"
 
 namespace install_static {
 
 ProcessType g_process_type = ProcessType::UNINITIALIZED;
 
 // TODO(ananta)
 // http://crbug.com/604923
 // The constants defined in this file are also defined in chrome/installer and
 // other places. we need to unify them.
@@ skipping 29 matching lines @@
 const wchar_t kAppGuidGoogleChrome[] =
     L"{8A69D345-D564-463c-AFF1-A69D9E530F96}";
 const wchar_t kAppGuidGoogleBinaries[] =
     L"{4DC8B4CA-1BDA-483e-B5FA-D3C12E15B62D}";
 
 const wchar_t kHeadless[] = L"CHROME_HEADLESS";
 const wchar_t kShowRestart[] = L"CHROME_CRASHED";
 const wchar_t kRestartInfo[] = L"CHROME_RESTART";
 const wchar_t kRtlLocale[] = L"RIGHT_TO_LEFT";
 
-const char kGpuProcess[] = "gpu-process";
-const char kPpapiPluginProcess[] = "ppapi";
-const char kRendererProcess[] = "renderer";
-const char kUtilityProcess[] = "utility";
-const char kProcessType[] = "type";
-const char kCrashpadHandler[] = "crashpad-handler";
+const wchar_t kCrashpadHandler[] = L"crashpad-handler";
+const wchar_t kProcessType[] = L"type";
+const wchar_t kUserDataDirSwitch[] = L"user-data-dir";
+const wchar_t kUtilityProcess[] = L"utility";
+
+#if defined(GOOGLE_CHROME_BUILD)
+const wchar_t kRegistryChromePolicyKey[] =
+    L"SOFTWARE\\Policies\\Google\\Chrome";
+#else
+const wchar_t kRegistryChromePolicyKey[] =
+    L"SOFTWARE\\Policies\\Chromium";
+#endif
+const wchar_t kUserDataDirRegKey[] = L"UserDataDir";
 
 namespace {
 
 // TODO(ananta)
 // http://crbug.com/604923
 // These constants are defined in the chrome/installer directory as well. We
 // need to unify them.
 #if defined(GOOGLE_CHROME_BUILD)
 const wchar_t kSxSSuffix[] = L" SxS";
 const wchar_t kGoogleChromeInstallSubDir1[] = L"Google";
@@ skipping 289 matching lines @@
   std::vector<StringType> tokens;
   std::basic_istringstream<typename StringType::value_type> buffer(str);
   for (StringType token; std::getline(buffer, token, delimiter);) {
     if (trim_spaces)
       TrimT<StringType>(&token);
     tokens.push_back(token);
   }
   return tokens;
 }
 
+// Checks if the key exists in the given hive and expands any string variables.
+bool LoadUserDataDirPolicyFromRegistry(nt::ROOT_KEY hive,
+                                       const wchar_t* key_name,
+                                       std::wstring* dir) {
+  std::wstring value;
+  bool result = false;
+  HANDLE key;
+  if (nt::OpenRegKey(hive, kRegistryChromePolicyKey, KEY_READ, &key, nullptr)) {
+    if (nt::QueryRegValueSZ(key, key_name, &value)) {
+      *dir = install_static::ExpandPathVariables(value);
+      result = true;
+    }
+    nt::CloseRegKey(key);

[Mark Mentovai, 2016/11/10 22:06:17]

I’m surprised that OpenRegKey doesn’t return a key and provide a ScopedRegKey to
deal with the close. Looks like that would improve every single call site
including this one.

[scottmg, 2016/11/15 23:23:15]

Agreed. Filed https://bugs.chromium.org/p/chromium/issues/detail?id=665635 for a
followup.

+  }
+  return result;
+}
+
+void CheckUserDataDirPolicy(std::wstring* user_data_dir) {
+  assert(user_data_dir);
+  // Policy from the HKLM hive has precedence over HKCU.
+  if (!LoadUserDataDirPolicyFromRegistry(nt::HKLM, kUserDataDirRegKey,
+                                         user_data_dir)) {
+    LoadUserDataDirPolicyFromRegistry(nt::HKCU, kUserDataDirRegKey,
+                                      user_data_dir);
+  }
+}
+
 }  // namespace
 
 bool IsSxSChrome(const wchar_t* exe_path) {
   return ::wcsstr(exe_path, L"Chrome SxS\\Application") != nullptr;
 }
 
 bool IsSystemInstall(const wchar_t* exe_path) {
   wchar_t program_dir[MAX_PATH] = {};
   DWORD ret = ::GetEnvironmentVariable(L"PROGRAMFILES", program_dir, MAX_PATH);
   if (ret && ret < MAX_PATH && !::wcsnicmp(exe_path, program_dir, ret)) {
@@ skipping 100 matching lines @@
 }
 
 bool IsNonBrowserProcess() {
   assert(g_process_type != ProcessType::UNINITIALIZED);
   return g_process_type == ProcessType::NON_BROWSER_PROCESS;
 }
 
 bool GetDefaultUserDataDirectory(std::wstring* result) {
   static const wchar_t kLocalAppData[] = L"%LOCALAPPDATA%";
 
   std::unique_ptr<wchar_t> user_data_dir_path;

[Mark Mentovai, 2016/11/10 22:06:17]

This gets a new wchar_t[], so it should be a unique_ptr<wchar_t[]>

[scottmg, 2016/11/15 23:23:15]

Done.

 
   // This environment variable should be set on Windows 7 and up.
   // If we fail to find this variable then we default to the temporary files
   // path.
   DWORD size = ::ExpandEnvironmentStrings(kLocalAppData, nullptr, 0);
   if (size) {
     user_data_dir_path.reset(new wchar_t[size]);
     if (::ExpandEnvironmentStrings(kLocalAppData, user_data_dir_path.get(),
                                    size) != size) {
       user_data_dir_path.reset();
@@ skipping 14 matching lines @@
 
   *result = user_data_dir_path.get();
   if ((*result)[result->length() - 1] != L'\\')
     result->append(L"\\");
   result->append(install_sub_directory);
   result->append(L"\\");
   result->append(kUserDataDirname);
   return true;
 }
 
-bool GetDefaultCrashDumpLocation(std::wstring* crash_dir) {
-  // In order to be able to start crash handling very early, we do not rely on
-  // chrome's PathService entries (for DIR_CRASH_DUMPS) being available on
-  // Windows. See https://crbug.com/564398.
-  if (!GetDefaultUserDataDirectory(crash_dir))
+bool GetUserDataDirectory(std::wstring* result) {
+  // WARNING! This has to mirror the logic in chrome/app/chrome_main_delegate.cc
+  // in InitializeUserDataDir(). This function derives the path in the same way,
+  // but without using base, nor reporting on failure as the Chrome
+  // implementation does.
+  std::wstring user_data_dir = GetSwitchValueFromCommandLine(
+      ::GetCommandLine(), install_static::kUserDataDirSwitch);
+
+  CheckUserDataDirPolicy(&user_data_dir);
+
+  // On Windows, trailing separators leave Chrome in a bad state. See
+  // crbug.com/464616.
+  while (!user_data_dir.empty() &&
+         (user_data_dir.back() == '\\' || user_data_dir.back() == '/')) {
+    user_data_dir = user_data_dir.substr(0, user_data_dir.size() - 1);

[Mark Mentovai, 2016/11/10 22:06:17]

If C++11 is guaranteed (and it’s 2016, it should be, right?), just .pop_back()?

Otherwise, .erase(use_data_dir.size() - 1, 1) or .resize(user_data_dir.size() -
1) ought to be better as they work with the already-existing string.

[scottmg, 2016/11/15 23:23:15]

Done.

+  }
+
+  const bool specified_directory_was_invalid =

[Mark Mentovai, 2016/11/10 22:06:17]

This says: If user_data_dir is empty, stop and set was_invalid to false.
Otherwise, set was_invalid to !RecursiveDirectoryCreate().

Is that really what you want? It’s compatible (and bug-compatible) with the
other file, but wouldn’t we want to try GetDefaultUserDatadirectory() if
user_data_dir is empty?

was_INvalid and the !s make this pretty double-negative-y, which probably
contributed to the confusion. It should read more cleanly and be less bug-prone
if you have a more positive attitude.

  was_invalid = empty() || !RecursiveDirectoryCreate()
or
  was_valid = !empty() && RecursiveDirectoryCreate()

…if this behavior is really intended. If not, the rationale for basically
failing if user_data_dir is empty is lost on me, and someone needs a comment.

[scottmg, 2016/11/11 00:24:46]

Yeah, you're right. I just copied and adapted the other function into this one.
I didn't really process it too critically and assumed it was doing what you said
above (i.e. if one wasn't supplied, use the default and create it if necessary).

I'm not sure what the correct behaviour is now that I've started writing some
tests for it. The original implementation alludes to the browser process
handling it "later", but I'm not sure how/where that happens yet. Hmm...

[scottmg, 2016/11/15 23:23:15]

I think the behaviour is semi-rational now.

+      !user_data_dir.empty() && !RecursiveDirectoryCreate(user_data_dir);
+
+  if (specified_directory_was_invalid) {
+    if (!GetDefaultUserDataDirectory(&user_data_dir)) {

[Mark Mentovai, 2016/11/10 22:06:17]

This can be folded into the if that contains it.

[scottmg, 2016/11/15 23:23:15]

Done.

+      // The Chrome implementation CHECKs() here in the browser process. We
+      // don't as this function is used to initialize crash reporting, so
+      // we would get no report of this failure.
+      assert(false);
+      return false;
+    }
+  }
+
+  *result = user_data_dir;
+  return true;
+}
+
+bool GetCrashDumpLocation(std::wstring* crash_dir) {
+  // In order to be able to start crash handling very early and in chrome_elf,
+  // we cannot rely on chrome's PathService entries (for DIR_CRASH_DUMPS) being
+  // available on Windows. See https://crbug.com/564398.
+  if (!GetUserDataDirectory(crash_dir))
     return false;
 
   // We have to make sure the user data dir exists on first run. See
   // http://crbug.com/591504.
   if (!RecursiveDirectoryCreate(crash_dir->c_str()))
     return false;
   crash_dir->append(L"\\Crashpad");
   return true;
 }
 
@@ skipping 288 matching lines @@
         *result = -1;
         return true;
       }
     }
   }
   // Here it means that both versions are equal.
   *result = 0;
   return true;
 }
 
-std::string GetSwitchValueFromCommandLine(const std::string& command_line,
-                                          const std::string& switch_name) {
+std::wstring GetSwitchValueFromCommandLine(const std::wstring& command_line,
+                                           const std::wstring& switch_name) {
   assert(!command_line.empty());
   assert(!switch_name.empty());
 
-  std::string command_line_copy = command_line;
+  std::wstring command_line_copy = command_line;
   // Remove leading and trailing spaces.
-  TrimT<std::string>(&command_line_copy);
+  TrimT<std::wstring>(&command_line_copy);
 
   // Find the switch in the command line. If we don't find the switch, return
   // an empty string.
-  std::string switch_token = "--";
+  std::wstring switch_token = L"--";
   switch_token += switch_name;
-  switch_token += "=";
+  switch_token += L"=";
   size_t switch_offset = command_line_copy.find(switch_token);
   if (switch_offset == std::string::npos)
-    return std::string();
+    return std::wstring();
 
   // The format is "--<switch name>=blah". Look for a space after the
   // "--<switch name>=" string. If we don't find a space assume that the switch
   // value ends at the end of the command line.
   size_t switch_value_start_offset = switch_offset + switch_token.length();
-  if (std::string(kWhiteSpaces).find(
-          command_line_copy[switch_value_start_offset]) != std::string::npos) {
+  if (std::wstring(kWhiteSpaces16).find(
+          command_line_copy[switch_value_start_offset]) != std::wstring::npos) {
     switch_value_start_offset = command_line_copy.find_first_not_of(
-        GetWhiteSpacesForType<std::string>(), switch_value_start_offset);
-    if (switch_value_start_offset == std::string::npos)
-      return std::string();
+        GetWhiteSpacesForType<std::wstring>(), switch_value_start_offset);
+    if (switch_value_start_offset == std::wstring::npos)
+      return std::wstring();
   }
   size_t switch_value_end_offset =
-      command_line_copy.find_first_of(GetWhiteSpacesForType<std::string>(),
+      command_line_copy.find_first_of(GetWhiteSpacesForType<std::wstring>(),
                                       switch_value_start_offset);
-  if (switch_value_end_offset == std::string::npos)
+  if (switch_value_end_offset == std::wstring::npos)
     switch_value_end_offset = command_line_copy.length();
 
-  std::string switch_value = command_line_copy.substr(
+  std::wstring switch_value = command_line_copy.substr(
       switch_value_start_offset,
       switch_value_end_offset - (switch_offset + switch_token.length()));
-  TrimT<std::string>(&switch_value);
+  TrimT<std::wstring>(&switch_value);
   return switch_value;
 }
 
 }  // namespace install_static