win: Address failure-to-start-handler case for async startup

Second follow up to https://chromium-review.googlesource.com/c/400015/

The ideal would be that if we fail to start the handler, then we don't
end up passing through our unhandled exception filter at all.

In the case of the non-initial client (i.e. renderers) we can do this by
not setting our UnhandledExceptionFilter until after we know we've
connected successfully (because those connections are synchronous from
its point of view). We also change WaitForNamedPipe in the connection
message to block forever, so as long as the precreated pipe exists,
they'll wait to connect. After the initial client has passed the server
side of that pipe to the handler, the handler has the only handle to it.
So, if the handler has disappeared for whatever reason, pipe-connecting
clients will fail with FILE_NOT_FOUND, and will not stick around in the
connection loop. This means non-initial clients do not need additional
logic to avoid getting stuck in our UnhandledExceptionFilter.

For the initial client, it would be ideal to avoid passing through our
UEF too, but none of the 3 options are great:
1. Block until we find out if we started, and then install the filter.
   We don't want to do that, because we don't want to wait.
2. Restore the old filter if it turns out we failed to start. We can't
   do that because Chrome disables ::SetUnhandledExceptionFilter()
   immediately after StartHandler/SetHandlerIPCPipe returns.
3. Don't install our filter until we've successfully started. We don't
   want to do that because we'd miss early crashes, negating the benefit
   of deferred startup.

So, we do need to pass through our UnhandledExceptionFilter. I don't
want more Win32 API calls during the vulnerable filter function. So, at
any point during async startup where there's a failure, set a global
atomic that allows the filter function to abort without trying to signal
a handler that's known to not exist.

One further improvement we might want to look at is unexpected
termination of the handler (as opposed to a failure to start) which
would still result in a useless Sleep(60s). This isn't new behaviour,
but now we have a clear thing to do if we detect the handler is gone.

(Also a missing DWORD/size_t cast for the _x64 bots.)

R=mark@chromium.org
BUG=chromium:567850,chromium:656800

Change-Id: I5be831ca39bd8b2e5c962b9647c8bd469e2be878
Reviewed-on: https://chromium-review.googlesource.com/400985
Reviewed-by: Mark Mentovai <mark@chromium.org>

client/crashpad_client_win.cc

@@ -22,6 +22,7 @@
 
 #include "base/atomicops.h"
 #include "base/logging.h"
+#include "base/macros.h"
 #include "base/scoped_generic.h"
 #include "base/strings/string16.h"
 #include "base/strings/stringprintf.h"
@@ -67,6 +68,19 @@ base::Lock* g_non_crash_dump_lock;
 // dump.
 ExceptionInformation g_non_crash_exception_information;
 
+enum class StartupState : int {
+  kNotReady = 0,   // This must be value 0 because it is the initial value of a
+                   // global AtomicWord.
+  kSucceeded = 1,  // The CreateProcess() for the handler succeeded.
+  kFailed = 2,     // The handler failed to start.
+};
+
+// This is a tri-state of type StartupState. It starts at 0 == kNotReady, and
+// when the handler is known to have started successfully, or failed to start
+// the value will be updated. The unhandled exception filter will not proceed
+// until one of those two cases happens.
+base::subtle::AtomicWord g_handler_startup_state;
+
 // A CRITICAL_SECTION initialized with
 // RTL_CRITICAL_SECTION_FLAG_FORCE_DEBUG_INFO to force it to be allocated with a
 // valid .DebugInfo field. The address of this critical section is given to the
@@ -74,7 +88,32 @@ ExceptionInformation g_non_crash_exception_information;
 // list, so this allows the handler to capture all of them.
 CRITICAL_SECTION g_critical_section_with_debug_info;
 
+void SetHandlerStartupState(StartupState state) {
+  DCHECK(state == StartupState::kSucceeded ||
+         state == StartupState::kFailed);
+  base::subtle::Acquire_Store(&g_handler_startup_state,
+                              static_cast<base::subtle::AtomicWord>(state));
+}
+
 LONG WINAPI UnhandledExceptionHandler(EXCEPTION_POINTERS* exception_pointers) {
+  // Wait until we know the handler has either succeeded or failed to start.
+  base::subtle::AtomicWord startup_state;
+  while (
+      (startup_state = base::subtle::Release_Load(&g_handler_startup_state)) ==
+      static_cast<int>(StartupState::kNotReady)) {
+    Sleep(1);
+  }
+
+  if (startup_state == static_cast<int>(StartupState::kFailed)) {
+    // If we know for certain that the handler has failed to start, then abort
+    // here, rather than trying to signal to a handler that will never arrive,
+    // and then sleeping unnecessarily.
+    LOG(ERROR) << "crash server failed to launch, self-terminating";
+    TerminateProcess(GetCurrentProcess(), kTerminationCodeCrashNoDump);
+    return EXCEPTION_CONTINUE_SEARCH;
+  }
+  // Otherwise, we know the handler startup has succeeded, and we can continue.
+
   // Tracks whether a thread has already entered UnhandledExceptionHandler.
   static base::subtle::AtomicWord have_crashed;
 
@@ -226,6 +265,7 @@ struct BackgroundHandlerStartThreadData {
       const std::string& url,
       const std::map<std::string, std::string>& annotations,
       const std::vector<std::string>& arguments,
+      const std::wstring& ipc_pipe,
       ScopedFileHANDLE ipc_pipe_handle)
       : handler(handler),
         database(database),
@@ -233,6 +273,7 @@ struct BackgroundHandlerStartThreadData {
         url(url),
         annotations(annotations),
         arguments(arguments),
+        ipc_pipe(ipc_pipe),
         ipc_pipe_handle(std::move(ipc_pipe_handle)) {}
 
   base::FilePath handler;
@@ -241,11 +282,33 @@ struct BackgroundHandlerStartThreadData {
   std::string url;
   std::map<std::string, std::string> annotations;
   std::vector<std::string> arguments;
+  std::wstring ipc_pipe;
   ScopedFileHANDLE ipc_pipe_handle;
 };
 
+// Ensures that SetHandlerStartupState() is called on scope exit. Assumes
+// failure, and on success, SetSuccessful() should be called.
+class ScopedCallSetHandlerStartupState {
+ public:
+  ScopedCallSetHandlerStartupState() : successful_(false) {}
+
+  ~ScopedCallSetHandlerStartupState() {
+    SetHandlerStartupState(successful_ ? StartupState::kSucceeded
+                                       : StartupState::kFailed);
+  }
+
+  void SetSuccessful() { successful_ = true; }
+
+ private:
+  bool successful_;
+
+  DISALLOW_COPY_AND_ASSIGN(ScopedCallSetHandlerStartupState);
+};
+
 bool StartHandlerProcess(
     std::unique_ptr<BackgroundHandlerStartThreadData> data) {
+  ScopedCallSetHandlerStartupState scoped_startup_state_caller;
+
   std::wstring command_line;
   AppendCommandLineArgument(data->handler.value(), &command_line);
   for (const std::string& argument : data->arguments) {
@@ -393,6 +456,22 @@ bool StartHandlerProcess(
   rv = CloseHandle(process_info.hProcess);
   PLOG_IF(WARNING, !rv) << "CloseHandle process";
 
+  // It is important to close our side of the pipe here before confirming that
+  // we can communicate with the server. By doing so, the only remaining copy of
+  // the server side of the pipe belongs to the exception handler process we
+  // just spawned. Otherwise, the pipe will continue to exist indefinitely, so
+  // the connection loop will not detect that it will never be serviced.
+  data->ipc_pipe_handle.reset();
+
+  // Confirm that the server is waiting for connections before continuing.
+  ClientToServerMessage message = {};
+  message.type = ClientToServerMessage::kPing;
+  ServerToClientMessage response = {};
+  if (!SendToCrashHandlerServer(data->ipc_pipe, message, &response)) {
+    return false;
+  }
+
+  scoped_startup_state_caller.SetSuccessful();
   return true;
 }
 
@@ -414,10 +493,6 @@ void CommonInProcessInitialization() {
       &g_critical_section_with_debug_info);
 
   g_non_crash_dump_lock = new base::Lock();
-
-  // In theory we could store the previous handler but it is not clear what
-  // use we have for it.
-  SetUnhandledExceptionFilter(&UnhandledExceptionHandler);
 }
 
 }  // namespace
@@ -456,12 +531,15 @@ bool CrashpadClient::StartHandler(
 
   CommonInProcessInitialization();
 
+  SetUnhandledExceptionFilter(&UnhandledExceptionHandler);
+
   auto data = new BackgroundHandlerStartThreadData(handler,
                                                    database,
                                                    metrics_dir,
                                                    url,
                                                    annotations,
                                                    arguments,
+                                                   ipc_pipe_,
                                                    std::move(ipc_pipe_handle));
 
   if (asynchronous_start) {
@@ -478,6 +556,7 @@ bool CrashpadClient::StartHandler(
                                              nullptr));
     if (!handler_start_thread_.is_valid()) {
       PLOG(ERROR) << "CreateThread";
+      SetHandlerStartupState(StartupState::kFailed);
       return false;
     }
 
@@ -524,6 +603,9 @@ bool CrashpadClient::SetHandlerIPCPipe(const std::wstring& ipc_pipe) {
     return false;
   }
 
+  SetHandlerStartupState(StartupState::kSucceeded);
+  SetUnhandledExceptionFilter(&UnhandledExceptionHandler);
+
   // The server returns these already duplicated to be valid in this process.
   g_signal_exception =
       IntToHandle(response.registration.request_crash_dump_event);

util/win/exception_handler_server.cc

@@ -271,8 +271,10 @@ void ExceptionHandlerServer::InitializeWithInheritedDataForInitialClient(
   // TODO(scottmg): Vista+. Might need to pass through or possibly find an Nt*.
   size_t bytes = sizeof(wchar_t) * _MAX_PATH + sizeof(FILE_NAME_INFO);
   std::unique_ptr<uint8_t[]> data(new uint8_t[bytes]);
-  if (!GetFileInformationByHandleEx(
+  if (!GetFileInformationByHandleEx(first_pipe_instance_.get(),
-          first_pipe_instance_.get(), FileNameInfo, data.get(), bytes)) {
+                                    FileNameInfo,
+                                    data.get(),
+                                    static_cast<DWORD>(bytes))) {
     PLOG(FATAL) << "GetFileInformationByHandleEx";
   }
   FILE_NAME_INFO* file_name_info =
@@ -411,6 +413,16 @@ bool ExceptionHandlerServer::ServiceClientConnection(
       return true;
     }
 
+    case ClientToServerMessage::kPing: {
+      // No action required, the fact that the message was processed is
+      // sufficient.
+      ServerToClientMessage shutdown_response = {};
+      LoggingWriteFile(service_context.pipe(),
+                       &shutdown_response,
+                       sizeof(shutdown_response));
+      return false;
+    }
+
     case ClientToServerMessage::kRegister:
       // Handled below.
       break;

util/win/registration_protocol_win.cc

@@ -25,8 +25,8 @@
 namespace crashpad {
 
 bool SendToCrashHandlerServer(const base::string16& pipe_name,
-                              const crashpad::ClientToServerMessage& message,
+                              const ClientToServerMessage& message,
-                              crashpad::ServerToClientMessage* response) {
+                              ServerToClientMessage* response) {
   // Retry CreateFile() in a loop. If the handler isn’t actively waiting in
   // ConnectNamedPipe() on a pipe instance because it’s busy doing something
   // else, CreateFile() will fail with ERROR_PIPE_BUSY. WaitNamedPipe() waits
@@ -42,7 +42,7 @@ bool SendToCrashHandlerServer(const base::string16& pipe_name,
   // around the same time as its client, something external to this code must be
   // done to guarantee correct ordering. When the client starts the handler
   // itself, CrashpadClient::StartHandler() provides this synchronization.
-  for (int tries = 0;;) {
+  for (;;) {
     ScopedFileHANDLE pipe(
         CreateFile(pipe_name.c_str(),
                    GENERIC_READ | GENERIC_WRITE,
@@ -52,13 +52,12 @@ bool SendToCrashHandlerServer(const base::string16& pipe_name,
                    SECURITY_SQOS_PRESENT | SECURITY_IDENTIFICATION,
                    nullptr));
     if (!pipe.is_valid()) {
-      if (++tries == 5 || GetLastError() != ERROR_PIPE_BUSY) {
+      if (GetLastError() != ERROR_PIPE_BUSY) {
         PLOG(ERROR) << "CreateFile";
         return false;
       }
 
-      if (!WaitNamedPipe(pipe_name.c_str(), 1000) &&
+      if (!WaitNamedPipe(pipe_name.c_str(), NMPWAIT_WAIT_FOREVER)) {
-          GetLastError() != ERROR_SEM_TIMEOUT) {
         PLOG(ERROR) << "WaitNamedPipe";
         return false;
       }
@@ -75,7 +74,7 @@ bool SendToCrashHandlerServer(const base::string16& pipe_name,
     BOOL result = TransactNamedPipe(
         pipe.get(),
         // This is [in], but is incorrectly declared non-const.
-        const_cast<crashpad::ClientToServerMessage*>(&message),
+        const_cast<ClientToServerMessage*>(&message),
         sizeof(message),
         response,
         sizeof(*response),

util/win/registration_protocol_win.h

@@ -82,8 +82,14 @@ struct ClientToServerMessage {
   enum Type : uint32_t {
     //! \brief For RegistrationRequest.
     kRegister,
+
     //! \brief For ShutdownRequest.
     kShutdown,
+
+    //! \brief An empty message sent by the initial client in asynchronous mode.
+    //!     No data is required, this just confirms that the server is ready to
+    //!     accept client registrations.
+    kPing,
   } type;
 
   union {