Introduce RegistrationServer.

handler/win/registration_server.cc

+// 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 "handler/win/registration_server.h"
+
+#include <string.h>
+#include <vector>
+
+#include "base/logging.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/strings/utf_string_conversions.h"
+#include "client/registration_protocol_win.h"
+#include "util/stdlib/pointer_container.h"
+
+namespace crashpad {
+
+namespace {
+
+// Invokes GetNamedPipeClientProcessId if available (Vista+). Returns true if
+// the method is unavailable or completes successfully. Returns false if an
+// error occurs.
+// If the method is unavailable process_id will be set to 0.
+bool TryGetNamedPipeClientProcessId(HANDLE pipe, DWORD* process_id) {
+  bool result = false;
+
+  HMODULE kernel_dll = LoadLibrary(L"kernel32.dll");

[cpu_(ooo_6.6-7.5), 2015/05/21 22:16:28]

loadlibrary is far slower than GetModuleHandle, this is sadness that appeared
when windows invented SxS.

[erikwright (departed), 2015/05/22 01:22:04]

Done.

+  if (kernel_dll) {
+    decltype(GetNamedPipeClientProcessId)* proc =
+        reinterpret_cast<decltype(GetNamedPipeClientProcessId)*>(
+            GetProcAddress(kernel_dll, "GetNamedPipeClientProcessId"));

[cpu_(ooo_6.6-7.5), 2015/05/21 22:16:28]

the lines 39-41 are classy. I've been meaning to do this all over chromium (with
a wrapper). You are welcome if you what to tackle that.

[erikwright (departed), 2015/05/22 01:22:04]

Credit goes to Scott, who proposed this approach.

+    if (!proc) {
+      *process_id = 0;
+      result = true;
+    } else {
+      result = proc(pipe, process_id);
+    }
+    FreeLibrary(kernel_dll);
+  }
+  return result;
+}
+
+// Implements the state and state transitions for a single pipe instance.
+class PipeState {
+ public:
+  // Instantiates an instance that will operate on |pipe| and handle requests
+  // using |delegate|. The client must call Initialize() before clients may
+  // connect to this pipe.
+  PipeState(ScopedFileHANDLE pipe, RegistrationServer::Delegate* delegate);
+  ~PipeState();
+
+  // Places the pipe in the running state, ready to receive and process client
+  // connections. Returns true if successful, in which case the client must
+  // observe completion_event() and invoke OnCompletion() whenever it is
+  // signaled.
+  // Before destroying a running PipeState instance you must invoke Stop() and
+  // then wait for completion_event() to be signaled one last time.
+  bool Initialize();
+
+  // Cancels any pending asynchronous operations. After invoking this method you
+  // must wait for completion_event() to be signaled before destroying the
+  // instance.
+  void Stop();
+
+  // Returns an event handle that will be signaled whenever an asynchronous
+  // operation associated with this instance completes.
+  HANDLE completion_event() { return event_.get(); }
+
+  // Must be called by the client whenever completion_event() is signaled.
+  // Returns true if the pipe is still in the running state. Otherwise, a
+  // permanent failure has occurred and the instance may be immediately
+  // destroyed.
+  bool OnCompletion();
+
+ private:
+  typedef bool (PipeState::*AsyncCompletionHandler)(DWORD bytes_transferred);
+

[cpu_(ooo_6.6-7.5), 2015/05/21 22:16:28]

as a side note c+11 alias makes 86 look nicer, but I don't care either way.

[erikwright (departed), 2015/05/22 01:22:04]

Done.

+  // State transition handlers. Return true if the pipe is still valid.
+
+  bool OnConnectComplete(DWORD /* bytes_transferred */);
+  bool OnReadComplete(DWORD bytes_transferred);
+  bool OnWriteComplete(DWORD bytes_transferred);
+
+  // Pipe operations. Return true if the pipe is still valid.
+
+  // Prepares the pipe to accept a new client connecion.
+  bool IssueConnect();
+  // Reads into |request_|.
+  bool IssueRead();
+  // Writes from |response_|.
+  bool IssueWrite();
+  // Processes |request_| using |delegate_| and stores the result in
+  // |response_|.
+  bool HandleRequest();
+  // Closes the active connection and invokes IssueConnect().
+  bool ResetConnection();
+
+  RegistrationRequest request_;
+  RegistrationResponse response_;
+  // The state transition handler to be invoked when the active asynchronous
+  // operation completes.
+  AsyncCompletionHandler completion_handler_;
+  OVERLAPPED overlapped_;
+  ScopedKernelHANDLE event_;
+  ScopedFileHANDLE pipe_;
+  RegistrationServer::Delegate* delegate_;
+
+  DISALLOW_COPY_AND_ASSIGN(PipeState);
+};
+
+PipeState::PipeState(ScopedFileHANDLE pipe,
+                     RegistrationServer::Delegate* delegate)
+    : request_(),
+      response_(),
+      completion_handler_(nullptr),
+      overlapped_(),
+      event_(),
+      pipe_(pipe.Pass()),
+      delegate_(delegate) {
+}
+
+PipeState::~PipeState() {
+}
+
+bool PipeState::Initialize() {
+  DCHECK(!event_.is_valid());
+  DCHECK(pipe_.is_valid());
+
+  event_.reset(CreateEvent(nullptr, true, false, nullptr));
+
+  if (!event_.is_valid()) {
+    PLOG(ERROR) << "CreateEvent";
+  } else {
+    overlapped_.hEvent = event_.get();
+    if (IssueConnect())
+      return true;
+  }
+
+  overlapped_.hEvent = nullptr;
+  event_.reset();
+  pipe_.reset();
+  completion_handler_ = nullptr;
+
+  return false;
+}
+
+void PipeState::Stop() {
+  DCHECK(pipe_.is_valid());
+  if (!CancelIo(pipe_.get()))
+    PLOG(FATAL) << "CancelIo";
+}
+
+bool PipeState::OnCompletion() {
+  AsyncCompletionHandler completion_handler = completion_handler_;
+  completion_handler_ = nullptr;
+
+  DWORD bytes_transferred = 0;
+  BOOL success = GetOverlappedResult(pipe_.get(),
+                                     &overlapped_,
+                                     &bytes_transferred,
+                                     false);  // Do not wait.
+  if (!success)
+    PLOG(ERROR) << "GetOverlappedResult";
+
+  bool still_running = false;
+  if (!ResetEvent(event_.get())) {
+    PLOG(ERROR) << "ResetEvent";
+  } else if (!completion_handler) {
+    NOTREACHED();
+    still_running = ResetConnection();
+  } else if (!success) {
+    still_running = ResetConnection();
+  } else {
+    still_running = (this->*completion_handler)(bytes_transferred);
+  }
+
+  if (!still_running) {
+    overlapped_.hEvent = nullptr;
+    event_.reset();
+    pipe_.reset();
+    completion_handler_ = nullptr;
+  } else {
+    DCHECK(completion_handler_);
+  }
+
+  return still_running;
+}
+
+bool PipeState::OnConnectComplete(DWORD /* bytes_transferred */) {
+  return IssueRead();
+}
+
+bool PipeState::OnReadComplete(DWORD bytes_transferred) {
+  if (bytes_transferred != sizeof(request_)) {
+    LOG(ERROR) << "Invalid message size: " << bytes_transferred;
+    return ResetConnection();
+  } else {
+    return HandleRequest();
+  }
+}
+
+bool PipeState::OnWriteComplete(DWORD bytes_transferred) {
+  if (bytes_transferred != sizeof(response_)) {
+    LOG(ERROR) << "Incomplete write operation. Bytes written: "
+               << bytes_transferred;
+  }
+  return ResetConnection();
+}
+
+bool PipeState::IssueConnect() {
+  if (ConnectNamedPipe(pipe_.get(), &overlapped_)) {
+    return OnConnectComplete(0);  // bytes_transferred (ignored)
+  } else {
+    DWORD result = GetLastError();
+    if (result == ERROR_PIPE_CONNECTED) {
+      return OnConnectComplete(0);  // bytes_transferred (ignored)
+    } else if (result == ERROR_IO_PENDING) {
+      completion_handler_ = &PipeState::OnConnectComplete;
+      return true;
+    } else {
+      PLOG(ERROR) << "ConnectNamedPipe";
+      return false;
+    }
+  }

[cpu_(ooo_6.6-7.5), 2015/05/21 22:16:28]

this is tricky, ansync is how we did breakpad but we have always had weird
crashes, for example it is not clear at all when it returns anything other than
0?


"If the operation is asynchronous, ConnectNamedPipe returns immediately. If the
operation is still pending, the return value is zero and GetLastError returns
ERROR_IO_PENDING. (You can use the HasOverlappedIoCompleted macro to determine
when the operation has finished.) If the function fails, the return value is
zero and GetLastError returns a value other than ERROR_IO_PENDING or
ERROR_PIPE_CONNECTED.
If a client connects before the function is called, the function returns zero
and GetLastError returns ERROR_PIPE_CONNECTED. This can happen if a client
connects in the interval between the call to CreateNamedPipe and the call to
ConnectNamedPipe. In this situation, there is a good connection between client
and server, even though the function returns zero."

[erikwright (departed), 2015/05/22 01:22:04]

I agree that these comments seem to suggest that this method will always return
0 in async mode. However they also make it very clear what to do if the method
does not return 0 - in that case the operation is "[not] still pending" and we
can therefore consider it to be connected. So I believe that, according to the
documentation, the implementation of IssueConnect is unambiguously good (even if
line 222 may never execute).

+}
+
+bool PipeState::IssueRead() {
+  DWORD bytes_read = 0;
+  if (ReadFile(pipe_.get(),
+               &request_,
+               sizeof(request_),
+               &bytes_read,
+               &overlapped_)) {
+    return OnReadComplete(bytes_read);
+  } else if (GetLastError() == ERROR_IO_PENDING) {
+    completion_handler_ = &PipeState::OnReadComplete;
+    return true;
+  } else {
+    PLOG(ERROR) << "ReadFile";
+    return ResetConnection();
+  }
+}
+
+bool PipeState::IssueWrite() {
+  DWORD bytes_written = 0;
+  if (WriteFile(pipe_.get(),
+                &response_,
+                sizeof(response_),
+                &bytes_written,
+                &overlapped_)) {
+    return OnWriteComplete(bytes_written);
+  } else if (GetLastError() == ERROR_IO_PENDING) {
+    completion_handler_ = &PipeState::OnWriteComplete;
+    return true;
+  } else {
+    PLOG(ERROR) << "WriteFile";
+    return ResetConnection();
+  }
+}
+
+bool PipeState::HandleRequest() {
+  // On Vista+ we can verify that the client is who they claim to be, thus
+  // preventing arbitrary processes from having us duplicate handles into other
+  // processes.
+  DWORD real_client_process_id = 0;
+  if (TryGetNamedPipeClientProcessId(pipe_.get(), &real_client_process_id)) {
+    if (real_client_process_id != 0 &&
+        real_client_process_id != request_.client_process_id) {
+      LOG(ERROR) << "Client process ID from request ("
+                 << request_.client_process_id
+                 << ") does not match pipe client process ID ("
+                 << real_client_process_id << ").";
+      return ResetConnection();
+    }
+  }
+
+  ScopedKernelHANDLE client_process(
+      OpenProcess(PROCESS_ALL_ACCESS, false, request_.client_process_id));
+  if (!client_process.is_valid()) {
+    if (ImpersonateNamedPipeClient(pipe_.get())) {
+      client_process.reset(
+          OpenProcess(PROCESS_ALL_ACCESS, false, request_.client_process_id));
+      RevertToSelf();
+    }
+  }
+
+  if (!client_process.is_valid()) {
+    LOG(ERROR) << "Failed to open client process.";
+    return ResetConnection();
+  }
+
+  memset(&response_, 0, sizeof(response_));
+
+  HANDLE request_report_event = nullptr;
+  HANDLE report_complete_event = nullptr;
+
+  if (!delegate_->RegisterClient(client_process.Pass(),
+                                 request_.crashpad_info_address,
+                                 &request_report_event,
+                                 &report_complete_event)) {
+    return ResetConnection();
+  }
+
+  // A handle has at most 32 significant bits, though its type is void*. Thus we
+  // truncate it here. An interesting exception is INVALID_HANDLE_VALUE, which
+  // is '-1'. It is still safe to truncate it from 0xFFFFFFFFFFFFFFFF to
+  // 0xFFFFFFFF, but a 64-bit client receiving that value must correctly sign
+  // extend it.
+  response_.request_report_event =
+      reinterpret_cast<uint32_t>(request_report_event);
+  response_.report_complete_event =
+      reinterpret_cast<uint32_t>(report_complete_event);
+  return IssueWrite();
+}
+
+bool PipeState::ResetConnection() {
+  memset(&request_, 0, sizeof(request_));
+
+  if (!DisconnectNamedPipe(pipe_.get())) {
+    PLOG(ERROR) << "DisconnectNamedPipe";
+    return false;
+  } else {
+    return IssueConnect();
+  }
+}
+
+}  // namespace
+
+RegistrationServer::RegistrationServer() : stop_event_() {
+  stop_event_.reset(CreateEvent(nullptr, false, false, nullptr));
+  DPCHECK(stop_event_.is_valid());
+}
+
+RegistrationServer::~RegistrationServer() {
+}
+
+bool RegistrationServer::Run(const base::string16& pipe_name,
+                             Delegate* delegate) {
+  if (!stop_event_.is_valid()) {
+    LOG(ERROR) << "Failed to create stop_event_.";
+    return false;
+  }
+
+  PointerVector<PipeState> pipes;
+  std::vector<HANDLE> handles;
+
+  const int kNumPipes = 3;
+
+  // Create the named pipes.
+  for (int i = 0; i < kNumPipes; ++i) {
+    DWORD open_mode = PIPE_ACCESS_DUPLEX | FILE_FLAG_OVERLAPPED;
+    if (pipes.size() == 0)
+      open_mode |= FILE_FLAG_FIRST_PIPE_INSTANCE;
+    ScopedFileHANDLE pipe(
+        CreateNamedPipe(pipe_name.c_str(),
+                        open_mode,
+                        PIPE_TYPE_MESSAGE | PIPE_READMODE_MESSAGE | PIPE_WAIT,
+                        kNumPipes,
+                        512,  // nOutBufferSize
+                        512,  // nInBufferSize
+                        20,  // nDefaultTimeOut
+                        nullptr));  // lpSecurityAttributes
+    if (pipe.is_valid()) {
+      scoped_ptr<PipeState> pipe_state(new PipeState(pipe.Pass(), delegate));
+      if (pipe_state->Initialize()) {
+        pipes.push_back(pipe_state.release());
+        handles.push_back(pipes.back()->completion_event());
+      }
+    } else {
+      PLOG(ERROR) << "CreateNamedPipe";
+    }
+  }
+
+  if (pipes.size() == 0) {
+    LOG(ERROR) << "Failed to initialize any pipes.";
+    return false;
+  }
+
+  delegate->OnStarted();
+
+  // Add stop_event_ to the list of events we will observe.
+  handles.push_back(stop_event_.get());
+
+  bool stopped = false;
+
+  // Run the main loop, dispatching completion event signals to the pipe
+  // instances.
+  while (true) {

[cpu_(ooo_6.6-7.5), 2015/05/21 22:55:58]

we also lost the ability to service two clients at the same time? because we are
single threaded? 

I guess you are trading off that over the complexity of having 3 threads (one
master two slave)?

'cause we have 3 pipes but if we have 3 processes connecting we still have more
latency. Maybe that is the tradeoff you want to make. Let me know.

[erikwright (departed), 2015/05/22 01:22:04]

Single-threaded and async IO are not mutually exclusive. But in my opinion
multi-threaded is no longer required. Three separate clients may still connect
simultaneously. Request handling will be interleaved, meaning that a delay in
one client will not affect the others. The server only operates on a request
after the data is already available and is not blocked while the client is
processing a response.

It would be trivial to have the separate pipes operate on separate threads, but
I see no reason to think it will be meaningfully faster. I would maintain this
simple implementation until benchmarking demonstrated differently.

+    DWORD wait_result = WaitForMultipleObjects(
+        static_cast<DWORD>(handles.size()), handles.data(), false, INFINITE);
+    if (wait_result >= WAIT_OBJECT_0 &&
+        wait_result < WAIT_OBJECT_0 + pipes.size()) {
+      int index = wait_result - WAIT_OBJECT_0;
+      // Handle a completion event.
+      if (!pipes[index]->OnCompletion()) {
+        pipes.erase(pipes.begin() + index);
+        handles.erase(handles.begin() + index);
+      }
+      if (pipes.size())
+        continue;
+      // Exit due to all pipes having failed.
+    } else if (wait_result == WAIT_OBJECT_0 + pipes.size()) {
+      // Exit due to stop_event_.
+      stopped = true;
+    } else if (wait_result == WAIT_FAILED) {
+      // Exit due to error.
+      PLOG(ERROR) << "WaitForMultipleObjects";
+    } else {
+      // Exit due to unexpected return code.
+      NOTREACHED();
+    }
+    break;
+  }
+
+  // Remove |stop_event_| from the wait list.
+  handles.pop_back();
+
+  // Cancel any ongoing asynchronous operations.
+  for (auto& pipe : pipes) {
+    pipe->Stop();
+  }
+

[cpu_(ooo_6.6-7.5), 2015/05/21 22:55:58]

How does this work? I mean why do we believe we get handle signaling on 434? and
if so why just one per event?

what if somebody connected at 424?

If I am not mistaken in chrome we had issues also keeping alive the overlapped
structure long enough because the completion can arrive quite late. The current
scheme in chrome involves passing the ownership of the overlapped to some global
entity and possibly leaking the overlapped structures. 

[erikwright (departed), 2015/05/22 01:22:04]

In PipeState::Stop we call CancelIo for the pipe. This will cancel the operation
that is in progress (causing the operation to complete). When the operation
completes, either due to cancellation or because it actually completed (i.e.
someone connected or data became available) the event will be signaled.

There is only ever one operation going on per pipe at a time. The event is
signaled when that operation completes. CancelIo triggers the associated event
indirectly, by causing the operation to complete (if and only if it was not
already complete).

+  // Wait until all of the pipe instances are ready to be destroyed.
+  DWORD wait_result = WaitForMultipleObjects(
+      static_cast<DWORD>(handles.size()), handles.data(), true, INFINITE);
+  PCHECK(wait_result != WAIT_FAILED);
+  DCHECK_GE(wait_result, WAIT_OBJECT_0);
+  DCHECK_LT(wait_result, WAIT_OBJECT_0 + handles.size());
+
+  return stopped;
+}
+
+void RegistrationServer::Stop() {
+  if (!SetEvent(stop_event_.get()))
+    PLOG(FATAL) << "SetEvent";
+}
+
+}  // namespace crashpad