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 <assert.h>
+#include <stdlib.h>
+#include <string.h>
 #include <windows.h>

[grt (UTC plus 2), 2016/11/21 10:29:54]

nit: i think we generally include windows.h first since it must be included
before certain other system headers. i dunno how universal this is, but i
thought it was a thing.

[scottmg, 2016/11/21 19:34:00]

Done.

-#include <assert.h>
-#include <string.h>
 
 #include <algorithm>
 #include <memory>
 #include <sstream>
 
 #include "chrome/install_static/install_details.h"
 #include "chrome/install_static/install_modes.h"
+#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;
 
 const wchar_t kRegValueChromeStatsSample[] = L"UsageStatsInSample";
 
 // 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.
 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[] =

[grt (UTC plus 2), 2016/11/21 10:29:54]

please follow the strategy in ReportingIsEnforcedByPolicy to compute this path.

[scottmg, 2016/11/21 19:34:01]

Done.

+    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
 // The constants defined in this file are also defined in chrome/installer and
 // other places. we need to unify them.
 // Chrome channel display names.
 constexpr wchar_t kChromeChannelDev[] = L"dev";
 constexpr wchar_t kChromeChannelBeta[] = L"beta";
@@ skipping 94 matching lines @@
     uint32_t size = 0;
     BOOL r = ::VerQueryValueW(version_resource, sub_block, &value, &size);
     if (r && value) {
       value_str->assign(static_cast<wchar_t*>(value));
       return true;
     }
   }
   return false;
 }
 
+std::wstring MakeAbsoluteFilePath(const std::wstring& input) {
+  wchar_t file_path[MAX_PATH];
+  if (!_wfullpath(file_path, input.c_str(), _countof(file_path)))
+    return std::wstring();
+  return file_path;
+}
+
+bool DirectoryExists(const std::wstring& path) {
+  DWORD file_attributes = ::GetFileAttributes(path.c_str());
+  if (file_attributes == INVALID_FILE_ATTRIBUTES)
+    return false;
+  return (file_attributes & FILE_ATTRIBUTE_DIRECTORY) != 0;
+}
+
 bool RecursiveDirectoryCreate(const std::wstring& full_path) {
   // If the path exists, we've succeeded if it's a directory, failed otherwise.
   const wchar_t* full_path_str = full_path.c_str();
   DWORD file_attributes = ::GetFileAttributes(full_path_str);
   if (file_attributes != INVALID_FILE_ATTRIBUTES) {
     if ((file_attributes & FILE_ATTRIBUTE_DIRECTORY) != 0) {
       Trace(L"%hs( %ls directory exists )\n", __func__, full_path_str);
       return true;
     }
     Trace(L"%hs( %ls directory conflicts with an existing file. )\n", __func__,
@@ skipping 10 matching lines @@
   std::size_t pos = full_path.find_last_of(L"/\\");
   if (pos != std::wstring::npos) {
     parent_path = full_path.substr(0, pos);
     if (!RecursiveDirectoryCreate(parent_path)) {
       Trace(L"Failed to create one of the parent directories");
       return false;
     }
   }
   if (!::CreateDirectory(full_path_str, nullptr)) {
     DWORD error_code = ::GetLastError();
-    if (error_code == ERROR_ALREADY_EXISTS) {
-      DWORD file_attributes = ::GetFileAttributes(full_path_str);
-      if ((file_attributes != INVALID_FILE_ATTRIBUTES) &&
-          ((file_attributes & FILE_ATTRIBUTE_DIRECTORY) != 0)) {
-        // This error code ERROR_ALREADY_EXISTS doesn't indicate whether we
-        // were racing with someone creating the same directory, or a file
-        // with the same path.  If the directory exists, we lost the
-        // race to create the same directory.
-        return true;
-      } else {
-        Trace(L"Failed to create directory %ls, last error is %d\n",
-              full_path_str, error_code);
-        return false;
-      }
+    if (error_code == ERROR_ALREADY_EXISTS && DirectoryExists(full_path_str)) {
+      // This error code ERROR_ALREADY_EXISTS doesn't indicate whether we
+      // were racing with someone creating the same directory, or a file
+      // with the same path.  If the directory exists, we lost the
+      // race to create the same directory.
+      return true;
+    } else {
+      Trace(L"Failed to create directory %ls, last error is %d\n",
+            full_path_str, error_code);
+      return false;
     }
   }
   return true;
 }
 
 // Appends "[kCompanyPathName\]kProductPathName[install_suffix]" to |path|,
 // returning a reference to |path|.
 std::wstring& AppendChromeInstallSubDirectory(std::wstring* path,
                                               bool include_suffix) {
   if (*kCompanyPathName) {
@@ skipping 144 matching lines @@
   // rules in the update configs. ChannelInfo::GetChannelName painstakingly
   // strips off known modifiers (e.g., "-multi-full") to see if the empty string
   // remains, returning channel "unknown" if not. This differs here in that some
   // clients will tag crashes as "stable" rather than "unknown" via this
   // codepath, but it is an accurate reflection of which update channel the
   // client is on according to the server-side rules.
 
   return channel_name;
 }
 
+// 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)) {

[grt (UTC plus 2), 2016/11/21 10:29:54]

nit:
  if (nt::QueryRegValueSZ(hive, nt::NONE, policy_path, key_name, &value)) {

[scottmg, 2016/11/21 19:34:01]

Done.

+    if (nt::QueryRegValueSZ(key, key_name, &value)) {
+      *dir = install_static::ExpandPathVariables(value);

[grt (UTC plus 2), 2016/11/21 10:29:54]

nit: omit "install_static::"

[scottmg, 2016/11/21 19:34:00]

Done.

+      result = true;
+    }
+    nt::CloseRegKey(key);
+  }
+  return result;
+}
+
+void CheckUserDataDirPolicy(std::wstring* user_data_dir) {

[grt (UTC plus 2), 2016/11/21 10:29:54]

please document this. the caller assumes that |user_data_dir| will only be
modified if a value is found. also, "Check" isn't a great name since it actually
Gets the policy value.

[scottmg, 2016/11/21 19:34:00]

Done.

+  assert(user_data_dir);
+  // Policy from the HKLM hive has precedence over HKCU.

[grt (UTC plus 2), 2016/11/21 10:29:54]

i don't think it would be a bad thing for this to look more like
ReportingIsEnforcedByPolicy below

[scottmg, 2016/11/21 19:34:01]

Done.

+  if (!LoadUserDataDirPolicyFromRegistry(nt::HKLM, kUserDataDirRegKey,
+                                         user_data_dir)) {
+    LoadUserDataDirPolicyFromRegistry(nt::HKCU, kUserDataDirRegKey,
+                                      user_data_dir);
+  }
+}
+
 }  // namespace
 
 bool IsSystemInstall() {
   return InstallDetails::Get().system_level();
 }
 
 bool IsMultiInstall() {
   return InstallDetails::Get().multi_install();
 }
 
@@ skipping 120 matching lines @@
 
   result->swap(user_data_dir);
   if ((*result)[result->length() - 1] != L'\\')
     result->push_back(L'\\');
   AppendChromeInstallSubDirectory(result, true /* include_suffix */);
   result->push_back(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(const std::wstring& user_data_dir_from_command_line,

[grt (UTC plus 2), 2016/11/21 10:29:54]

if i'm not mistaken, the only codepath for
GetUserDataDirectoryUsingProcessCommandLine, GetUserDataDirectory,
CheckUserDataDirPolicy, and GetDefaultUserDataDirectory is the one during
startup when creating the ProductInstallDetails. i think it's cleaner to move
all of the code for this into product_install_details.cc and rejigger as needed
so that they can be called by MakeProductDetails before the global instance has
been put into place. i think this will resolve the GetMutable thing.

[scottmg, 2016/11/21 19:34:00]

Done. It's bit ugly to have to pass the partial InstallDetails through things
(it's needed for install_suffix in AppendChromeInstallSubdirectory() which I
didn't explain very clearly). But I think overall this is probably nicer now.

+                          std::wstring* result,
+                          std::wstring* invalid_supplied_directory) {
+  std::wstring user_data_dir = user_data_dir_from_command_line;
+
+  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.pop_back();
+  }
+
+  bool got_valid_directory =
+      !user_data_dir.empty() && RecursiveDirectoryCreate(user_data_dir);
+  if (!got_valid_directory) {
+    *invalid_supplied_directory = user_data_dir;
+    got_valid_directory = GetDefaultUserDataDirectory(&user_data_dir);
+  }
+
+  // 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(got_valid_directory);
+  if (!got_valid_directory)
     return false;
 
-  // We have to make sure the user data dir exists on first run. See
-  // http://crbug.com/591504.
-  if (!RecursiveDirectoryCreate(*crash_dir))
-    return false;
+  *result = MakeAbsoluteFilePath(user_data_dir);
+  return true;
+}
+
+bool GetUserDataDirectoryUsingProcessCommandLine(
+    std::wstring* result,
+    std::wstring* invalid_supplied_directory) {
+  return GetUserDataDirectory(
+      GetSwitchValueFromCommandLine(::GetCommandLine(),
+                                    install_static::kUserDataDirSwitch),

[grt (UTC plus 2), 2016/11/21 10:29:54]

nit: omit "install_static::"

[scottmg, 2016/11/21 19:34:01]

Done.

+      result, invalid_supplied_directory);
+}
+
+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.
+  *crash_dir = install_static::InstallDetails::Get().user_data_dir();

[grt (UTC plus 2), 2016/11/21 10:29:54]

nit: omit "install_static::"

[scottmg, 2016/11/21 19:34:00]

Done.

   crash_dir->append(L"\\Crashpad");
   return true;
 }
 
 std::string GetEnvironmentString(const std::string& variable_name) {
   return UTF16ToUTF8(
       GetEnvironmentString16(UTF8ToUTF16(variable_name).c_str()));
 }
 
 std::wstring GetEnvironmentString16(const wchar_t* variable_name) {
@@ skipping 121 matching lines @@
                                         bool trim_spaces) {
   return TokenizeStringT<std::string>(str, delimiter, trim_spaces);
 }
 
 std::vector<std::wstring> TokenizeString16(const std::wstring& str,
                                            wchar_t delimiter,
                                            bool trim_spaces) {
   return TokenizeStringT<std::wstring>(str, delimiter, trim_spaces);
 }
 
-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;
 }
 
 // This function takes these inputs rather than accessing the module's
 // InstallDetails instance since it is used to bootstrap InstallDetails.
 std::wstring DetermineChannel(const InstallConstants& mode,
                               bool system_level,
                               bool multi_install) {
   if (!kUseGoogleUpdateIntegration)
     return std::wstring();
 
   switch (mode.channel_strategy) {
     case ChannelStrategy::UNSUPPORTED:
       assert(false);
       break;
     case ChannelStrategy::ADDITIONAL_PARAMETERS:
       return ChannelFromAdditionalParameters(mode, system_level, multi_install);
     case ChannelStrategy::FIXED:
       return mode.default_channel_name;
   }
 
   return std::wstring();
 }
 
 }  // namespace install_static