Review helper for https://codereview.chromium.org/1409073013

client/crashpad_client_mac.cc

 // Copyright 2014 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 "client/crashpad_client.h"
 
 #include <mach/mach.h>
 #include <sys/wait.h>
 #include <unistd.h>
 
 #include "base/logging.h"
+#include "base/mac/mach_logging.h"
 #include "base/posix/eintr_wrapper.h"
 #include "base/strings/stringprintf.h"
 #include "util/mach/child_port_handshake.h"
 #include "util/mach/exception_ports.h"
 #include "util/mach/mach_extensions.h"
+#include "util/misc/implicit_cast.h"
 #include "util/posix/close_multiple.h"
 
 namespace crashpad {
 
 namespace {
 
 std::string FormatArgumentString(const std::string& name,
                                  const std::string& value) {
   return base::StringPrintf("--%s=%s", name.c_str(), value.c_str());
 }
@@ skipping 33 matching lines @@
 // so AND them with ExcMaskValid(). EXC_MASK_CRASH is always supported.
 bool SetCrashExceptionPorts(exception_handler_t exception_handler) {
   ExceptionPorts exception_ports(ExceptionPorts::kTargetTypeTask, TASK_NULL);
   return exception_ports.SetExceptionPort(
       (EXC_MASK_CRASH | EXC_MASK_RESOURCE | EXC_MASK_GUARD) & ExcMaskValid(),
       exception_handler,
       EXCEPTION_STATE_IDENTITY | MACH_EXCEPTION_CODES,
       MACHINE_THREAD_STATE);
 }
 
+//! \brief Starts a Crashpad handler.
+class HandlerStarter final {
+ public:
+  //! \brief Starts a Crashpad handler.
+  //!
+  //! All parameters are as in CrashpadClient::StartHandler().
+  //!
+  //! \return On success, a send right to the Crashpad handler that has been
+  //!     started. On failure, `MACH_PORT_NULL` with a message logged.
+//  static base::mac::ScopedMachSendRight Start(
+//      const base::FilePath& handler,
+//      const base::FilePath& database,
+//      const std::string& url,
+//      const std::map<std::string, std::string>& annotations,
+//      const std::vector<std::string>& arguments) {
+
+ private:
+//  static bool CommonStart(const base::FilePath& handler,
+//                          const base::FilePath& database,
+//                          const std::string& url,
+//                          const std::map<std::string, std::string>& annotations,
+//                          const std::vector<std::string>& arguments,
+//                          base::mac::ScopedMachReceiveRight receive_right) {
+
+  DISALLOW_IMPLICIT_CONSTRUCTORS(HandlerStarter);
+};
+
 }  // namespace
 
 CrashpadClient::CrashpadClient()
     : exception_port_() {
 }
 
 CrashpadClient::~CrashpadClient() {
 }
 
 bool CrashpadClient::StartHandler(
     const base::FilePath& handler,
     const base::FilePath& database,
     const std::string& url,
     const std::map<std::string, std::string>& annotations,
     const std::vector<std::string>& arguments) {
   DCHECK(!exception_port_.is_valid());
 
-  // Set up the arguments for execve() first. These aren’t needed until execve()
-  // is called, but it’s dangerous to do this in a child process after fork().
+  // exception_port_ = HandlerStarter::Start(
+  //     handler, database, url, annotations, arguments);
+
+//  static base::mac::ScopedMachSendRight Start(
+//      const base::FilePath& handler,
+//      const base::FilePath& database,
+//      const std::string& url,
+//      const std::map<std::string, std::string>& annotations,
+//      const std::vector<std::string>& arguments) {
+  base::mac::ScopedMachReceiveRight receive_right(
+      NewMachPort(MACH_PORT_RIGHT_RECEIVE));
+  if (receive_right == kMachPortNull) {
+    return base::mac::ScopedMachSendRight();
+  }
+
+  mach_port_t port;
+  mach_msg_type_name_t right_type;
+  kern_return_t kr = mach_port_extract_right(mach_task_self(),
+                                             receive_right.get(),
+                                             MACH_MSG_TYPE_MAKE_SEND,
+                                             &port,
+                                             &right_type);
+  if (kr != KERN_SUCCESS) {
+    MACH_LOG(ERROR, kr) << "mach_port_extract_right";
+    return base::mac::ScopedMachSendRight();
+  }
+  base::mac::ScopedMachSendRight send_right(port);
+  DCHECK_EQ(port, receive_right.get());
+  DCHECK_EQ(right_type,
+            implicit_cast<mach_msg_type_name_t>(MACH_MSG_TYPE_PORT_SEND));
+
+  if (!CommonStart(handler,
+                   database,
+                   url,
+                   annotations,
+                   arguments,
+                   receive_right.Pass())) {
+    return base::mac::ScopedMachSendRight();
+  }
+
+  return send_right;
+
+//  static bool CommonStart(const base::FilePath& handler,
+//                          const base::FilePath& database,
+//                          const std::string& url,
+//                          const std::map<std::string, std::string>& annotations,
+//                          const std::vector<std::string>& arguments,
+//                          base::mac::ScopedMachReceiveRight receive_right) {
+  // Set up the arguments for execve() first. These aren’t needed until
+  // execve() is called, but it’s dangerous to do this in a child process
+  // after fork().
   ChildPortHandshake child_port_handshake;
-  base::ScopedFD client_read_fd = child_port_handshake.ClientReadFD();
+  base::ScopedFD server_write_fd = child_port_handshake.ServerWriteFD();
 
   // Use handler as argv[0], followed by arguments directed by this method’s
   // parameters and a --handshake-fd argument. |arguments| are added first so
-  // that if it erroneously contains an argument such as --url, the actual |url|
-  // argument passed to this method will supersede it. In normal command-line
-  // processing, the last parameter wins in the case of a conflict.
+  // that if it erroneously contains an argument such as --url, the actual
+  // |url| argument passed to this method will supersede it. In normal
+  // command-line processing, the last parameter wins in the case of a
+  // conflict.
   std::vector<std::string> argv(1, handler.value());
   argv.reserve(1 + arguments.size() + 2 + annotations.size() + 1);
   for (const std::string& argument : arguments) {
     argv.push_back(argument);
   }
   if (!database.value().empty()) {
     argv.push_back(FormatArgumentString("database", database.value()));
   }
   if (!url.empty()) {
     argv.push_back(FormatArgumentString("url", url));
   }
   for (const auto& kv : annotations) {
     argv.push_back(
         FormatArgumentString("annotation", kv.first + '=' + kv.second));
   }
-  argv.push_back(FormatArgumentInt("handshake-fd", client_read_fd.get()));
+  argv.push_back(FormatArgumentInt("handshake-fd", server_write_fd.get()));
+
+  const char* handler_c = handler.value().c_str();
 
   // argv_c contains const char* pointers and is terminated by nullptr. argv
   // is required because the pointers in argv_c need to point somewhere, and
   // they can’t point to temporaries such as those returned by
   // FormatArgumentString().
   std::vector<const char*> argv_c;
   argv_c.reserve(argv.size() + 1);
   for (const std::string& argument : argv) {
     argv_c.push_back(argument.c_str());
   }
   argv_c.push_back(nullptr);
 
   // Double-fork(). The three processes involved are parent, child, and
-  // grandchild. The grandchild will become the handler process. The child exits
-  // immediately after spawning the grandchild, so the grandchild becomes an
-  // orphan and its parent process ID becomes 1. This relieves the parent and
-  // child of the responsibility for reaping the grandchild with waitpid() or
-  // similar. The handler process is expected to outlive the parent process, so
-  // the parent shouldn’t be concerned with reaping it. This approach means that
-  // accidental early termination of the handler process will not result in a
-  // zombie process.
+  // grandchild. The grandchild will become the handler process. The child
+  // exits immediately after spawning the grandchild, so the grandchild
+  // becomes an orphan and its parent process ID becomes 1. This relieves the
+  // parent and child of the responsibility for reaping the grandchild with
+  // waitpid() or similar. The handler process is expected to outlive the
+  // parent process, so the parent shouldn’t be concerned with reaping it.
+  // This approach means that accidental early termination of the handler
+  // process will not result in a zombie process.
   pid_t pid = fork();
   if (pid < 0) {
     PLOG(ERROR) << "fork";
     return false;
   }
 
   if (pid == 0) {
     // Child process.
 
     // Call setsid(), creating a new process group and a new session, both led
-    // by this process. The new process group has no controlling terminal. This
-    // disconnects it from signals generated by the parent process’ terminal.
+    // by this process. The new process group has no controlling terminal.
+    // This disconnects it from signals generated by the parent process’
+    // terminal.
     //
     // setsid() is done in the child instead of the grandchild so that the
-    // grandchild will not be a session leader. If it were a session leader, an
-    // accidental open() of a terminal device without O_NOCTTY would make that
-    // terminal the controlling terminal.
+    // grandchild will not be a session leader. If it were a session leader,
+    // an accidental open() of a terminal device without O_NOCTTY would make
+    // that terminal the controlling terminal.
     //
     // It’s not desirable for the handler to have a controlling terminal. The
-    // handler monitors clients on its own and manages its own lifetime, exiting
-    // when it loses all clients and when it deems it appropraite to do so. It
-    // may serve clients in different process groups or sessions than its
-    // original client, and receiving signals intended for its original client’s
-    // process group could be harmful in that case.
+    // handler monitors clients on its own and manages its own lifetime,
+    // exiting when it loses all clients and when it deems it appropraite to
+    // do so. It may serve clients in different process groups or sessions
+    // than its original client, and receiving signals intended for its
+    // original client’s process group could be harmful in that case.
     PCHECK(setsid() != -1) << "setsid";
 
     pid = fork();
     if (pid < 0) {
       PLOG(FATAL) << "fork";
     }
 
     if (pid > 0) {
       // Child process.
 
       // _exit() instead of exit(), because fork() was called.
       _exit(EXIT_SUCCESS);
     }
 
     // Grandchild process.
 
-    CloseMultipleNowOrOnExec(STDERR_FILENO + 1, client_read_fd.get());
+    CloseMultipleNowOrOnExec(STDERR_FILENO + 1, server_write_fd.get());
 
     // &argv_c[0] is a pointer to a pointer to const char data, but because of
     // how C (not C++) works, execvp() wants a pointer to a const pointer to
     // char data. It modifies neither the data nor the pointers, so the
     // const_cast is safe.
-    execvp(handler.value().c_str(), const_cast<char* const*>(&argv_c[0]));
-    PLOG(FATAL) << "execvp " << handler.value();
+    execvp(handler_c, const_cast<char* const*>(&argv_c[0]));
+    PLOG(FATAL) << "execvp " << handler_c;
   }
 
   // Parent process.
 
-  client_read_fd.reset();
+  // Close the write side of the pipe, so that the handler process is the only
+  // process that can write to it.
+  server_write_fd.reset();
 
   // waitpid() for the child, so that it does not become a zombie process. The
   // child normally exits quickly.
   int status;
   pid_t wait_pid = HANDLE_EINTR(waitpid(pid, &status, 0));
   PCHECK(wait_pid != -1) << "waitpid";
   DCHECK_EQ(wait_pid, pid);
 
   if (WIFSIGNALED(status)) {
     LOG(WARNING) << "intermediate process: signal " << WTERMSIG(status);
   } else if (!WIFEXITED(status)) {
     DLOG(WARNING) << "intermediate process: unknown termination " << status;
   } else if (WEXITSTATUS(status) != EXIT_SUCCESS) {
-    LOG(WARNING) << "intermediate process: exit status " << WEXITSTATUS(status);
+    LOG(WARNING) << "intermediate process: exit status "
+                 << WEXITSTATUS(status);
   }
 
   // Rendezvous with the handler running in the grandchild process.
-  exception_port_.reset(child_port_handshake.RunServer(
-      ChildPortHandshake::PortRightType::kSendRight));
+  if (!child_port_handshake.RunClient(receive_right.get(),
+                                      MACH_MSG_TYPE_MOVE_RECEIVE)) {
+    return false;
+  }
 
-  return exception_port_.is_valid();
+  ignore_result(receive_right.release());
+  return true;
+
+  if (!exception_port_.is_valid()) {
+    return false;
+  }
+
+  return true;
 }
 
 bool CrashpadClient::UseHandler() {
   DCHECK(exception_port_.is_valid());
 
   return SetCrashExceptionPorts(exception_port_.get());
 }
 
 // static
 void CrashpadClient::UseSystemDefaultHandler() {
   base::mac::ScopedMachSendRight
       system_crash_reporter_handler(SystemCrashReporterHandler());
 
   // Proceed even if SystemCrashReporterHandler() failed, setting MACH_PORT_NULL
   // to clear the current exception ports.
   if (!SetCrashExceptionPorts(system_crash_reporter_handler.get())) {
     SetCrashExceptionPorts(MACH_PORT_NULL);
   }
 }
 
 }  // namespace crashpad