win: Crash handler server

util/win/exception_handler_server.cc

Index: util/win/exception_handler_server.cc
diff --git a/util/win/exception_handler_server.cc b/util/win/exception_handler_server.cc
new file mode 100644
index 0000000000000000000000000000000000000000..f8b765a4ca858b6afcbfa909061eca93674d246b
--- /dev/null
+++ b/util/win/exception_handler_server.cc
@@ -0,0 +1,379 @@
+// Copyright 2015 The Crashpad Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#include "util/win/exception_handler_server.h"
+
+#include "base/logging.h"
+#include "base/strings/stringprintf.h"
+#include "base/strings/utf_string_conversions.h"
+#include "minidump/minidump_file_writer.h"
+#include "snapshot/crashpad_info_client_options.h"
+#include "snapshot/win/process_snapshot_win.h"
+#include "util/file/file_writer.h"
+#include "util/misc/tri_state.h"
+#include "util/misc/uuid.h"
+#include "util/win/registration_protocol_win.h"
+
+namespace crashpad {
+
+namespace {
+
+//! \brief Context information for the named pipe handler threads.
+class PipeServiceContext {
+ public:
+  PipeServiceContext(HANDLE port,
+                     HANDLE pipe,
+                     ExceptionHandlerServer::Delegate* delegate,
+                     base::Lock* clients_lock,
+                     std::set<internal::ClientData*>* clients)
+      : port_(port),
+        pipe_(pipe),
+        delegate_(delegate),
+        clients_lock_(clients_lock),
+        clients_(clients) {}
+
+  HANDLE port() { return port_; }
+  HANDLE pipe() { return pipe_.get(); }
+  ExceptionHandlerServer::Delegate* delegate() { return delegate_; }
+  base::Lock* clients_lock() { return clients_lock_; }
+  std::set<internal::ClientData*>* clients() { return clients_; }
+
+ private:
+  HANDLE port_;  // weak
+  ScopedKernelHANDLE pipe_;
+  ExceptionHandlerServer::Delegate* delegate_;  // weak
+  base::Lock* clients_lock_;  // weak
+  std::set<internal::ClientData*>* clients_;  // weak
+
+  DISALLOW_COPY_AND_ASSIGN(PipeServiceContext);
+};
+
+decltype(GetNamedPipeClientProcessId)* GetNamedPipeClientProcessIdFunction() {
+  static decltype(GetNamedPipeClientProcessId)* func =
+      reinterpret_cast<decltype(GetNamedPipeClientProcessId)*>(GetProcAddress(
+          GetModuleHandle(L"kernel32.dll"), "GetNamedPipeClientProcessId"));
+  return func;
+}
+
+HANDLE DuplicateEvent(HANDLE process, HANDLE event) {
+  HANDLE handle;
+  if (DuplicateHandle(GetCurrentProcess(),
+                      event,
+                      process,
+                      &handle,
+                      SYNCHRONIZE | EVENT_MODIFY_STATE,
+                      false,
+                      0)) {
+    return handle;
+  }
+  return nullptr;
+}
+
+}  // namespace
+
+namespace internal {
+
+//! \brief The context data for registered threadpool waits.
+//!
+//! This object must be created and destroyed on the main thread. Access must be
+//! guarded by use of the lock() with the exception of the threadpool wait
+//! variables which are accessed only by the main thread.
+class ClientData {
+ public:
+  ClientData(HANDLE port,
+             ExceptionHandlerServer::Delegate* delegate,
+             ScopedKernelHANDLE process,
+             WinVMAddress exception_information_address,
+             WAITORTIMERCALLBACK dump_request_callback,
+             WAITORTIMERCALLBACK process_end_callback)
+      : dump_request_thread_pool_wait_(INVALID_HANDLE_VALUE),
+        process_end_thread_pool_wait_(INVALID_HANDLE_VALUE),
+        lock_(),
+        port_(port),
+        delegate_(delegate),
+        dump_requested_event_(
+            CreateEvent(nullptr, false /* auto reset */, false, nullptr)),
+        process_(process.Pass()),
+        exception_information_address_(exception_information_address) {
+    RegisterThreadPoolWaits(dump_request_callback, process_end_callback);
+  }
+
+  ~ClientData() {
+    // It is important that this only access the threadpool waits (it's called
+    // from the main thread) until the waits are unregistered, to ensure that
+    // any outstanding callbacks are complete.
+    UnregisterThreadPoolWaits();
+  }
+
+  base::Lock* lock() { return &lock_; }
+  HANDLE port() { return port_; }
+  ExceptionHandlerServer::Delegate* delegate() { return delegate_; }
+  HANDLE dump_requested_event() { return dump_requested_event_.get(); }
+  WinVMAddress exception_information_address() const {
+    return exception_information_address_;
+  }
+  HANDLE process() { return process_.get(); }
+
+ private:
+  void RegisterThreadPoolWaits(WAITORTIMERCALLBACK dump_request_callback,
+                               WAITORTIMERCALLBACK process_end_callback) {
+    if (!RegisterWaitForSingleObject(&dump_request_thread_pool_wait_,
+                                     dump_requested_event_.get(),
+                                     dump_request_callback,
+                                     this,
+                                     INFINITE,
+                                     WT_EXECUTEDEFAULT)) {
+      LOG(ERROR) << "RegisterWaitForSingleObject dump requested";
+    }
+
+    if (!RegisterWaitForSingleObject(&process_end_thread_pool_wait_,
+                                     process_.get(),
+                                     process_end_callback,
+                                     this,
+                                     INFINITE,
+                                     WT_EXECUTEONLYONCE)) {
+      LOG(ERROR) << "RegisterWaitForSingleObject process end";
+    }
+  }
+
+  // This blocks until outstanding calls complete so that we know it's safe to
+  // delete this object. Because of this, it must be executed on the main
+  // thread, not a threadpool thread.
+  void UnregisterThreadPoolWaits() {
+    UnregisterWaitEx(dump_request_thread_pool_wait_, INVALID_HANDLE_VALUE);
+    dump_request_thread_pool_wait_ = INVALID_HANDLE_VALUE;
+    UnregisterWaitEx(process_end_thread_pool_wait_, INVALID_HANDLE_VALUE);
+    process_end_thread_pool_wait_ = INVALID_HANDLE_VALUE;
+  }
+
+  // These are only accessed on the main thread.
+  HANDLE dump_request_thread_pool_wait_;
+  HANDLE process_end_thread_pool_wait_;
+
+  base::Lock lock_;
+  // Access to these fields must be guarded by lock_.
+  HANDLE port_;  // weak
+  ExceptionHandlerServer::Delegate* delegate_;  // weak
+  ScopedKernelHANDLE dump_requested_event_;
+  ScopedKernelHANDLE process_;
+  WinVMAddress exception_information_address_;
+
+  DISALLOW_COPY_AND_ASSIGN(ClientData);
+};
+
+}  // namespace internal
+
+ExceptionHandlerServer::Delegate::~Delegate() {
+}
+
+ExceptionHandlerServer::ExceptionHandlerServer(Delegate* delegate)
+    : delegate_(delegate),
+      port_(CreateIoCompletionPort(INVALID_HANDLE_VALUE, nullptr, 0, 1)),
+      clients_lock_(),
+      clients_() {
+}
+
+ExceptionHandlerServer::~ExceptionHandlerServer() {
+}
+
+void ExceptionHandlerServer::Run(const std::string& pipe_name) {
+  // We create two pipe instances, so that there's one listening while the
+  // PipeServiceProc is processing a registration.
+  const int kPipeInstances = 2;
+  ScopedKernelHANDLE thread_handles[kPipeInstances];
+  for (int i = 0; i < kPipeInstances; ++i) {
+    HANDLE pipe =
+        CreateNamedPipe(base::UTF8ToUTF16(pipe_name).c_str(),

[Mark Mentovai, 2015/08/26 21:40:28]

UTF8ToUTF16 outside of the loop?

[scottmg, 2015/08/27 01:04:38]

Done.

+                        PIPE_ACCESS_DUPLEX,
+                        PIPE_TYPE_MESSAGE | PIPE_READMODE_MESSAGE | PIPE_WAIT,
+                        kPipeInstances,
+                        512,
+                        512,
+                        0,
+                        nullptr);
+
+    // Ownership of this object (and the pipe instance) is given to the new
+    // thread. We close the thread handles at the end of the scope, but need not
+    // wait to join them. They clean up the context object and the pipe

[Mark Mentovai, 2015/08/26 21:40:28]

“Need not wait to join them”—really? Doesn’t that risk tearing them down while
they might still be in the middle of doing something? This would happen if Run()
returns and Run()’s caller (probably something like main() or close to it) just
decides to exit while there’s still something happening on a thread.

(after reading a bit more) Oh, I see you’re doing a logical join with the client
data. I don’t see why you wouldn’t do a real join, though.

[scottmg, 2015/08/27 01:04:38]

Done.

+    // instance on termination.
+    PipeServiceContext* context = new PipeServiceContext(
+        port_.get(), pipe, delegate_, &clients_lock_, &clients_);
+    thread_handles[i].reset(
+        CreateThread(nullptr, 0, &PipeServiceProc, context, 0, nullptr));
+  }
+
+  delegate_->OnStarted();
+
+  // This is the main loop of the server. Most work is done on the threadpool,
+  // other than process end handing which is posted back to this main thread, as

[Mark Mentovai, 2015/08/26 21:40:28]

process end handing?

[scottmg, 2015/08/27 01:04:38]

Done.

+  // we must unregister the threadpool waits here.
+  for (;;) {
+    OVERLAPPED* ov = nullptr;
+    ULONG_PTR key = 0;
+    DWORD bytes = 0;
+    GetQueuedCompletionStatus(port_.get(), &bytes, &key, &ov, INFINITE);
+    if (!key) {
+      // Shutting down.
+      break;
+    }
+
+    // Otherwise, this is a request to unregister and destroy the given client.
+    // delete'ing the ClientData blocks in UnregisterWaitEx to ensure all
+    // outstanding threadpool waits are complete. This is important because the
+    // process handle can be signalled *before* the dump request is signalled.
+    internal::ClientData* client = reinterpret_cast<internal::ClientData*>(key);
+    {
+      base::AutoLock lock(clients_lock_);
+      clients_.erase(client);
+    }
+    delete client;
+  }
+
+  // Signal to the named pipe instances that they should terminate.
+  for (int i = 0; i < kPipeInstances; ++i) {
+    RegistrationRequest shutdown_request = {0};
+    shutdown_request.client_process_id = GetCurrentProcessId();
+    RegistrationResponse response;
+    internal::RegisterWithCrashHandlerServer(
+        base::UTF8ToUTF16(pipe_name), shutdown_request, &response);
+  }
+
+  // Deleting ClientData does a blocking wait until the threadpool executions
+  // have terminated when unregistering them.
+  {
+    base::AutoLock lock(clients_lock_);
+    for (auto* client : clients_)
+      delete client;
+    clients_.clear();
+  }
+
+  delegate_->OnShutdown();

[Mark Mentovai, 2015/08/26 21:40:28]

Since this happens as the last thing in Run(), is it useful to have as a
delegate method, or would you just have Run()’s caller do the thing that
OnShutdown() would do? It’s hard to envision how you want it used, but certainly
in the test, it’s just nonfunctional bloat.

(I have a similar question about OnStarted(), although I’m expecting that there
are more legitimate cases for it to exist than OnShutdown()).

[scottmg, 2015/08/27 01:04:37]

Remove OnShutdown. Started is useful to know that the server is ready to accept
requests in tests, mostly.

+}
+
+void ExceptionHandlerServer::Stop() {
+  // Post a null key (third argument) to trigger shutdown.
+  PostQueuedCompletionStatus(port_.get(), 0, 0, nullptr);
+}
+
+// static
+DWORD __stdcall ExceptionHandlerServer::PipeServiceProc(void* ctx) {
+  PipeServiceContext* service_context =
+      reinterpret_cast<PipeServiceContext*>(ctx);
+
+  for (;;) {
+    // Connect can false if the client connects before we do. Continue and

[Mark Mentovai, 2015/08/26 21:40:28]

What’s this comment mean?

[scottmg, 2015/08/27 01:04:37]

Removed, and made it check for the condition the comment was discussing.

+    // attempt a read anyway.
+    ConnectNamedPipe(service_context->pipe(), nullptr);
+    RegistrationRequest request;
+    if (!LoggingReadFile(service_context->pipe(), &request, sizeof(request)))
+      goto AbortAndResumeListening;

[Mark Mentovai, 2015/08/26 21:40:28]

Can you restructure all of this to avoid “goto”? Probably the best way to do it
would be to hoist the loop body into a helper function.

[scottmg, 2015/08/27 01:04:38]

Done.

+
+    // Shutdown request.
+    if (request.client_process_id == GetCurrentProcessId()) {

[Mark Mentovai, 2015/08/26 21:40:28]

This lets any evil client crash the party for everyone simply by lying about its
process ID. No good.

I actually don’t think we should recycle the registration message to use to
indicate shutdown. The shutdown request comes from the same process that’s
hosting this thread, isn’t there a better way to indicate a shutdown request
within the same process?

[scottmg, 2015/08/27 01:04:37]

Not that I know of (a better way). AFAIK, we'd have to give up the benefit of
having it be a plain synchronous IO loop, which makes this code a lot more
straightforward. There's CancelSynchronousIO, but it's Vista+ (and can be tricky
because it just cancels whatever's next so you're always racing with an IO call
completing.)

Carlos or Justin: do you know of any better way to abort this loop during
shutdown?

In practice this really only for tests -- in production the crash handler could
just as well _exit(0) (or otherwise leak/ignore these threads until it did any
other cleanup).

[scottmg, 2015/08/27 04:18:00]

WRT this first part, added a shared token so the request can't be spoofed.

I also added a pseudo-different message type so it's more clear (?) it's being
used for a different category of message, but I didn't go as far as adding type
information to the message as it seems awkward for the client case. (I'm not
sure if the extra message type makes in more clear or more confusing though
tbh.) This is still the best way I know of to do shutdown of synchronous IO
though.

+      RegistrationResponse shutdown_response = {0};
+      LoggingWriteFile(service_context->pipe(),
+                       &shutdown_response,
+                       sizeof(shutdown_response));
+      break;
+    }
+
+    decltype(GetNamedPipeClientProcessId)* get_named_pipe_client_process_id =
+        GetNamedPipeClientProcessIdFunction();
+    if (get_named_pipe_client_process_id) {
+      // GetNamedPipeClientProcessId is only available on Vista+.
+      DWORD real_pid = 0;
+      if (get_named_pipe_client_process_id(service_context->pipe(),
+                                           &real_pid) &&
+          request.client_process_id != real_pid) {
+        LOG(ERROR) << "forged client pid";

[Mark Mentovai, 2015/08/26 21:40:28]

Log the evil real pid too?

[scottmg, 2015/08/27 01:04:38]

Done.

+        goto AbortAndResumeListening;
+      }
+    }
+
+    // We attempt to open the process as us. This is the main case that should
+    // almost always succeed as the server will generally be more privileged. If
+    // we're running as a different user, it may be that we will fail to open
+    // the process, but the client will be able to, so we make a second attempt
+    // having impersonated the client.

[Mark Mentovai, 2015/08/26 21:40:28]

Can we have the client pass its own process handle in the registration request?

[scottmg, 2015/08/27 01:04:37]

I have to defer to the Windows handle people again here (Carlos or Justin?)

The client could do the OpenProcess() I think since this would be pre-sandbox.
It seems like we'd have to trust the HANDLE we get then though, because we don't
have a way to ask the system if the other end of the named pipe really is that
HANDLE. (I don't think?)

+    HANDLE client_process =
+        OpenProcess(PROCESS_ALL_ACCESS, false, request.client_process_id);
+    if (!client_process) {
+      if (!ImpersonateNamedPipeClient(service_context->pipe())) {
+        PLOG(ERROR) << "ImpersonateNamedPipeClient";
+        goto AbortAndResumeListening;
+      }
+      HANDLE client_process =
+          OpenProcess(PROCESS_ALL_ACCESS, false, request.client_process_id);
+      RevertToSelf();

[Mark Mentovai, 2015/08/26 21:40:28]

On the fence about a scoper to ensure this happens. But also, this is a pretty
important thing to get right, maybe it should be PCHECK()ed.

[scottmg, 2015/08/27 01:04:38]

PCHECKd.

+      if (!client_process) {
+        LOG(ERROR) << "failed to open " << request.client_process_id;

[Mark Mentovai, 2015/08/26 21:40:28]

I’d say PLOG, but that intervening RevertToSelf() might mess that up.

[scottmg, 2015/08/27 01:04:37]

Right.

+        goto AbortAndResumeListening;
+      }
+    }
+
+    internal::ClientData* client;
+    {
+      base::AutoLock lock(*service_context->clients_lock());
+      client = new internal::ClientData(service_context->port(),
+                                        service_context->delegate(),
+                                        ScopedKernelHANDLE(client_process),
+                                        request.exception_information,
+                                        &OnDumpEvent,
+                                        &OnProcessEnd);
+      service_context->clients()->insert(client);
+    }
+
+    // Duplicate the events back to the client so they can request a dump.
+    RegistrationResponse response;
+    response.request_report_event = reinterpret_cast<uint32_t>(
+        DuplicateEvent(client->process(), client->dump_requested_event()));
+
+    if (!LoggingWriteFile(service_context->pipe(), &response, sizeof(response)))
+      goto AbortAndResumeListening;
+
+  AbortAndResumeListening:
+    DisconnectNamedPipe(service_context->pipe());
+  }
+
+  delete service_context;
+
+  return 0;
+}
+
+// static
+void __stdcall ExceptionHandlerServer::OnDumpEvent(void* ctx, BOOLEAN) {
+  // This function is executed on the thread pool.
+  internal::ClientData* client = reinterpret_cast<internal::ClientData*>(ctx);
+  base::AutoLock lock(*client->lock());
+
+  // Capture the exception.
+  client->delegate()->OnException(client->process(),
+                                  client->exception_information_address());
+
+  // Terminate the client with a known exit code.

[Mark Mentovai, 2015/08/26 21:40:28]

Is this what the system would do in the absence of any exception handler? I
thought it would terminate it using the exception code as the exit code.

[scottmg, 2015/08/27 01:04:38]

Yes, you're right. I was sort of thinking that this wasn't really the same as
doing nothing, so using a special exit code makes sense. But maybe there's some
useful information there.

Switched to having the delegate say what it thinks the exit code should be, and
then the ExceptionCode from there in the normal case.

+  const UINT kCrashExitCode = 0xffff7002;
+  TerminateProcess(client->process(), kCrashExitCode);
+}
+
+// static
+void __stdcall ExceptionHandlerServer::OnProcessEnd(void* ctx, BOOLEAN) {
+  // This function is executed on the thread pool.
+  internal::ClientData* client = reinterpret_cast<internal::ClientData*>(ctx);
+  base::AutoLock lock(*client->lock());
+
+  // Post back to the main thread to have it delete this client record.
+  PostQueuedCompletionStatus(client->port(), 0, ULONG_PTR(client), nullptr);
+}
+
+}  // namespace crashpad