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 <cstring>

[scottmg, 2015/05/20 18:53:09]

string.h

[erikwright (departed), 2015/05/20 20:54:14]

Done.

+#include <vector>
+
+#include "base/logging.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(TEXT("Kernel32.dll"));

[scottmg, 2015/05/20 18:53:09]

We only build with _UNICODE, just L" instead, and lower case 'k' on kernel32 for
consistency with other code.

[erikwright (departed), 2015/05/20 20:54:13]

Done.

+  if (kernel_dll) {
+    typedef BOOL(WINAPI * GetNamedPipeClientProcessIdProc)(HANDLE, PULONG);

[scottmg, 2015/05/20 18:53:09]

We've been using decltype instead (since the prototype is available). See e.g.
NtQueryInformationProcess in util/win/process_info.cc

[erikwright (departed), 2015/05/20 20:54:14]

Done.

+    GetNamedPipeClientProcessIdProc proc =
+        reinterpret_cast<GetNamedPipeClientProcessIdProc>(
+            GetProcAddress(kernel_dll, "GetNamedPipeClientProcessId"));
+    if (!proc) {
+      *process_id = 0;
+      result = true;
+    } else {
+      result = (proc)(pipe, process_id);

[scottmg, 2015/05/20 18:53:09]

Remove extra () around `proc`.

[erikwright (departed), 2015/05/20 20:54:13]

Done.

+    }
+    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|. Initiates an asynchronous ConnectNamedPipe before
+  // returning.
+  // The client must observe completion_event() and invoke OnCompletion()
+  // whenever it is signaled.
+  // Before destroying a PipeState instance you must invoke Stop() and then wait
+  // for completion_event() to be signaled one last time.
+  PipeState(ScopedFileHANDLE pipe, RegistrationServer::Delegate* delegate);
+  ~PipeState();
+
+  // Cancels any pending asynchronous operations.
+  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.
+  void OnCompletion();
+
+ private:
+  typedef void (PipeState::*AsyncCompletionHandler)(DWORD bytes_transferred);
+
+  // State transition handlers.
+  void OnConnectComplete(DWORD /* bytes_transferred */);
+  void OnReadComplete(DWORD bytes_transferred);
+  void OnWriteComplete(DWORD bytes_transferred);
+
+  // Invokes ConnectNamedPipe.
+  void IssueConnect();
+  // Invokes ReadFile.

[scottmg, 2015/05/20 18:53:09]

These function comments don't seem very useful. (Just removing them is fine if
there's nothing to say.)

[erikwright (departed), 2015/05/20 20:54:14]

Done.

[scottmg, 2015/05/21 02:32:36]

Sorry, I meant just IssueRead and IssueWrite. I think the HandleRequest and
ResetConnection ones were valuable.

[erikwright (departed), 2015/05/21 15:12:37]

Done.

+  void IssueRead();
+  // Invokes WriteFile.
+  void IssueWrite();
+
+  // Handles the request in request_ using delegate_;
+  void HandleRequest();
+
+  // Disconnects from the current client and invokes IssueConnect.
+  void 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) {
+  event_.reset(CreateEvent(nullptr, TRUE, FALSE, nullptr));

[scottmg, 2015/05/20 18:53:09]

true and false (even though they're BOOL, not bool) and elsewhere.

[erikwright (departed), 2015/05/20 20:54:14]

Done.

+  if (!event_.is_valid()) {
+    PLOG(ERROR);

[scottmg, 2015/05/20 18:53:09]

<< "failed to create event" or just << "CreateEvent", and elsewhere

[erikwright (departed), 2015/05/20 20:54:14]

I received some guidance in Chromium to not include verbose non-Debug log
messages. Reasoning:

Whatever string I put here will be baked into the code. On the other hand, with
no string a line number and filename will be output, and we have the source.

[scottmg, 2015/05/21 02:32:36]

Yeah, it's a divergence from Chromium. :(

Mark has generally requested that these logs be somewhat more informative with a
view to capturing and including them in dumps. Tracking them back to the line
number is plausible, but could be somewhat tedious as there's likely to be code
shifts amongst versions and it could get quite complicated to correlate in that
case.

[erikwright (departed), 2015/05/21 15:12:37]

Done.

+    pipe_.reset();
+  } else {
+    overlapped_.hEvent = event_.get();
+    IssueConnect();
+  }
+
+  DCHECK(!pipe_.get() || completion_handler_);
+}
+
+PipeState::~PipeState() {
+}
+
+void PipeState::Stop() {
+  if (pipe_.is_valid()) {
+    if (!CancelIo(pipe_.get()))
+      PLOG(FATAL);
+  } else {
+    SetEvent(event_.get());

[scottmg, 2015/05/20 18:53:09]

I think this path would execute if the event failed to be created in the
constructor.

[erikwright (departed), 2015/05/20 20:54:14]

I fixed things so the client now knows when it enters an invalid state. So Stop
should never be called in that condition

+  }
+}
+
+void 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
+  ResetEvent(event_.get());
+
+  if (!completion_handler) {
+    NOTREACHED();
+    ResetConnection();
+  } else if (!success) {
+    PLOG(ERROR);
+    ResetConnection();
+  } else {
+    (this->*completion_handler)(bytes_transferred);
+  }
+
+  DCHECK(!pipe_.get() || completion_handler_);
+}
+
+void PipeState::OnConnectComplete(DWORD /* bytes_transferred */) {
+  IssueRead();
+}
+
+void PipeState::OnReadComplete(DWORD bytes_transferred) {
+  if (bytes_transferred != sizeof(request_)) {
+    LOG(ERROR) << "Invalid message size: " << bytes_transferred;
+    ResetConnection();
+  } else {
+    HandleRequest();
+  }
+}
+
+void PipeState::OnWriteComplete(DWORD bytes_transferred) {
+  if (bytes_transferred != sizeof(response_)) {
+    LOG(ERROR) << "Incomplete write operation. Bytes written: "
+               << bytes_transferred;
+  }
+  ResetConnection();
+}
+
+void PipeState::IssueConnect() {
+  if (ConnectNamedPipe(pipe_.get(), &overlapped_)) {
+    OnConnectComplete(0);  // bytes_transferred (ignored)
+  } else {
+    DWORD result = GetLastError();
+    if (result == ERROR_PIPE_CONNECTED) {
+      OnConnectComplete(0);  // bytes_transferred (ignored)
+    } else if (result == ERROR_IO_PENDING){
+      completion_handler_ = &PipeState::OnConnectComplete;
+    } else {
+      PLOG(ERROR);
+      pipe_.reset();
+      completion_handler_ = nullptr;
+    }
+  }
+}
+
+void PipeState::IssueRead() {
+  DWORD bytes_read = 0;
+  if (ReadFile(pipe_.get(), &request_, sizeof(request_), &bytes_read, &overlapped_)) {

[scottmg, 2015/05/20 18:53:09]

80 col

[erikwright (departed), 2015/05/20 20:54:14]

Done.

+    OnReadComplete(bytes_read);
+  } else if (GetLastError() == ERROR_IO_PENDING) {
+    completion_handler_ = &PipeState::OnReadComplete;
+  } else {
+    PLOG(ERROR);
+    ResetConnection();
+  }
+}
+
+void PipeState::IssueWrite() {
+  DWORD bytes_written = 0;
+  if (WriteFile(pipe_.get(), &response_, sizeof(response_), &bytes_written, &overlapped_)) {

[scottmg, 2015/05/20 18:53:09]

80 col

[erikwright (departed), 2015/05/20 20:54:14]

Done.

+    OnWriteComplete(bytes_written);
+  } else if (GetLastError() == ERROR_IO_PENDING) {
+    completion_handler_ = &PipeState::OnWriteComplete;
+  } else {
+    PLOG(ERROR);
+    ResetConnection();
+  }
+}
+
+void PipeState::HandleRequest() {
+  DWORD real_client_process_id = 0;
+  if (TryGetNamedPipeClientProcessId(pipe_.get(), &real_client_process_id)) {

[scottmg, 2015/05/20 18:53:09]

What's the upside of doing this when it doesn't work on XP? (in a comment if it
makes sense)

[erikwright (departed), 2015/05/20 20:54:14]

Added a comment. Carlos implied there was a minor hypothetical attack vector
here.

+    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 << ").";
+      ResetConnection();
+      return;
+    }
+  }
+
+  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.";
+    ResetConnection();
+    return;
+  }
+
+  memset(&response_, 0, sizeof(response_));
+
+  if (!delegate_->RegisterClient(client_process.Pass(),
+                                 request_.crashpad_info_address,
+                                 &response_.request_report_event,
+                                 &response_.report_complete_event)) {
+    ResetConnection();

[scottmg, 2015/05/20 18:53:09]

LOG(ERROR) here

[erikwright (departed), 2015/05/20 20:54:14]

When calling into other crashpad code, I assume that that code will log whatever
error might occur.

+    return;
+  }
+  IssueWrite();
+}
+
+void PipeState::ResetConnection() {
+  memset(&request_, 0, sizeof(request_));
+
+  if (!DisconnectNamedPipe(pipe_.get())) {
+    PLOG(ERROR);
+    pipe_.reset();
+  } else {
+    IssueConnect();
+  }
+}
+
+}  // namespace
+
+RegistrationServer::RegistrationServer() : stop_event_()/*, stopped_event_()*/ {

[scottmg, 2015/05/20 18:53:09]

Delete old (?) comment.

[erikwright (departed), 2015/05/20 20:54:14]

Done.

+  stop_event_.reset(CreateEvent(nullptr, FALSE, FALSE, nullptr));
+  DPCHECK(stop_event_.is_valid());
+}
+
+RegistrationServer::~RegistrationServer() {
+}
+
+void RegistrationServer::Run(const base::string16& pipe_name,
+                             Delegate* delegate) {
+  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 (i == 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()) {
+      // If this is the first successfully created pipe, notify the delegate.
+      if (pipes.size() == 0)
+        delegate->OnStarted();

[scottmg, 2015/05/20 18:53:09]

Moving this down below the loop seems tidier than having an extra condition in
the loop.

[erikwright (departed), 2015/05/20 20:54:14]

Done.

+      pipes.push_back(new PipeState(pipe.Pass(), delegate));
+      handles.push_back(pipes.back()->completion_event());
+    } else {
+      PLOG(ERROR);

[scottmg, 2015/05/20 18:53:09]

If we get to this case, then the next time around the loop isn't going to have
FILE_FLAG_FIRST_PIPE_INSTANCE set so isn't going to work properly. So I think it
might as well exit after logging an error. Alternatively if it's worth retrying,
then i == 0 above needs to be pipes.size() == 0.

[erikwright (departed), 2015/05/20 20:54:13]

Done.

+    }
+  }
+
+  // Add stop_event_ to the list of events we will observe.
+  handles.push_back(stop_event_.get());
+
+  // Run the main loop, dispatching completion event signals to the pipe
+  // instances.
+  while (true) {
+    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()) {
+      // Handle a completion event.
+      pipes[wait_result - WAIT_OBJECT_0]->OnCompletion();
+      continue;
+    } else if (wait_result == WAIT_OBJECT_0 + pipes.size()) {
+      // Exit due to stop_event_.
+    } else if (wait_result == WAIT_FAILED) {
+      // Exit due to error.
+      PLOG(ERROR);
+    } 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();
+  }
+
+  // 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);

[scottmg, 2015/05/20 18:53:09]

If we exited the loop above with WAIT_FAILED, is this going to succeed? Is the
PCHECK going to crash?

[erikwright (departed), 2015/05/20 20:54:13]

It's hard to rationalize about what would happen if WAIT_FAILED occurs in the
first WaitForMultipleObjects. Quite possibly the root cause of that failure
might cause the second WFMO to fail too.

The important thing is that we don't destroy the PipeState instances if we don't
know that the CancelIo completed. The PCHECK on line 360 ensures this.

+  PCHECK(wait_result != WAIT_FAILED);
+  DCHECK_GE(wait_result, WAIT_OBJECT_0);
+  DCHECK_LT(wait_result, WAIT_OBJECT_0 + handles.size());
+}
+
+void RegistrationServer::Stop() {
+  if (!SetEvent(stop_event_.get()))
+    PLOG(FATAL);
+}
+
+}  // namespace crashpad