Adding error handlers to setup.exe.

The handlers make setup crash cleanly when we run out of memory, on heap corruption and on invalid parameters in the CRT. Moved 2 functions from startup_helper_win to base target. Renamed startup_helper_win to sandbox_helper_win now that it contains only a function related to the sandbox. BUG=530624 Review URL: https://codereview.chromium.org/1387963006 Cr-Commit-Position: refs/heads/master@{#354591}
marcosholgado · Oct 16, 2015 · 18d3ed3 · 18d3ed3
1 parent f12641d
commit 18d3ed3
Show file tree

Hide file tree

Showing 34 changed files with 149 additions and 109 deletions.
diff --git a/ash/ash.gyp b/ash/ash.gyp
@@ -1300,7 +1300,7 @@
       'conditions': [
         ['OS=="win"', {
           'dependencies': [
-            '../content/content.gyp:content_startup_helper_win',
+            '../content/content.gyp:sandbox_helper_win',
           ],
         }],
       ],

diff --git a/ash/shell/content/shell_with_content_main.cc b/ash/shell/content/shell_with_content_main.cc
@@ -10,7 +10,7 @@
 #include "content/public/app/content_main.h"
 
 #if defined(OS_WIN)
-#include "content/public/app/startup_helper_win.h"
+#include "content/public/app/sandbox_helper_win.h"
 #include "sandbox/win/src/sandbox_types.h"
 #endif
 

diff --git a/base/BUILD.gn b/base/BUILD.gn
@@ -608,6 +608,8 @@ component("base") {
     "win/metro.h",
     "win/object_watcher.cc",
     "win/object_watcher.h",
+    "win/process_startup_helper.cc",
+    "win/process_startup_helper.h",
     "win/registry.cc",
     "win/registry.h",
     "win/resource_util.cc",

diff --git a/base/base.gypi b/base/base.gypi
@@ -726,6 +726,8 @@
           'win/metro.h',
           'win/object_watcher.cc',
           'win/object_watcher.h',
+          'win/process_startup_helper.cc',
+          'win/process_startup_helper.h',
           'win/registry.cc',
           'win/registry.h',
           'win/resource_util.cc',

diff --git a/content/app/startup_helper_win.cc → base/win/process_startup_helper.cc b/content/app/startup_helper_win.cc → base/win/process_startup_helper.cc
@@ -1,17 +1,14 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2015 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 "content/public/app/startup_helper_win.h"
+#include "base/win/process_startup_helper.h"
 
 #include <crtdbg.h>
 #include <new.h>
 
 #include "base/base_switches.h"
 #include "base/command_line.h"
-#include "base/win/windows_version.h"
-#include "sandbox/win/src/process_mitigations.h"
-#include "sandbox/win/src/sandbox_factory.h"
 
 namespace {
 
@@ -33,20 +30,8 @@ void PureCall() {
 
 }  // namespace
 
-namespace content {
-
-void InitializeSandboxInfo(sandbox::SandboxInterfaceInfo* info) {
-  info->broker_services = sandbox::SandboxFactory::GetBrokerServices();
-  if (!info->broker_services) {
-    info->target_services = sandbox::SandboxFactory::GetTargetServices();
-  } else {
-    // Ensure the proper mitigations are enforced for the browser process.
-    sandbox::ApplyProcessMitigationsToCurrentProcess(
-        sandbox::MITIGATION_DEP |
-        sandbox::MITIGATION_DEP_NO_ATL_THUNK |
-        sandbox::MITIGATION_HARDEN_TOKEN_IL_POLICY);
-  }
-}
+namespace base {
+namespace win {
 
 // Register the invalid param handler and pure call handler to be able to
 // notify breakpad when it happens.
@@ -55,7 +40,7 @@ void RegisterInvalidParamHandler() {
   _set_purecall_handler(PureCall);
 }
 
-void SetupCRT(const base::CommandLine& command_line) {
+void SetupCRT(const CommandLine& command_line) {
 #if defined(_CRTDBG_MAP_ALLOC)
   _CrtSetReportFile(_CRT_WARN, _CRTDBG_FILE_STDERR);
   _CrtSetReportMode(_CRT_WARN, _CRTDBG_MODE_FILE);
@@ -66,4 +51,5 @@ void SetupCRT(const base::CommandLine& command_line) {
 #endif
 }
 
-}  // namespace content
+}  // namespace win
+}  // namespace base
diff --git a/base/win/process_startup_helper.h b/base/win/process_startup_helper.h
@@ -0,0 +1,26 @@
+// Copyright (c) 2015 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.
+
+#ifndef BASE_WIN_PROCESS_STARTUP_HELPER_H_
+#define BASE_WIN_PROCESS_STARTUP_HELPER_H_
+
+#include "base/base_export.h"
+
+namespace base {
+
+class CommandLine;
+
+namespace win {
+
+// Register the invalid param handler and pure call handler to be able to
+// notify breakpad when it happens.
+BASE_EXPORT void RegisterInvalidParamHandler();
+
+// Sets up the CRT's debugging macros to output to stdout.
+BASE_EXPORT void SetupCRT(const CommandLine& command_line);
+
+}  // namespace win
+}  // namespace base
+
+#endif  // BASE_WIN_PROCESS_STARTUP_HELPER_H_
diff --git a/chrome/BUILD.gn b/chrome/BUILD.gn
@@ -148,7 +148,7 @@ if (!is_android) {
         "//components/browser_watcher:browser_watcher_client",
         "//components/crash/content/app",
         "//components/crash/core/common",
-        "//content:startup_helper_win",
+        "//content:sandbox_helper_win",
         "//crypto",
         "//sandbox",
         "//ui/gfx",

diff --git a/chrome/app/client_util.cc b/chrome/app/client_util.cc
@@ -42,7 +42,7 @@
 #include "components/crash/content/app/breakpad_win.h"
 #include "components/crash/content/app/crash_reporter_client.h"
 #include "components/metrics/client_info.h"
-#include "content/public/app/startup_helper_win.h"
+#include "content/public/app/sandbox_helper_win.h"
 #include "content/public/common/content_switches.h"
 #include "sandbox/win/src/sandbox.h"
 
@@ -319,7 +319,7 @@ void ChromeDllLoader::OnBeforeLaunch(const std::string& process_type,
                 g_chrome_crash_client.Get().GetIsPerUserInstall(
                     base::FilePath(exe_path));
             if (g_chrome_crash_client.Get().GetShouldDumpLargerDumps(
-                    is_per_user_install)){
+                    is_per_user_install)) {
               minidump_type = kasko::api::LARGER_DUMP_TYPE;
             }
           }

diff --git a/chrome/chrome_exe.gypi b/chrome/chrome_exe.gypi
@@ -56,7 +56,7 @@
       'sources': [
         # Note that due to InitializeSandboxInfo, this must be directly linked
         # into chrome.exe, not into a dependent.
-        '<(DEPTH)/content/app/startup_helper_win.cc',
+        '<(DEPTH)/content/app/sandbox_helper_win.cc',
         '<(DEPTH)/content/public/common/content_switches.cc',
         'app/chrome_exe_load_config_win.cc',
         'app/chrome_exe_main_aura.cc',
@@ -531,7 +531,7 @@
               'type': 'executable',
               'product_name': 'nacl64',
               'sources': [
-                '../content/app/startup_helper_win.cc',
+                '../content/app/sandbox_helper_win.cc',
                 '../content/common/sandbox_init_win.cc',
                 '../content/common/sandbox_win.cc',
                 '../content/public/common/content_switches.cc',

diff --git a/chrome/installer/setup/setup_main.cc b/chrome/installer/setup/setup_main.cc
@@ -21,13 +21,15 @@
 #include "base/memory/scoped_ptr.h"
 #include "base/path_service.h"
 #include "base/process/launch.h"
+#include "base/process/memory.h"
 #include "base/strings/string16.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/values.h"
 #include "base/version.h"
+#include "base/win/process_startup_helper.h"
 #include "base/win/registry.h"
 #include "base/win/scoped_com_initializer.h"
 #include "base/win/scoped_comptr.h"
@@ -1758,6 +1760,12 @@ int WINAPI wWinMain(HINSTANCE instance, HINSTANCE prev_instance,
   scoped_ptr<google_breakpad::ExceptionHandler> breakpad(
       InitializeCrashReporting(system_install));
 
+  // Make sure the process exits cleanly on unexpected errors.
+  base::EnableTerminationOnHeapCorruption();
+  base::EnableTerminationOnOutOfMemory();
+  base::win::RegisterInvalidParamHandler();
+  base::win::SetupCRT(cmd_line);
+
   InstallationState original_state;
   original_state.Initialize();
 

diff --git a/chrome/nacl/DEPS b/chrome/nacl/DEPS
@@ -2,7 +2,7 @@ include_rules = [
   "+chrome/app/chrome_crash_reporter_client.h",
   "+components/crash",
   "+components/nacl",
-  "+content/public/app/startup_helper_win.h",
+  "+content/public/app/sandbox_helper_win.h",
   "+content/public/common/content_switches.h",
   "+sandbox/win/src",
 ]
diff --git a/components/nacl/broker/DEPS b/components/nacl/broker/DEPS
@@ -1,4 +1,4 @@
 include_rules = [
-  "+content/public/app/startup_helper_win.h",
+  "+content/public/app/sandbox_helper_win.h",
   "+sandbox/win/src",
 ]
diff --git a/components/nacl/loader/DEPS b/components/nacl/loader/DEPS
@@ -1,6 +1,6 @@
 include_rules = [
   "+components/nacl",
-  "+content/public/app/startup_helper_win.h",
+  "+content/public/app/sandbox_helper_win.h",
   "+content/public/common",
   "+crypto",
   "+sandbox/linux/bpf_dsl",

diff --git a/components/nacl/loader/nacl_helper_win_64.cc b/components/nacl/loader/nacl_helper_win_64.cc
@@ -2,6 +2,10 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "components/nacl/loader/nacl_helper_win_64.h"
+
+#include <string>
+
 #include "base/command_line.h"
 #include "base/logging.h"
 #include "base/message_loop/message_loop.h"
@@ -11,11 +15,12 @@
 #include "base/process/memory.h"
 #include "base/strings/string_util.h"
 #include "base/timer/hi_res_timer_manager.h"
+#include "base/win/process_startup_helper.h"
 #include "components/nacl/broker/nacl_broker_listener.h"
 #include "components/nacl/common/nacl_switches.h"
 #include "components/nacl/loader/nacl_listener.h"
 #include "components/nacl/loader/nacl_main_platform_delegate.h"
-#include "content/public/app/startup_helper_win.h"
+#include "content/public/app/sandbox_helper_win.h"
 #include "content/public/common/content_switches.h"
 #include "content/public/common/main_function_params.h"
 #include "content/public/common/sandbox_init.h"
@@ -57,8 +62,8 @@ int NaClWin64Main() {
   // Copy what ContentMain() does.
   base::EnableTerminationOnHeapCorruption();
   base::EnableTerminationOnOutOfMemory();
-  content::RegisterInvalidParamHandler();
-  content::SetupCRT(command_line);
+  base::win::RegisterInvalidParamHandler();
+  base::win::SetupCRT(command_line);
   // Route stdio to parent console (if any) or create one.
   if (command_line.HasSwitch(switches::kEnableLogging))
     base::RouteStdioToConsole(true);

diff --git a/content/BUILD.gn b/content/BUILD.gn
@@ -86,16 +86,14 @@ source_set("export") {
 # In the GYP build, this file is listed in several targets. In GN just have
 # those targets depend on this one. This can be depended on for any
 # platform for simplicity, and is a no-op on non-Windows.
-source_set("startup_helper_win") {
+source_set("sandbox_helper_win") {
   if (is_win) {
     sources = [
-      "app/startup_helper_win.cc",
-      "public/app/startup_helper_win.h",
+      "app/sandbox_helper_win.cc",
+      "public/app/sandbox_helper_win.h",
     ]
 
     deps = [
-      "//base",
-      "//base:i18n",
       "//sandbox",
     ]
   }

diff --git a/content/app/BUILD.gn b/content/app/BUILD.gn
@@ -31,7 +31,7 @@ content_app_deps = [
   # picking the allocator.
   "//base/allocator",
   "//content:export",
-  "//content:startup_helper_win",
+  "//content:sandbox_helper_win",
   "//content/public/common:common_sources",
   "//content/public/common:mojo_bindings",
   "//crypto",

diff --git a/content/app/content_main_runner.cc b/content/app/content_main_runner.cc
@@ -6,6 +6,8 @@
 
 #include <stdlib.h>
 
+#include <string>
+
 #include "base/allocator/allocator_extension.h"
 #include "base/at_exit.h"
 #include "base/command_line.h"
@@ -36,7 +38,6 @@
 #include "content/gpu/in_process_gpu_thread.h"
 #include "content/public/app/content_main.h"
 #include "content/public/app/content_main_delegate.h"
-#include "content/public/app/startup_helper_win.h"
 #include "content/public/browser/content_browser_client.h"
 #include "content/public/common/content_client.h"
 #include "content/public/common/content_constants.h"
@@ -75,8 +76,8 @@
 #include <malloc.h>
 #include <cstring>
 
-#include "base/strings/string_number_conversions.h"
 #include "base/trace_event/trace_event_etw_export_win.h"
+#include "base/win/process_startup_helper.h"
 #include "ui/base/win/atl_module.h"
 #include "ui/gfx/win/dpi.h"
 #elif defined(OS_MACOSX)
@@ -98,7 +99,6 @@
 #include "content/public/common/content_descriptors.h"
 
 #if !defined(OS_MACOSX)
-#include "content/public/common/content_descriptors.h"
 #include "content/public/common/zygote_fork_delegate_linux.h"
 #endif
 #if !defined(OS_MACOSX) && !defined(OS_ANDROID)
@@ -157,8 +157,8 @@ void SetupSignalHandlers() {
   // Sanitise our signal handling state. Signals that were ignored by our
   // parent will also be ignored by us. We also inherit our parent's sigmask.
   sigset_t empty_signal_set;
-  CHECK(0 == sigemptyset(&empty_signal_set));
-  CHECK(0 == sigprocmask(SIG_SETMASK, &empty_signal_set, NULL));
+  CHECK_EQ(0, sigemptyset(&empty_signal_set));
+  CHECK_EQ(0, sigprocmask(SIG_SETMASK, &empty_signal_set, NULL));
 
   struct sigaction sigact;
   memset(&sigact, 0, sizeof(sigact));
@@ -167,11 +167,11 @@ void SetupSignalHandlers() {
       {SIGHUP, SIGINT, SIGQUIT, SIGILL, SIGABRT, SIGFPE, SIGSEGV,
        SIGALRM, SIGTERM, SIGCHLD, SIGBUS, SIGTRAP};  // SIGPIPE is set below.
   for (unsigned i = 0; i < arraysize(signals_to_reset); i++) {
-    CHECK(0 == sigaction(signals_to_reset[i], &sigact, NULL));
+    CHECK_EQ(0, sigaction(signals_to_reset[i], &sigact, NULL));
   }
 
   // Always ignore SIGPIPE.  We check the return value of write().
-  CHECK(signal(SIGPIPE, SIG_IGN) != SIG_ERR);
+  CHECK_NE(SIG_ERR, signal(SIGPIPE, SIG_IGN));
 }
 
 #endif  // OS_POSIX && !OS_IOS
@@ -443,7 +443,7 @@ class ContentMainRunnerImpl : public ContentMainRunner {
 
     base::EnableTerminationOnOutOfMemory();
 #if defined(OS_WIN)
-    RegisterInvalidParamHandler();
+    base::win::RegisterInvalidParamHandler();
     ui::win::CreateATLModuleIfNeeded();
 
     sandbox_info_ = *params.sandbox_info;
@@ -567,7 +567,7 @@ class ContentMainRunnerImpl : public ContentMainRunner {
 #if !defined(OS_IOS)
     SetProcessTitleFromCommandLine(argv);
 #endif
-#endif // !OS_ANDROID
+#endif  // !OS_ANDROID
 
     int exit_code = 0;
     if (delegate_ && delegate_->BasicStartupComplete(&exit_code))
@@ -639,7 +639,7 @@ class ContentMainRunnerImpl : public ContentMainRunner {
     // Other OSes have to wait till we get here in order for all the memory
     // management setup to be completed.
     TRACE_EVENT0("startup,benchmark", "ContentMainRunnerImpl::Initialize");
-#endif // !OS_ANDROID
+#endif  // !OS_ANDROID
 
 #if defined(OS_MACOSX) && !defined(OS_IOS)
     // We need to allocate the IO Ports before the Sandbox is initialized or
@@ -669,7 +669,7 @@ class ContentMainRunnerImpl : public ContentMainRunner {
       }
     }
 #elif defined(OS_WIN)
-    SetupCRT(command_line);
+    base::win::SetupCRT(command_line);
 #endif
 
 #if defined(OS_POSIX)