diff --git a/client/crash_report_database_mac.mm b/client/crash_report_database_mac.mm index ca355c05..80cc5cd6 100644 --- a/client/crash_report_database_mac.mm +++ b/client/crash_report_database_mac.mm @@ -94,6 +94,15 @@ bool CreateOrEnsureDirectoryExists(const base::FilePath& path) { return EnsureDirectoryExists(path); } +// Creates a long database xattr name from the short constant name. These names +// have changed, and new_name determines whether the returned xattr name will be +// the old name or its new equivalent. +std::string XattrNameInternal(const base::StringPiece& name, bool new_name) { + return base::StringPrintf(new_name ? "org.chromium.crashpad.database.%s" + : "com.googlecode.crashpad.%s", + name.data()); +} + //! \brief A CrashReportDatabase that uses HFS+ extended attributes to store //! report metadata. //! @@ -173,8 +182,7 @@ class CrashReportDatabaseMac : public CrashReportDatabase { //! //! \return `true` if all the metadata was read successfully, `false` //! otherwise. - static bool ReadReportMetadataLocked(const base::FilePath& path, - Report* report); + bool ReadReportMetadataLocked(const base::FilePath& path, Report* report); //! \brief Reads the metadata from all the reports in a database subdirectory. //! Invalid reports are skipped. @@ -183,18 +191,19 @@ class CrashReportDatabaseMac : public CrashReportDatabase { //! \param[out] reports An empty vector of reports, which will be filled. //! //! \return The operation status code. - static OperationStatus ReportsInDirectory(const base::FilePath& path, - std::vector* reports); + OperationStatus ReportsInDirectory(const base::FilePath& path, + std::vector* reports); //! \brief Creates a database xattr name from the short constant name. //! //! \param[in] name The short name of the extended attribute. //! //! \return The long name of the extended attribute. - static std::string XattrName(const base::StringPiece& name); + std::string XattrName(const base::StringPiece& name); base::FilePath base_dir_; Settings settings_; + bool xattr_new_names_; InitializationStateDcheck initialized_; DISALLOW_COPY_AND_ASSIGN(CrashReportDatabaseMac); @@ -204,6 +213,7 @@ CrashReportDatabaseMac::CrashReportDatabaseMac(const base::FilePath& path) : CrashReportDatabase(), base_dir_(path), settings_(base_dir_.Append(kSettings)), + xattr_new_names_(false), initialized_() { } @@ -230,10 +240,25 @@ bool CrashReportDatabaseMac::Initialize(bool may_create) { if (!settings_.Initialize()) return false; - // Write an xattr as the last step, to ensure the filesystem has support for - // them. This attribute will never be read. - if (!WriteXattrBool(base_dir_, XattrName(kXattrDatabaseInitialized), true)) - return false; + // Do an xattr operation as the last step, to ensure the filesystem has + // support for them. This xattr also serves as a marker for whether the + // database uses old or new xattr names. + bool value; + if (ReadXattrBool(base_dir_, + XattrNameInternal(kXattrDatabaseInitialized, true), + &value) == XattrStatus::kOK && + value) { + xattr_new_names_ = true; + } else if (ReadXattrBool(base_dir_, + XattrNameInternal(kXattrDatabaseInitialized, false), + &value) == XattrStatus::kOK && + value) { + xattr_new_names_ = false; + } else { + xattr_new_names_ = true; + if (!WriteXattrBool(base_dir_, XattrName(kXattrDatabaseInitialized), true)) + return false; + } INITIALIZATION_STATE_SET_VALID(initialized_); return true; @@ -540,7 +565,6 @@ base::ScopedFD CrashReportDatabaseMac::ObtainReportLock( return base::ScopedFD(fd); } -// static bool CrashReportDatabaseMac::ReadReportMetadataLocked( const base::FilePath& path, Report* report) { std::string uuid_string; @@ -583,7 +607,6 @@ bool CrashReportDatabaseMac::ReadReportMetadataLocked( return true; } -// static CrashReportDatabase::OperationStatus CrashReportDatabaseMac::ReportsInDirectory( const base::FilePath& path, std::vector* reports) { @@ -620,9 +643,8 @@ CrashReportDatabase::OperationStatus CrashReportDatabaseMac::ReportsInDirectory( return kNoError; } -// static std::string CrashReportDatabaseMac::XattrName(const base::StringPiece& name) { - return base::StringPrintf("com.googlecode.crashpad.%s", name.data()); + return XattrNameInternal(name, xattr_new_names_); } scoped_ptr InitializeInternal(const base::FilePath& path, diff --git a/client/crashpad_client_mac.cc b/client/crashpad_client_mac.cc index 5797fba8..caf7e91b 100644 --- a/client/crashpad_client_mac.cc +++ b/client/crashpad_client_mac.cc @@ -19,11 +19,13 @@ #include #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 { @@ -77,6 +79,199 @@ bool SetCrashExceptionPorts(exception_handler_t exception_handler) { 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& annotations, + const std::vector& 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_PORT_SEND)); + + if (!CommonStart(handler, + database, + url, + annotations, + arguments, + receive_right.Pass())) { + return base::mac::ScopedMachSendRight(); + } + + return send_right; + } + + private: + static bool CommonStart(const base::FilePath& handler, + const base::FilePath& database, + const std::string& url, + const std::map& annotations, + const std::vector& 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 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. + std::vector 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", 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 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. + 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. + // + // 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. + // + // 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. + 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, 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_c, const_cast(&argv_c[0])); + PLOG(FATAL) << "execvp " << handler_c; + } + + // Parent process. + + // 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); + } + + // Rendezvous with the handler running in the grandchild process. + if (!child_port_handshake.RunClient(receive_right.get(), + MACH_MSG_TYPE_MOVE_RECEIVE)) { + return false; + } + + ignore_result(receive_right.release()); + return true; + } + + DISALLOW_IMPLICIT_CONSTRUCTORS(HandlerStarter); +}; + } // namespace CrashpadClient::CrashpadClient() @@ -94,124 +289,13 @@ bool CrashpadClient::StartHandler( const std::vector& 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(). - ChildPortHandshake child_port_handshake; - int handshake_fd = child_port_handshake.ReadPipeFD(); - - // 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. - std::vector 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", handshake_fd)); - - // 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 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. - pid_t pid = fork(); - if (pid < 0) { - PLOG(ERROR) << "fork"; + exception_port_ = HandlerStarter::Start( + handler, database, url, annotations, arguments); + if (!exception_port_.is_valid()) { 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. - // - // 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. - // - // 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. - 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, handshake_fd); - - // &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(&argv_c[0])); - PLOG(FATAL) << "execvp " << handler.value(); - } - - // Parent process. - - // 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); - } - - // Rendezvous with the handler running in the grandchild process. - exception_port_.reset(child_port_handshake.RunServer()); - - return exception_port_.is_valid(); + return true; } bool CrashpadClient::UseHandler() { diff --git a/client/prune_crash_reports.cc b/client/prune_crash_reports.cc index dadb93d7..d94f212c 100644 --- a/client/prune_crash_reports.cc +++ b/client/prune_crash_reports.cc @@ -59,10 +59,9 @@ void PruneCrashReportDatabase(CrashReportDatabase* database, } } - // TODO(rsesek): For databases that do not use a directory structure, - // it is possible for the metadata sidecar to become corrupted and thus - // leave orphaned crash report files on-disk. - // https://code.google.com/p/crashpad/issues/detail?id=66 + // TODO(rsesek): For databases that do not use a directory structure, it is + // possible for the metadata sidecar to become corrupted and thus leave + // orphaned crash report files on-disk. https://crashpad.chromium.org/bug/66 } // static diff --git a/doc/appengine/README b/doc/appengine/README index d3f4d227..00a7f761 100644 --- a/doc/appengine/README +++ b/doc/appengine/README @@ -1,4 +1,4 @@ -This is the App Engine app that serves https://crashpad-home.appspot.com/. +This is the App Engine app that serves https://crashpad.chromium.org/. To work on this app, obtain the App Engine SDK for Go from https://cloud.google.com/appengine/downloads. Unpacking it produces a diff --git a/doc/developing.ad b/doc/developing.ad index 58ca79de..9174f6b1 100644 --- a/doc/developing.ad +++ b/doc/developing.ad @@ -83,7 +83,7 @@ $ *gclient sync* == Building -Crashpad uses https://gyp.googlecode.com/[GYP] to generate +Crashpad uses https://gyp.gsrc.io/[GYP] to generate https://martine.github.io/ninja/[Ninja] build files. The build is described by `.gyp` files throughout the Crashpad source code tree. The `build/gyp_crashpad.py` script runs GYP properly for Crashpad, and is also @@ -106,12 +106,13 @@ need to install it separately. == Testing -Crashpad uses https://googletest.googlecode.com/[Google Test] as its +Crashpad uses https://github.com/google/googletest/[Google Test] as its unit-testing framework, and some tests use -https://googlemock.googlecode.com/[Google Mock] as well. Its tests are currently -split up into several test executables, each dedicated to testing a different -component. This may change in the future. After a successful build, the test -executables will be found at `out/Debug/crashpad_*_test`. +https://github.com/google/googletest/tree/master/googlemock/[Google Mock] as +well. Its tests are currently split up into several test executables, each +dedicated to testing a different component. This may change in the future. After +a successful build, the test executables will be found at +`out/Debug/crashpad_*_test`. [subs="verbatim,quotes"] ---- diff --git a/doc/index.ad b/doc/index.ad index f9031d2f..5bb16f8f 100644 --- a/doc/index.ad +++ b/doc/index.ad @@ -16,16 +16,16 @@ = Crashpad -https://crashpad-home.appspot.com/[Crashpad] is a crash-reporting system. +https://crashpad.chromium.org/[Crashpad] is a crash-reporting system. == Documentation * link:status.html[Project status] * link:developing.html[Developing Crashpad]: instructions for getting the source code, building, testing, and contributing to the project. - * https://crashpad-home.appspot.com/doxygen/index.html[Crashpad interface + * https://crashpad.chromium.org/doxygen/index.html[Crashpad interface documentation] - * https://crashpad-home.appspot.com/man/index.html[Crashpad tool man pages] + * https://crashpad.chromium.org/man/index.html[Crashpad tool man pages] == Source Code @@ -34,8 +34,8 @@ https://chromium.googlesource.com/crashpad/crashpad. == Other Links - * Bugs can be reported at the - https://code.google.com/p/crashpad/issues/entry[Crashpad issue tracker]. + * Bugs can be reported at the https://crashpad.chromium.org/bug/new[Crashpad + issue tracker]. * The https://build.chromium.org/p/client.crashpad[Crashpad Buildbot] performs automated builds and tests. * https://groups.google.com/a/chromium.org/group/crashpad-dev[crashpad-dev] is diff --git a/doc/status.ad b/doc/status.ad index 301195fc..e980b9d0 100644 --- a/doc/status.ad +++ b/doc/status.ad @@ -28,18 +28,17 @@ https://chromium.googlesource.com/chromium/src/+/d413b2dcb54d523811d386f1ff4084f == In Progress Crashpad is actively being bootstrapped on -https://code.google.com/p/crashpad/issues/detail?id=1[Windows]. All major -components are now working on Windows, and integration into Chromium is expected -shortly. +https://crashpad.chromium.org/bug/1[Windows]. All major components are now +working on Windows, and integration into Chromium is expected shortly. Initial work on a Crashpad client for -https://code.google.com/p/crashpad/issues/detail?id=30[Android] has begun. This -is currently in the design phase. +https://crashpad.chromium.org/bug/30[Android] has begun. This is currently in +the design phase. == Future There are plans to bring Crashpad clients to other operating systems in the future, including a more generic non-Android Linux implementation. There are -also plans to implement a -https://code.google.com/p/crashpad/issues/detail?id=29[crash report processor] -as part of Crashpad. No timeline for completing this work has been set yet. +also plans to implement a https://crashpad.chromium.org/bug/29[crash report +processor] as part of Crashpad. No timeline for completing this work has been +set yet. diff --git a/doc/support/generate.sh b/doc/support/generate.sh index f4410aee..0a2be484 100755 --- a/doc/support/generate.sh +++ b/doc/support/generate.sh @@ -43,7 +43,7 @@ done # Move doc/index.html to index.html, adjusting relative paths to other files in # doc. -base_url=https://crashpad-home.appspot.com/ +base_url=https://crashpad.chromium.org/ ${sed_ext} -e 's%%%g' \ -e 's%%%g' \ -e 's%CatchMachException(behavior, @@ -172,7 +172,7 @@ class ExceptionHandlerServerRun : public UniversalMachExcServer::Interface, // to craft and send a no-senders notification via its exception port, and // cause the handler to stop processing exceptions and exit. LOG(WARNING) << "notify port mismatch"; - return MIG_BAD_ID; + return KERN_FAILURE; } running_ = false; @@ -199,11 +199,11 @@ class ExceptionHandlerServerRun : public UniversalMachExcServer::Interface, // called. if (notify != notify_port_) { LOG(WARNING) << "notify port mismatch"; - return MIG_BAD_ID; + return KERN_FAILURE; } NOTREACHED(); - return KERN_FAILURE; + return MIG_BAD_ID; } UniversalMachExcServer mach_exc_server_; @@ -219,8 +219,9 @@ class ExceptionHandlerServerRun : public UniversalMachExcServer::Interface, } // namespace -ExceptionHandlerServer::ExceptionHandlerServer() - : receive_port_(NewMachPort(MACH_PORT_RIGHT_RECEIVE)) { +ExceptionHandlerServer::ExceptionHandlerServer( + base::mac::ScopedMachReceiveRight receive_port) + : receive_port_(receive_port.Pass()) { CHECK(receive_port_.is_valid()); } diff --git a/handler/mac/exception_handler_server.h b/handler/mac/exception_handler_server.h index 83e131be..05c4056f 100644 --- a/handler/mac/exception_handler_server.h +++ b/handler/mac/exception_handler_server.h @@ -28,19 +28,24 @@ namespace crashpad { //! process. class ExceptionHandlerServer { public: - ExceptionHandlerServer(); + //! \brief Constructs an ExceptionHandlerServer object. + //! + //! \param[in] receive_port The port that exception messages and no-senders + //! notifications will be received on. + explicit ExceptionHandlerServer( + base::mac::ScopedMachReceiveRight receive_port); ~ExceptionHandlerServer(); //! \brief Runs the exception-handling server. //! //! \param[in] exception_interface An object to send exception messages to. //! - //! This method monitors receive_port() for exception messages and no-senders - //! notifications. It continues running until it has no more clients, - //! indicated by the receipt of a no-senders notification. It is important to - //! assure that a send right has been transferred to a client (or queued by - //! `mach_msg()` to be sent to a client) prior to calling this method, or it - //! will detect that it is sender-less and return immediately. + //! This method monitors the receive port for exception messages and + //! no-senders notifications. It continues running until it has no more + //! clients, indicated by the receipt of a no-senders notification. It is + //! important to assure that a send right exists in a client (or has been + //! queued by `mach_msg()` to be sent to a client) prior to calling this + //! method, or it will detect that it is sender-less and return immediately. //! //! All exception messages will be passed to \a exception_interface. //! @@ -48,17 +53,10 @@ class ExceptionHandlerServer { //! //! If an unexpected condition that prevents this method from functioning is //! encountered, it will log a message and terminate execution. Receipt of an - //! invalid message on receive_port() will cause a message to be logged, but + //! invalid message on the receive port will cause a message to be logged, but //! this method will continue running normally. void Run(UniversalMachExcServer::Interface* exception_interface); - //! \brief Returns the receive right that will be monitored for exception - //! messages. - //! - //! The caller does not take ownership of this port. The caller must not use - //! this port for any purpose other than to make send rights for clients. - mach_port_t receive_port() const { return receive_port_.get(); } - private: base::mac::ScopedMachReceiveRight receive_port_; diff --git a/handler/main.cc b/handler/main.cc index 5f192f92..de3171ed 100644 --- a/handler/main.cc +++ b/handler/main.cc @@ -19,6 +19,7 @@ #include #include "base/files/file_path.h" +#include "base/files/scoped_file.h" #include "base/logging.h" #include "base/memory/scoped_ptr.h" #include "build/build_config.h" @@ -33,6 +34,7 @@ #if defined(OS_MACOSX) #include +#include "base/mac/scoped_mach_port.h" #include "handler/mac/crash_report_exception_handler.h" #include "handler/mac/exception_handler_server.h" #include "util/mach/child_port_handshake.h" @@ -210,18 +212,23 @@ int HandlerMain(int argc, char* argv[]) { } #if defined(OS_MACOSX) + CloseStdinAndStdout(); + if (options.reset_own_crash_exception_port_to_system_default) { CrashpadClient::UseSystemDefaultHandler(); } -#endif - ExceptionHandlerServer exception_handler_server; + base::mac::ScopedMachReceiveRight receive_right( + ChildPortHandshake::RunServerForFD( + base::ScopedFD(options.handshake_fd), + ChildPortHandshake::PortRightType::kReceiveRight)); + if (!receive_right.is_valid()) { + return EXIT_FAILURE; + } -#if defined(OS_MACOSX) - CloseStdinAndStdout(); - ChildPortHandshake::RunClient(options.handshake_fd, - exception_handler_server.receive_port(), - MACH_MSG_TYPE_MAKE_SEND); + ExceptionHandlerServer exception_handler_server(receive_right.Pass()); +#elif defined(OS_WIN) + ExceptionHandlerServer exception_handler_server(options.pipe_name); #endif // OS_MACOSX scoped_ptr database(CrashReportDatabase::Initialize( @@ -237,11 +244,7 @@ int HandlerMain(int argc, char* argv[]) { CrashReportExceptionHandler exception_handler( database.get(), &upload_thread, &options.annotations); -#if defined(OS_MACOSX) exception_handler_server.Run(&exception_handler); -#elif defined(OS_WIN) - exception_handler_server.Run(&exception_handler, options.pipe_name); -#endif // OS_MACOSX upload_thread.Stop(); diff --git a/minidump/minidump_module_writer.cc b/minidump/minidump_module_writer.cc index 7e316ea2..fd78b9f8 100644 --- a/minidump/minidump_module_writer.cc +++ b/minidump/minidump_module_writer.cc @@ -106,15 +106,7 @@ void MinidumpModuleCodeViewRecordPDB70Writer::InitializeFromSnapshot( const ModuleSnapshot* module_snapshot) { DCHECK_EQ(state(), kStateMutable); - std::string name = module_snapshot->Name(); - std::string leaf_name; - size_t last_slash = name.find_last_of('/'); - if (last_slash != std::string::npos) { - leaf_name = name.substr(last_slash + 1); - } else { - leaf_name = name; - } - SetPDBName(leaf_name); + SetPDBName(module_snapshot->DebugFileName()); UUID uuid; uint32_t age; diff --git a/minidump/minidump_module_writer_test.cc b/minidump/minidump_module_writer_test.cc index 06d23123..f0cc36c1 100644 --- a/minidump/minidump_module_writer_test.cc +++ b/minidump/minidump_module_writer_test.cc @@ -614,6 +614,7 @@ void InitializeTestModuleSnapshotFromMinidumpModule( TestModuleSnapshot* module_snapshot, const MINIDUMP_MODULE& minidump_module, const std::string& name, + const std::string& pdb_name, const crashpad::UUID& uuid, uint32_t age) { module_snapshot->SetName(name); @@ -647,12 +648,14 @@ void InitializeTestModuleSnapshotFromMinidumpModule( module_snapshot->SetModuleType(module_type); module_snapshot->SetUUIDAndAge(uuid, age); + module_snapshot->SetDebugFileName(pdb_name); } TEST(MinidumpModuleWriter, InitializeFromSnapshot) { MINIDUMP_MODULE expect_modules[3] = {}; const char* module_paths[arraysize(expect_modules)] = {}; const char* module_names[arraysize(expect_modules)] = {}; + const char* module_pdbs[arraysize(expect_modules)] = {}; UUID uuids[arraysize(expect_modules)] = {}; uint32_t ages[arraysize(expect_modules)] = {}; @@ -666,6 +669,7 @@ TEST(MinidumpModuleWriter, InitializeFromSnapshot) { expect_modules[0].VersionInfo.dwFileType = VFT_APP; module_paths[0] = "/usr/bin/true"; module_names[0] = "true"; + module_pdbs[0] = "true"; const uint8_t kUUIDBytes0[16] = {0x00, 0x11, 0x22, 0x33, 0x44, 0x55, 0x66, 0x77, 0x88, 0x99, 0xaa, 0xbb, 0xcc, 0xdd, 0xee, 0xff}; @@ -682,6 +686,7 @@ TEST(MinidumpModuleWriter, InitializeFromSnapshot) { expect_modules[1].VersionInfo.dwFileType = VFT_DLL; module_paths[1] = "/usr/lib/libSystem.B.dylib"; module_names[1] = "libSystem.B.dylib"; + module_pdbs[1] = "libSystem.B.dylib.pdb"; const uint8_t kUUIDBytes1[16] = {0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08, 0x09, 0x0a, 0x0b, 0x0c, 0x0d, 0x0e, 0x0f}; @@ -698,6 +703,7 @@ TEST(MinidumpModuleWriter, InitializeFromSnapshot) { expect_modules[2].VersionInfo.dwFileType = VFT_UNKNOWN; module_paths[2] = "/usr/lib/dyld"; module_names[2] = "dyld"; + module_pdbs[2] = "/usr/lib/dyld.pdb"; const uint8_t kUUIDBytes2[16] = {0xff, 0xfe, 0xfd, 0xfc, 0xfb, 0xfa, 0xf9, 0xf8, 0xf7, 0xf6, 0xf5, 0xf4, 0xf3, 0xf2, 0xf1, 0xf0}; @@ -712,6 +718,7 @@ TEST(MinidumpModuleWriter, InitializeFromSnapshot) { InitializeTestModuleSnapshotFromMinidumpModule(module_snapshot, expect_modules[index], module_paths[index], + module_pdbs[index], uuids[index], ages[index]); module_snapshots.push_back(module_snapshot); @@ -738,7 +745,7 @@ TEST(MinidumpModuleWriter, InitializeFromSnapshot) { &module_list->Modules[index], string_file.string(), module_paths[index], - module_names[index], + module_pdbs[index], &uuids[index], 0, ages[index], diff --git a/package.h b/package.h index 2556b2f2..a18d7b82 100644 --- a/package.h +++ b/package.h @@ -15,7 +15,7 @@ #ifndef CRASHPAD_PACKAGE_H_ #define CRASHPAD_PACKAGE_H_ -#define PACKAGE_BUGREPORT "https://code.google.com/p/crashpad/issues/entry" +#define PACKAGE_BUGREPORT "https://crashpad.chromium.org/bug/new" #define PACKAGE_COPYRIGHT \ "Copyright " PACKAGE_COPYRIGHT_YEAR " " PACKAGE_COPYRIGHT_OWNER #define PACKAGE_COPYRIGHT_OWNER "The Crashpad Authors" @@ -24,6 +24,6 @@ #define PACKAGE_STRING PACKAGE_NAME " " PACKAGE_VERSION #define PACKAGE_TARNAME "crashpad" #define PACKAGE_VERSION "0.7.0" -#define PACKAGE_URL "https://crashpad-home.appspot.com/" +#define PACKAGE_URL "https://crashpad.chromium.org/" #endif // CRASHPAD_PACKAGE_H_ diff --git a/snapshot/mac/module_snapshot_mac.cc b/snapshot/mac/module_snapshot_mac.cc index 7ddf217d..64f04dce 100644 --- a/snapshot/mac/module_snapshot_mac.cc +++ b/snapshot/mac/module_snapshot_mac.cc @@ -17,6 +17,7 @@ #include #include +#include "base/files/file_path.h" #include "base/strings/stringprintf.h" #include "snapshot/mac/mach_o_image_annotations_reader.h" #include "snapshot/mac/mach_o_image_reader.h" @@ -156,6 +157,11 @@ void ModuleSnapshotMac::UUIDAndAge(crashpad::UUID* uuid, uint32_t* age) const { *age = 0; } +std::string ModuleSnapshotMac::DebugFileName() const { + INITIALIZATION_STATE_DCHECK_VALID(initialized_); + return base::FilePath(Name()).BaseName().value(); +} + std::vector ModuleSnapshotMac::AnnotationsVector() const { INITIALIZATION_STATE_DCHECK_VALID(initialized_); MachOImageAnnotationsReader annotations_reader( diff --git a/snapshot/mac/module_snapshot_mac.h b/snapshot/mac/module_snapshot_mac.h index 198eae96..42b14703 100644 --- a/snapshot/mac/module_snapshot_mac.h +++ b/snapshot/mac/module_snapshot_mac.h @@ -76,6 +76,7 @@ class ModuleSnapshotMac final : public ModuleSnapshot { uint16_t* version_3) const override; ModuleType GetModuleType() const override; void UUIDAndAge(crashpad::UUID* uuid, uint32_t* age) const override; + std::string DebugFileName() const override; std::vector AnnotationsVector() const override; std::map AnnotationsSimpleMap() const override; diff --git a/snapshot/minidump/module_snapshot_minidump.cc b/snapshot/minidump/module_snapshot_minidump.cc index 18989f5f..973d3b27 100644 --- a/snapshot/minidump/module_snapshot_minidump.cc +++ b/snapshot/minidump/module_snapshot_minidump.cc @@ -58,25 +58,25 @@ bool ModuleSnapshotMinidump::Initialize( std::string ModuleSnapshotMinidump::Name() const { INITIALIZATION_STATE_DCHECK_VALID(initialized_); - NOTREACHED(); // https://code.google.com/p/crashpad/issues/detail?id=10 + NOTREACHED(); // https://crashpad.chromium.org/bug/10 return std::string(); } uint64_t ModuleSnapshotMinidump::Address() const { INITIALIZATION_STATE_DCHECK_VALID(initialized_); - NOTREACHED(); // https://code.google.com/p/crashpad/issues/detail?id=10 + NOTREACHED(); // https://crashpad.chromium.org/bug/10 return 0; } uint64_t ModuleSnapshotMinidump::Size() const { INITIALIZATION_STATE_DCHECK_VALID(initialized_); - NOTREACHED(); // https://code.google.com/p/crashpad/issues/detail?id=10 + NOTREACHED(); // https://crashpad.chromium.org/bug/10 return 0; } time_t ModuleSnapshotMinidump::Timestamp() const { INITIALIZATION_STATE_DCHECK_VALID(initialized_); - NOTREACHED(); // https://code.google.com/p/crashpad/issues/detail?id=10 + NOTREACHED(); // https://crashpad.chromium.org/bug/10 return 0; } @@ -85,7 +85,7 @@ void ModuleSnapshotMinidump::FileVersion(uint16_t* version_0, uint16_t* version_2, uint16_t* version_3) const { INITIALIZATION_STATE_DCHECK_VALID(initialized_); - NOTREACHED(); // https://code.google.com/p/crashpad/issues/detail?id=10 + NOTREACHED(); // https://crashpad.chromium.org/bug/10 *version_0 = 0; *version_1 = 0; *version_2 = 0; @@ -97,7 +97,7 @@ void ModuleSnapshotMinidump::SourceVersion(uint16_t* version_0, uint16_t* version_2, uint16_t* version_3) const { INITIALIZATION_STATE_DCHECK_VALID(initialized_); - NOTREACHED(); // https://code.google.com/p/crashpad/issues/detail?id=10 + NOTREACHED(); // https://crashpad.chromium.org/bug/10 *version_0 = 0; *version_1 = 0; *version_2 = 0; @@ -106,18 +106,24 @@ void ModuleSnapshotMinidump::SourceVersion(uint16_t* version_0, ModuleSnapshot::ModuleType ModuleSnapshotMinidump::GetModuleType() const { INITIALIZATION_STATE_DCHECK_VALID(initialized_); - NOTREACHED(); // https://code.google.com/p/crashpad/issues/detail?id=10 + NOTREACHED(); // https://crashpad.chromium.org/bug/10 return kModuleTypeUnknown; } void ModuleSnapshotMinidump::UUIDAndAge(crashpad::UUID* uuid, uint32_t* age) const { INITIALIZATION_STATE_DCHECK_VALID(initialized_); - NOTREACHED(); // https://code.google.com/p/crashpad/issues/detail?id=10 + NOTREACHED(); // https://crashpad.chromium.org/bug/10 *uuid = crashpad::UUID(); *age = 0; } +std::string ModuleSnapshotMinidump::DebugFileName() const { + INITIALIZATION_STATE_DCHECK_VALID(initialized_); + NOTREACHED(); // https://crashpad.chromium.org/bug/10 + return std::string(); +} + std::vector ModuleSnapshotMinidump::AnnotationsVector() const { INITIALIZATION_STATE_DCHECK_VALID(initialized_); return annotations_vector_; diff --git a/snapshot/minidump/module_snapshot_minidump.h b/snapshot/minidump/module_snapshot_minidump.h index c845c39b..bf933663 100644 --- a/snapshot/minidump/module_snapshot_minidump.h +++ b/snapshot/minidump/module_snapshot_minidump.h @@ -73,6 +73,7 @@ class ModuleSnapshotMinidump final : public ModuleSnapshot { uint16_t* version_3) const override; ModuleType GetModuleType() const override; void UUIDAndAge(crashpad::UUID* uuid, uint32_t* age) const override; + std::string DebugFileName() const override; std::vector AnnotationsVector() const override; std::map AnnotationsSimpleMap() const override; diff --git a/snapshot/minidump/process_snapshot_minidump.cc b/snapshot/minidump/process_snapshot_minidump.cc index 2d6c6dbf..6a1572a8 100644 --- a/snapshot/minidump/process_snapshot_minidump.cc +++ b/snapshot/minidump/process_snapshot_minidump.cc @@ -94,26 +94,26 @@ bool ProcessSnapshotMinidump::Initialize(FileReaderInterface* file_reader) { pid_t ProcessSnapshotMinidump::ProcessID() const { INITIALIZATION_STATE_DCHECK_VALID(initialized_); - NOTREACHED(); // https://code.google.com/p/crashpad/issues/detail?id=10 + NOTREACHED(); // https://crashpad.chromium.org/bug/10 return 0; } pid_t ProcessSnapshotMinidump::ParentProcessID() const { INITIALIZATION_STATE_DCHECK_VALID(initialized_); - NOTREACHED(); // https://code.google.com/p/crashpad/issues/detail?id=10 + NOTREACHED(); // https://crashpad.chromium.org/bug/10 return 0; } void ProcessSnapshotMinidump::SnapshotTime(timeval* snapshot_time) const { INITIALIZATION_STATE_DCHECK_VALID(initialized_); - NOTREACHED(); // https://code.google.com/p/crashpad/issues/detail?id=10 + NOTREACHED(); // https://crashpad.chromium.org/bug/10 snapshot_time->tv_sec = 0; snapshot_time->tv_usec = 0; } void ProcessSnapshotMinidump::ProcessStartTime(timeval* start_time) const { INITIALIZATION_STATE_DCHECK_VALID(initialized_); - NOTREACHED(); // https://code.google.com/p/crashpad/issues/detail?id=10 + NOTREACHED(); // https://crashpad.chromium.org/bug/10 start_time->tv_sec = 0; start_time->tv_usec = 0; } @@ -121,7 +121,7 @@ void ProcessSnapshotMinidump::ProcessStartTime(timeval* start_time) const { void ProcessSnapshotMinidump::ProcessCPUTimes(timeval* user_time, timeval* system_time) const { INITIALIZATION_STATE_DCHECK_VALID(initialized_); - NOTREACHED(); // https://code.google.com/p/crashpad/issues/detail?id=10 + NOTREACHED(); // https://crashpad.chromium.org/bug/10 user_time->tv_sec = 0; user_time->tv_usec = 0; system_time->tv_sec = 0; @@ -145,20 +145,20 @@ ProcessSnapshotMinidump::AnnotationsSimpleMap() const { // annotations_simple_map_ to be lazily constructed: InitializeCrashpadInfo() // could be called here, and from other locations that require it, rather than // calling it from Initialize(). - // https://code.google.com/p/crashpad/issues/detail?id=9 + // https://crashpad.chromium.org/bug/9 INITIALIZATION_STATE_DCHECK_VALID(initialized_); return annotations_simple_map_; } const SystemSnapshot* ProcessSnapshotMinidump::System() const { INITIALIZATION_STATE_DCHECK_VALID(initialized_); - NOTREACHED(); // https://code.google.com/p/crashpad/issues/detail?id=10 + NOTREACHED(); // https://crashpad.chromium.org/bug/10 return nullptr; } std::vector ProcessSnapshotMinidump::Threads() const { INITIALIZATION_STATE_DCHECK_VALID(initialized_); - NOTREACHED(); // https://code.google.com/p/crashpad/issues/detail?id=10 + NOTREACHED(); // https://crashpad.chromium.org/bug/10 return std::vector(); } @@ -173,27 +173,27 @@ std::vector ProcessSnapshotMinidump::Modules() const { const ExceptionSnapshot* ProcessSnapshotMinidump::Exception() const { INITIALIZATION_STATE_DCHECK_VALID(initialized_); - NOTREACHED(); // https://code.google.com/p/crashpad/issues/detail?id=10 + NOTREACHED(); // https://crashpad.chromium.org/bug/10 return nullptr; } std::vector ProcessSnapshotMinidump::MemoryMap() const { INITIALIZATION_STATE_DCHECK_VALID(initialized_); - NOTREACHED(); // https://code.google.com/p/crashpad/issues/detail?id=10 + NOTREACHED(); // https://crashpad.chromium.org/bug/10 return std::vector(); } std::vector ProcessSnapshotMinidump::Handles() const { INITIALIZATION_STATE_DCHECK_VALID(initialized_); - NOTREACHED(); // https://code.google.com/p/crashpad/issues/detail?id=10 + NOTREACHED(); // https://crashpad.chromium.org/bug/10 return std::vector(); } std::vector ProcessSnapshotMinidump::ExtraMemory() const { INITIALIZATION_STATE_DCHECK_VALID(initialized_); - NOTREACHED(); // https://code.google.com/p/crashpad/issues/detail?id=10 + NOTREACHED(); // https://crashpad.chromium.org/bug/10 return std::vector(); } diff --git a/snapshot/module_snapshot.h b/snapshot/module_snapshot.h index 6c397ed0..a1bafd92 100644 --- a/snapshot/module_snapshot.h +++ b/snapshot/module_snapshot.h @@ -118,8 +118,20 @@ class ModuleSnapshot { //! \a age is the number of times the UUID has been reused. This occurs on //! Windows with incremental linking. On other platforms \a age will always be //! `0`. + //! + //! \sa DebugFileName() virtual void UUIDAndAge(crashpad::UUID* uuid, uint32_t* age) const = 0; + //! \brief Returns the module’s debug file info name. + //! + //! On Windows, this references the PDB file, which contains symbol + //! information held separately from the module itself. On other platforms, + //! this is normally just be the basename of the module, because the debug + //! info file’s name is not relevant even in split-debug scenarios. + //! + //! \sa UUIDAndAge() + virtual std::string DebugFileName() const = 0; + //! \brief Returns string annotations recorded in the module. //! //! This method retrieves annotations recorded in a module. These annotations diff --git a/snapshot/test/test_module_snapshot.cc b/snapshot/test/test_module_snapshot.cc index fc6a4277..cfe9a1de 100644 --- a/snapshot/test/test_module_snapshot.cc +++ b/snapshot/test/test_module_snapshot.cc @@ -27,6 +27,7 @@ TestModuleSnapshot::TestModuleSnapshot() module_type_(kModuleTypeUnknown), age_(0), uuid_(), + debug_file_name_(), annotations_vector_(), annotations_simple_map_() { } @@ -79,6 +80,10 @@ void TestModuleSnapshot::UUIDAndAge(crashpad::UUID* uuid, uint32_t* age) const { *age = age_; } +std::string TestModuleSnapshot::DebugFileName() const { + return debug_file_name_; +} + std::vector TestModuleSnapshot::AnnotationsVector() const { return annotations_vector_; } diff --git a/snapshot/test/test_module_snapshot.h b/snapshot/test/test_module_snapshot.h index 3a59645e..85a5622e 100644 --- a/snapshot/test/test_module_snapshot.h +++ b/snapshot/test/test_module_snapshot.h @@ -64,6 +64,9 @@ class TestModuleSnapshot final : public ModuleSnapshot { uuid_ = uuid; age_ = age; } + void SetDebugFileName(const std::string& debug_file_name) { + debug_file_name_ = debug_file_name; + } void SetAnnotationsVector( const std::vector& annotations_vector) { annotations_vector_ = annotations_vector; @@ -89,6 +92,7 @@ class TestModuleSnapshot final : public ModuleSnapshot { uint16_t* version_3) const override; ModuleType GetModuleType() const override; void UUIDAndAge(crashpad::UUID* uuid, uint32_t* age) const override; + std::string DebugFileName() const override; std::vector AnnotationsVector() const override; std::map AnnotationsSimpleMap() const override; @@ -102,6 +106,7 @@ class TestModuleSnapshot final : public ModuleSnapshot { ModuleType module_type_; uint32_t age_; crashpad::UUID uuid_; + std::string debug_file_name_; std::vector annotations_vector_; std::map annotations_simple_map_; diff --git a/snapshot/win/exception_snapshot_win.cc b/snapshot/win/exception_snapshot_win.cc index 731669ad..07b1c8ea 100644 --- a/snapshot/win/exception_snapshot_win.cc +++ b/snapshot/win/exception_snapshot_win.cc @@ -153,7 +153,7 @@ bool ExceptionSnapshotWin::InitializeFromExceptionPointers( for (DWORD i = 0; i < first_record.NumberParameters; ++i) codes_.push_back(first_record.ExceptionInformation[i]); if (first_record.ExceptionRecord) { - // https://code.google.com/p/crashpad/issues/detail?id=43 + // https://crashpad.chromium.org/bug/43 LOG(WARNING) << "dropping chained ExceptionRecord"; } diff --git a/snapshot/win/exception_snapshot_win_test.cc b/snapshot/win/exception_snapshot_win_test.cc index 7c3b3691..fe626a38 100644 --- a/snapshot/win/exception_snapshot_win_test.cc +++ b/snapshot/win/exception_snapshot_win_test.cc @@ -41,18 +41,16 @@ class RunServerThread : public Thread { public: // Instantiates a thread which will invoke server->Run(delegate, pipe_name); RunServerThread(ExceptionHandlerServer* server, - ExceptionHandlerServer::Delegate* delegate, - const std::string& pipe_name) - : server_(server), delegate_(delegate), pipe_name_(pipe_name) {} + ExceptionHandlerServer::Delegate* delegate) + : server_(server), delegate_(delegate) {} ~RunServerThread() override {} private: // Thread: - void ThreadMain() override { server_->Run(delegate_, pipe_name_); } + void ThreadMain() override { server_->Run(delegate_); } ExceptionHandlerServer* server_; ExceptionHandlerServer::Delegate* delegate_; - std::string pipe_name_; DISALLOW_COPY_AND_ASSIGN(RunServerThread); }; @@ -130,9 +128,8 @@ void TestCrashingChild(const base::string16& directory_modification) { ScopedKernelHANDLE completed(CreateEvent(nullptr, false, false, nullptr)); CrashingDelegate delegate(server_ready.get(), completed.get()); - ExceptionHandlerServer exception_handler_server; - RunServerThread server_thread( - &exception_handler_server, &delegate, pipe_name); + ExceptionHandlerServer exception_handler_server(pipe_name); + RunServerThread server_thread(&exception_handler_server, &delegate); server_thread.Start(); ScopedStopServerAndJoinThread scoped_stop_server_and_join_thread( &exception_handler_server, &server_thread); @@ -233,9 +230,8 @@ void TestDumpWithoutCrashingChild( ScopedKernelHANDLE completed(CreateEvent(nullptr, false, false, nullptr)); SimulateDelegate delegate(server_ready.get(), completed.get()); - ExceptionHandlerServer exception_handler_server; - RunServerThread server_thread( - &exception_handler_server, &delegate, pipe_name); + ExceptionHandlerServer exception_handler_server(pipe_name); + RunServerThread server_thread(&exception_handler_server, &delegate); server_thread.Start(); ScopedStopServerAndJoinThread scoped_stop_server_and_join_thread( &exception_handler_server, &server_thread); diff --git a/snapshot/win/module_snapshot_win.cc b/snapshot/win/module_snapshot_win.cc index 86aae48f..3876fb1e 100644 --- a/snapshot/win/module_snapshot_win.cc +++ b/snapshot/win/module_snapshot_win.cc @@ -27,8 +27,12 @@ namespace internal { ModuleSnapshotWin::ModuleSnapshotWin() : ModuleSnapshot(), name_(), - timestamp_(0), + pdb_name_(), + uuid_(), + pe_image_reader_(), process_reader_(nullptr), + timestamp_(0), + age_(0), initialized_() { } @@ -51,6 +55,13 @@ bool ModuleSnapshotWin::Initialize( return false; } + DWORD age_dword; + if (pe_image_reader_->DebugDirectoryInformation( + &uuid_, &age_dword, &pdb_name_)) { + static_assert(sizeof(DWORD) == sizeof(uint32_t), "unexpected age size"); + age_ = age_dword; + } + INITIALIZATION_STATE_SET_VALID(initialized_); return true; } @@ -137,23 +148,20 @@ ModuleSnapshot::ModuleType ModuleSnapshotWin::GetModuleType() const { void ModuleSnapshotWin::UUIDAndAge(crashpad::UUID* uuid, uint32_t* age) const { INITIALIZATION_STATE_DCHECK_VALID(initialized_); - // TODO(scottmg): Consider passing pdbname through to snapshot. - std::string pdbname; - DWORD age_dword; - if (!pe_image_reader_->DebugDirectoryInformation( - uuid, &age_dword, &pdbname)) { - *uuid = crashpad::UUID(); - *age = 0; - } - static_assert(sizeof(DWORD) == sizeof(uint32_t), "unexpected age size"); - *age = age_dword; + *uuid = uuid_; + *age = age_; +} + +std::string ModuleSnapshotWin::DebugFileName() const { + INITIALIZATION_STATE_DCHECK_VALID(initialized_); + return pdb_name_; } std::vector ModuleSnapshotWin::AnnotationsVector() const { INITIALIZATION_STATE_DCHECK_VALID(initialized_); // These correspond to system-logged things on Mac. We don't currently track // any of these on Windows, but could in the future. - // See https://code.google.com/p/crashpad/issues/detail?id=38. + // See https://crashpad.chromium.org/bug/38. return std::vector(); } diff --git a/snapshot/win/module_snapshot_win.h b/snapshot/win/module_snapshot_win.h index 7cdcc566..54b5539c 100644 --- a/snapshot/win/module_snapshot_win.h +++ b/snapshot/win/module_snapshot_win.h @@ -81,6 +81,7 @@ class ModuleSnapshotWin final : public ModuleSnapshot { uint16_t* version_3) const override; ModuleType GetModuleType() const override; void UUIDAndAge(crashpad::UUID* uuid, uint32_t* age) const override; + std::string DebugFileName() const override; std::vector AnnotationsVector() const override; std::map AnnotationsSimpleMap() const override; @@ -89,9 +90,12 @@ class ModuleSnapshotWin final : public ModuleSnapshot { void GetCrashpadOptionsInternal(CrashpadInfoClientOptions* options); std::wstring name_; - time_t timestamp_; + std::string pdb_name_; + UUID uuid_; scoped_ptr pe_image_reader_; ProcessReaderWin* process_reader_; // weak + time_t timestamp_; + uint32_t age_; InitializationStateDcheck initialized_; DISALLOW_COPY_AND_ASSIGN(ModuleSnapshotWin); diff --git a/snapshot/win/pe_image_reader.cc b/snapshot/win/pe_image_reader.cc index 6f5bb610..b6bb7aa6 100644 --- a/snapshot/win/pe_image_reader.cc +++ b/snapshot/win/pe_image_reader.cc @@ -191,7 +191,7 @@ bool PEImageReader::ReadDebugDirectoryInformation(UUID* uuid, if (*reinterpret_cast(data.get()) != CodeViewRecordPDB70::kSignature) { // TODO(scottmg): Consider supporting other record types, see - // https://code.google.com/p/crashpad/issues/detail?id=47. + // https://crashpad.chromium.org/bug/47. LOG(WARNING) << "encountered non-7.0 CodeView debug record"; continue; } diff --git a/snapshot/win/process_snapshot_win.cc b/snapshot/win/process_snapshot_win.cc index bc3b7b1a..4146ae8d 100644 --- a/snapshot/win/process_snapshot_win.cc +++ b/snapshot/win/process_snapshot_win.cc @@ -353,7 +353,7 @@ void ProcessSnapshotWin::AddMemorySnapshot( // useful for the LDR module lists which are a set of doubly-linked lists, all // pointing to the same module name strings. // TODO(scottmg): A more general version of this, handling overlapping, - // contained, etc. https://code.google.com/p/crashpad/issues/detail?id=61. + // contained, etc. https://crashpad.chromium.org/bug/61. for (const auto& memory_snapshot : *into) { if (memory_snapshot->Address() == address && memory_snapshot->Size() == size) { diff --git a/snapshot/win/thread_snapshot_win.cc b/snapshot/win/thread_snapshot_win.cc index 3593a7e1..9c8c9ea6 100644 --- a/snapshot/win/thread_snapshot_win.cc +++ b/snapshot/win/thread_snapshot_win.cc @@ -112,8 +112,7 @@ uint64_t ThreadSnapshotWin::ThreadSpecificDataAddress() const { std::vector ThreadSnapshotWin::ExtraMemory() const { INITIALIZATION_STATE_DCHECK_VALID(initialized_); // TODO(scottmg): Ensure this region is readable, and make sure we don't - // discard the entire dump if it isn't. - // https://code.google.com/p/crashpad/issues/detail?id=59 + // discard the entire dump if it isn't. https://crashpad.chromium.org/bug/59 return std::vector(1, &teb_); } diff --git a/test/mac/mach_multiprocess.cc b/test/mac/mach_multiprocess.cc index 65ea46df..0458ef3c 100644 --- a/test/mac/mach_multiprocess.cc +++ b/test/mac/mach_multiprocess.cc @@ -92,7 +92,7 @@ void MachMultiprocess::PreFork() { // Set up the parent port and register it with the bootstrap server before // forking, so that it’s guaranteed to be there when the child attempts to // look it up. - info_->service_name = "com.googlecode.crashpad.test.mach_multiprocess."; + info_->service_name = "org.chromium.crashpad.test.mach_multiprocess."; for (int index = 0; index < 16; ++index) { info_->service_name.append(1, base::RandInt('A', 'Z')); } diff --git a/test/scoped_temp_dir_posix.cc b/test/scoped_temp_dir_posix.cc index 2618dc8d..34db76f3 100644 --- a/test/scoped_temp_dir_posix.cc +++ b/test/scoped_temp_dir_posix.cc @@ -33,7 +33,7 @@ void ScopedTempDir::Rename() { // static base::FilePath ScopedTempDir::CreateTemporaryDirectory() { - char dir_template[] = "/tmp/com.googlecode.crashpad.test.XXXXXX"; + char dir_template[] = "/tmp/org.chromium.crashpad.test.XXXXXX"; PCHECK(mkdtemp(dir_template)) << "mkdtemp " << dir_template; return base::FilePath(dir_template); } diff --git a/third_party/gmock/README.crashpad b/third_party/gmock/README.crashpad index 783f942b..430d6c5c 100644 --- a/third_party/gmock/README.crashpad +++ b/third_party/gmock/README.crashpad @@ -1,6 +1,6 @@ Name: Google C++ Mocking Framework (googlemock) Short Name: gmock -URL: https://googlemock.googlecode.com/ +URL: https://github.com/google/googletest/tree/master/googlemock/ Revision: See DEPS License: BSD 3-clause License File: gmock/LICENSE diff --git a/third_party/gtest/README.crashpad b/third_party/gtest/README.crashpad index 5bc23a0d..364f0e17 100644 --- a/third_party/gtest/README.crashpad +++ b/third_party/gtest/README.crashpad @@ -1,6 +1,6 @@ Name: Google C++ Testing Framework (googletest) Short Name: gtest -URL: https://googletest.googlecode.com/ +URL: https://github.com/google/googletest/ Revision: See DEPS License: BSD 3-clause License File: gtest/LICENSE diff --git a/third_party/gyp/README.crashpad b/third_party/gyp/README.crashpad index 6121d7c9..18eb1649 100644 --- a/third_party/gyp/README.crashpad +++ b/third_party/gyp/README.crashpad @@ -1,6 +1,6 @@ Name: GYP (Generate Your Projects) Short Name: gyp -URL: https://gyp.googlecode.com/ +URL: https://gyp.gsrc.io/ Revision: See DEPS License: BSD 3-clause License File: gyp/LICENSE diff --git a/util/mac/service_management_test.mm b/util/mac/service_management_test.mm index 18eb9d60..9f08b575 100644 --- a/util/mac/service_management_test.mm +++ b/util/mac/service_management_test.mm @@ -117,9 +117,9 @@ TEST(ServiceManagement, SubmitRemoveJob) { base::StringPrintf("sleep 10; echo %s", cookie.c_str()); NSString* shell_script_ns = base::SysUTF8ToNSString(shell_script); - const char kJobLabel[] = "com.googlecode.crashpad.test.service_management"; + const char kJobLabel[] = "org.chromium.crashpad.test.service_management"; NSDictionary* job_dictionary_ns = @{ - @LAUNCH_JOBKEY_LABEL : @"com.googlecode.crashpad.test.service_management", + @LAUNCH_JOBKEY_LABEL : @"org.chromium.crashpad.test.service_management", @LAUNCH_JOBKEY_RUNATLOAD : @YES, @LAUNCH_JOBKEY_PROGRAMARGUMENTS : @[ @"/bin/sh", @"-c", shell_script_ns, ], diff --git a/util/mach/child_port.defs b/util/mach/child_port.defs index ce88c115..b227f00f 100644 --- a/util/mach/child_port.defs +++ b/util/mach/child_port.defs @@ -16,10 +16,10 @@ #include // child_port provides an interface for port rights to be transferred between -// tasks. Its expected usage is for child processes to be able to pass port -// rights to their parent processes. A child may wish to give its parent a copy -// of a send right to its own task port, or a child may hold a receive right for -// a server and wish to furnish its parent with a send right to that server. +// tasks. Its expected usage is for processes to be able to pass port rights +// across IPC boundaries. A child process may wish to give its parent a copy of +// of a send right to its own task port, or a parent process may wish to give a +// receive right to a child process that implements a server. // // This Mach subsystem defines the lowest-level interface for these rights to // be transferred. Most users will not user this interface directly, but will @@ -48,9 +48,7 @@ import "util/mach/child_port_types.h"; // secret allowing the server to verify that it has received a request from // the intended client. |server| will reject requests with an invalid // |token|. -// port[in]: A port right to transfer to the server. In expected usage, this may -// be a send or send-once right, and the |server| will reject a receive -// right. It is permissible to specify make-send for a receive right. +// port[in]: A port right to transfer to the server. // // Return value: As this is a “simpleroutine”, the server does not respond to // the client request, and the client does not block waiting for a response diff --git a/util/mach/child_port_handshake.cc b/util/mach/child_port_handshake.cc index 119a53c0..ff779df2 100644 --- a/util/mach/child_port_handshake.cc +++ b/util/mach/child_port_handshake.cc @@ -32,60 +32,60 @@ #include "base/strings/stringprintf.h" #include "util/file/file_io.h" #include "util/mach/child_port.h" +#include "util/mach/child_port_server.h" #include "util/mach/mach_extensions.h" #include "util/mach/mach_message.h" #include "util/mach/mach_message_server.h" #include "util/misc/implicit_cast.h" namespace crashpad { +namespace { -ChildPortHandshake::ChildPortHandshake() +class ChildPortHandshakeServer final : public ChildPortServer::Interface { + public: + ChildPortHandshakeServer(); + ~ChildPortHandshakeServer(); + + mach_port_t RunServer(base::ScopedFD server_write_fd, + ChildPortHandshake::PortRightType port_right_type); + + private: + // ChildPortServer::Interface: + kern_return_t HandleChildPortCheckIn(child_port_server_t server, + child_port_token_t token, + mach_port_t port, + mach_msg_type_name_t right_type, + const mach_msg_trailer_t* trailer, + bool* destroy_request) override; + + child_port_token_t token_; + mach_port_t port_; + mach_msg_type_name_t right_type_; + bool checked_in_; + + DISALLOW_COPY_AND_ASSIGN(ChildPortHandshakeServer); +}; + +ChildPortHandshakeServer::ChildPortHandshakeServer() : token_(0), - pipe_read_(), - pipe_write_(), - child_port_(MACH_PORT_NULL), + port_(MACH_PORT_NULL), + right_type_(MACH_MSG_TYPE_PORT_NONE), checked_in_(false) { - // Use socketpair() instead of pipe(). There is no way to suppress SIGPIPE on - // pipes in Mac OS X 10.6, because the F_SETNOSIGPIPE fcntl() command was not - // introduced until 10.7. - int pipe_fds[2]; - PCHECK(socketpair(AF_UNIX, SOCK_STREAM, PF_UNSPEC, pipe_fds) == 0) - << "socketpair"; - - pipe_read_.reset(pipe_fds[0]); - pipe_write_.reset(pipe_fds[1]); - - // Simulate pipe() semantics by shutting down the “wrong” sides of the socket. - PCHECK(shutdown(pipe_write_.get(), SHUT_RD) == 0) << "shutdown"; - PCHECK(shutdown(pipe_read_.get(), SHUT_WR) == 0) << "shutdown"; - - // SIGPIPE is undesirable when writing to this pipe. Allow broken-pipe writes - // to fail with EPIPE instead. - const int value = 1; - PCHECK(setsockopt( - pipe_write_.get(), SOL_SOCKET, SO_NOSIGPIPE, &value, sizeof(value)) == 0) - << "setsockopt"; } -ChildPortHandshake::~ChildPortHandshake() { +ChildPortHandshakeServer::~ChildPortHandshakeServer() { } -int ChildPortHandshake::ReadPipeFD() const { - DCHECK_NE(pipe_read_.get(), -1); - return pipe_read_.get(); -} - -mach_port_t ChildPortHandshake::RunServer() { - DCHECK_NE(pipe_read_.get(), -1); - pipe_read_.reset(); - - // Transfer ownership of the write pipe into this method’s scope. - base::ScopedFD pipe_write_owner = pipe_write_.Pass(); +mach_port_t ChildPortHandshakeServer::RunServer( + base::ScopedFD server_write_fd, + ChildPortHandshake::PortRightType port_right_type) { + DCHECK_EQ(port_, kMachPortNull); + DCHECK(!checked_in_); + DCHECK(server_write_fd.is_valid()); // Initialize the token and share it with the client via the pipe. token_ = base::RandUint64(); - int pipe_write = pipe_write_owner.get(); - if (!LoggingWriteFile(pipe_write, &token_, sizeof(token_))) { + if (!LoggingWriteFile(server_write_fd.get(), &token_, sizeof(token_))) { LOG(WARNING) << "no client check-in"; return MACH_PORT_NULL; } @@ -97,7 +97,7 @@ mach_port_t ChildPortHandshake::RunServer() { errno = pthread_threadid_np(pthread_self(), &thread_id); PCHECK(errno == 0) << "pthread_threadid_np"; std::string service_name = base::StringPrintf( - "com.googlecode.crashpad.child_port_handshake.%d.%llu.%016llx", + "org.chromium.crashpad.child_port_handshake.%d.%llu.%016llx", getpid(), thread_id, base::RandUint64()); @@ -109,14 +109,15 @@ mach_port_t ChildPortHandshake::RunServer() { // Share the service name with the client via the pipe. uint32_t service_name_length = service_name.size(); - if (!LoggingWriteFile( - pipe_write, &service_name_length, sizeof(service_name_length))) { + if (!LoggingWriteFile(server_write_fd.get(), + &service_name_length, + sizeof(service_name_length))) { LOG(WARNING) << "no client check-in"; return MACH_PORT_NULL; } if (!LoggingWriteFile( - pipe_write, service_name.c_str(), service_name_length)) { + server_write_fd.get(), service_name.c_str(), service_name_length)) { LOG(WARNING) << "no client check-in"; return MACH_PORT_NULL; } @@ -147,7 +148,7 @@ mach_port_t ChildPortHandshake::RunServer() { 0, nullptr); EV_SET(&changelist[1], - pipe_write, + server_write_fd.get(), EVFILT_WRITE, EV_ADD | EV_CLEAR, 0, @@ -162,7 +163,7 @@ mach_port_t ChildPortHandshake::RunServer() { bool blocking = true; DCHECK(!checked_in_); while (!checked_in_) { - DCHECK_EQ(child_port_, kMachPortNull); + DCHECK_EQ(port_, kMachPortNull); // Get a kevent from the kqueue. Block while waiting for an event unless the // write pipe has arrived at EOF, in which case the kevent() should be @@ -230,7 +231,7 @@ mach_port_t ChildPortHandshake::RunServer() { // pipe. Ignore that case. Multiple notifications for that situation // will not be generated because edge triggering (EV_CLEAR) is used // above. - DCHECK_EQ(implicit_cast(event.ident), pipe_write); + DCHECK_EQ(implicit_cast(event.ident), server_write_fd.get()); if (event.flags & EV_EOF) { // There are no readers attached to the write pipe. The client has // closed its side of the pipe. There can be one last shot at @@ -246,19 +247,48 @@ mach_port_t ChildPortHandshake::RunServer() { } } - mach_port_t child_port = MACH_PORT_NULL; - std::swap(child_port_, child_port); - return child_port; + if (port_ == MACH_PORT_NULL) { + return MACH_PORT_NULL; + } + + bool mismatch = false; + switch (port_right_type) { + case ChildPortHandshake::PortRightType::kReceiveRight: + if (right_type_ != MACH_MSG_TYPE_PORT_RECEIVE) { + LOG(ERROR) << "expected receive right, observed " << right_type_; + mismatch = true; + } + break; + case ChildPortHandshake::PortRightType::kSendRight: + if (right_type_ != MACH_MSG_TYPE_PORT_SEND && + right_type_ != MACH_MSG_TYPE_PORT_SEND_ONCE) { + LOG(ERROR) << "expected send or send-once right, observed " + << right_type_; + mismatch = true; + } + break; + } + + if (mismatch) { + MachMessageDestroyReceivedPort(port_, right_type_); + port_ = MACH_PORT_NULL; + return MACH_PORT_NULL; + } + + mach_port_t port = MACH_PORT_NULL; + std::swap(port_, port); + return port; } -kern_return_t ChildPortHandshake::HandleChildPortCheckIn( +kern_return_t ChildPortHandshakeServer::HandleChildPortCheckIn( child_port_server_t server, const child_port_token_t token, mach_port_t port, mach_msg_type_name_t right_type, const mach_msg_trailer_t* trailer, bool* destroy_request) { - DCHECK_EQ(child_port_, kMachPortNull); + DCHECK_EQ(port_, kMachPortNull); + DCHECK(!checked_in_); if (token != token_) { // If the token’s not correct, someone’s attempting to spoof the legitimate @@ -268,19 +298,18 @@ kern_return_t ChildPortHandshake::HandleChildPortCheckIn( } else { checked_in_ = true; - if (right_type == MACH_MSG_TYPE_PORT_RECEIVE) { - // The message needs to carry a send right or a send-once right. This - // isn’t a strict requirement of the protocol, but users of this class - // expect a send right or a send-once right, both of which can be managed - // by base::mac::ScopedMachSendRight. It is invalid to store a receive - // right in that scoper. - LOG(WARNING) << "ignoring MACH_MSG_TYPE_PORT_RECEIVE"; + if (right_type != MACH_MSG_TYPE_PORT_RECEIVE && + right_type != MACH_MSG_TYPE_PORT_SEND && + right_type != MACH_MSG_TYPE_PORT_SEND_ONCE) { + // The message needs to carry a receive, send, or send-once right. + LOG(ERROR) << "invalid right type " << right_type; *destroy_request = true; } else { - // Communicate the child port back to the RunServer(). + // Communicate the child port and right type back to the RunServer(). // *destroy_request is left at false, because RunServer() needs the right // to remain intact. It gives ownership of the right to its caller. - child_port_ = port; + port_ = port; + right_type_ = right_type; } } @@ -288,40 +317,112 @@ kern_return_t ChildPortHandshake::HandleChildPortCheckIn( return MIG_NO_REPLY; } -// static -void ChildPortHandshake::RunClient(int pipe_read, - mach_port_t port, +} // namespace + +ChildPortHandshake::ChildPortHandshake() + : client_read_fd_(), + server_write_fd_() { + // Use socketpair() instead of pipe(). There is no way to suppress SIGPIPE on + // pipes in Mac OS X 10.6, because the F_SETNOSIGPIPE fcntl() command was not + // introduced until 10.7. + int pipe_fds[2]; + PCHECK(socketpair(AF_UNIX, SOCK_STREAM, PF_UNSPEC, pipe_fds) == 0) + << "socketpair"; + + client_read_fd_.reset(pipe_fds[0]); + server_write_fd_.reset(pipe_fds[1]); + + // Simulate pipe() semantics by shutting down the “wrong” sides of the socket. + PCHECK(shutdown(server_write_fd_.get(), SHUT_RD) == 0) << "shutdown SHUT_RD"; + PCHECK(shutdown(client_read_fd_.get(), SHUT_WR) == 0) << "shutdown SHUT_WR"; + + // SIGPIPE is undesirable when writing to this pipe. Allow broken-pipe writes + // to fail with EPIPE instead. + const int value = 1; + PCHECK(setsockopt(server_write_fd_.get(), + SOL_SOCKET, + SO_NOSIGPIPE, + &value, + sizeof(value)) == 0) << "setsockopt"; +} + +ChildPortHandshake::~ChildPortHandshake() { +} + +base::ScopedFD ChildPortHandshake::ClientReadFD() { + DCHECK(client_read_fd_.is_valid()); + return client_read_fd_.Pass(); +} + +base::ScopedFD ChildPortHandshake::ServerWriteFD() { + DCHECK(server_write_fd_.is_valid()); + return server_write_fd_.Pass(); +} + +mach_port_t ChildPortHandshake::RunServer(PortRightType port_right_type) { + client_read_fd_.reset(); + return RunServerForFD(server_write_fd_.Pass(), port_right_type); +} + +bool ChildPortHandshake::RunClient(mach_port_t port, mach_msg_type_name_t right_type) { - base::ScopedFD pipe_read_owner(pipe_read); + server_write_fd_.reset(); + return RunClientForFD(client_read_fd_.Pass(), port, right_type); +} + +// static +mach_port_t ChildPortHandshake::RunServerForFD(base::ScopedFD server_write_fd, + PortRightType port_right_type) { + ChildPortHandshakeServer server; + return server.RunServer(server_write_fd.Pass(), port_right_type); +} + +// static +bool ChildPortHandshake::RunClientForFD(base::ScopedFD client_read_fd, + mach_port_t port, + mach_msg_type_name_t right_type) { + DCHECK(client_read_fd.is_valid()); // Read the token and the service name from the read side of the pipe. child_port_token_t token; std::string service_name; - RunClientInternal_ReadPipe(pipe_read, &token, &service_name); + if (!RunClientInternal_ReadPipe( + client_read_fd.get(), &token, &service_name)) { + return false; + } // Look up the server and check in with it by providing the token and port. - RunClientInternal_SendCheckIn(service_name, token, port, right_type); + return RunClientInternal_SendCheckIn(service_name, token, port, right_type); } // static -void ChildPortHandshake::RunClientInternal_ReadPipe(int pipe_read, +bool ChildPortHandshake::RunClientInternal_ReadPipe(int client_read_fd, child_port_token_t* token, std::string* service_name) { // Read the token from the pipe. - CheckedReadFile(pipe_read, token, sizeof(*token)); + if (!LoggingReadFile(client_read_fd, token, sizeof(*token))) { + return false; + } // Read the service name from the pipe. uint32_t service_name_length; - CheckedReadFile(pipe_read, &service_name_length, sizeof(service_name_length)); + if (!LoggingReadFile( + client_read_fd, &service_name_length, sizeof(service_name_length))) { + return false; + } service_name->resize(service_name_length); - if (!service_name->empty()) { - CheckedReadFile(pipe_read, &(*service_name)[0], service_name_length); + if (!service_name->empty() && + !LoggingReadFile( + client_read_fd, &(*service_name)[0], service_name_length)) { + return false; } + + return true; } // static -void ChildPortHandshake::RunClientInternal_SendCheckIn( +bool ChildPortHandshake::RunClientInternal_SendCheckIn( const std::string& service_name, child_port_token_t token, mach_port_t port, @@ -329,12 +430,19 @@ void ChildPortHandshake::RunClientInternal_SendCheckIn( // Get a send right to the server by looking up the service with the bootstrap // server by name. base::mac::ScopedMachSendRight server_port(BootstrapLookUp(service_name)); - CHECK(server_port.is_valid()); + if (server_port == kMachPortNull) { + return false; + } // Check in with the server. - kern_return_t kr = - child_port_check_in(server_port.get(), token, port, right_type); - MACH_CHECK(kr == KERN_SUCCESS, kr) << "child_port_check_in"; + kern_return_t kr = child_port_check_in( + server_port.get(), token, port, right_type); + if (kr != KERN_SUCCESS) { + MACH_LOG(ERROR, kr) << "child_port_check_in"; + return false; + } + + return true; } } // namespace crashpad diff --git a/util/mach/child_port_handshake.h b/util/mach/child_port_handshake.h index 734220be..8956daf7 100644 --- a/util/mach/child_port_handshake.h +++ b/util/mach/child_port_handshake.h @@ -21,7 +21,7 @@ #include "base/basictypes.h" #include "base/files/scoped_file.h" -#include "util/mach/child_port_server.h" +#include "util/mach/child_port_types.h" namespace crashpad { @@ -31,80 +31,177 @@ class ChildPortHandshakeTest; } // namespace } // namespace test -//! \brief Implements a handshake protocol that allows a parent process to -//! obtain a Mach port right from a child process. +//! \brief Implements a handshake protocol that allows processes to exchange +//! port rights. //! //! Ordinarily, there is no way for parent and child processes to exchange port //! rights, outside of the rights that children inherit from their parents. //! These include task-special ports and exception ports, but all of these have //! system-defined uses, and cannot reliably be replaced: in a multi-threaded -//! parent, it is impossible to temporarily change one an inheritable port while +//! parent, it is impossible to temporarily change an inheritable port while //! maintaining a guarantee that another thread will not attempt to use it, and //! in children, it difficult to guarantee that nothing will attempt to use an //! inheritable port before it can be replaced with the correct one. This latter //! concern is becoming increasingly more pronounced as system libraries perform -//! more operations that rely on an inheritable port in module initializers. +//! more operations that rely on an inherited port in module initializers. //! -//! The protocol implemented by this class involves a server that runs in the -//! parent process. The server is published with the bootstrap server, which the -//! child has access to because the bootstrap port is one of the inherited -//! task-special ports. The parent and child also share a pipe, which the parent -//! can write to and the child can read from. After launching a child process, -//! the parent will write a random token to this pipe, along with the name under -//! which its server has been registered with the bootstrap server. The child -//! can then obtain a send right to this server with `bootstrap_look_up()`, and -//! send a check-in message containing the token value and the port right of its -//! choice by calling `child_port_check_in()`. +//! The protocol implemented by this class involves a server that runs in one +//! process. The server is published with the bootstrap server, which the other +//! process has access to because the bootstrap port is one of the inherited +//! task-special ports. The two processes also share a pipe, which the server +//! can write to and the client can read from. The server will write a random +//! token to this pipe, along with the name under which its service has been +//! registered with the bootstrap server. The client can then obtain a send +//! right to this service with `bootstrap_look_up()`, and send a check-in +//! message containing the token value and the port right of its choice by +//! calling `child_port_check_in()`. //! -//! The inclusion of the token authenticates the child to its parent. This is +//! The inclusion of the token authenticates the client to the server. This is //! necessary because the service is published with the bootstrap server, which -//! opens up access to it to more than the child process. Because the token is -//! passed to the child by a shared pipe, it constitutes a shared secret not +//! opens up access to it to more than the intended client. Because the token is +//! passed to the client by a shared pipe, it constitutes a shared secret not //! known by other processes that may have incidental access to the server. The //! ChildPortHandshake server considers its randomly-generated token valid until //! a client checks in with it. This mechanism is used instead of examining the //! request message’s audit trailer to verify the sender’s process ID because in -//! some process architectures, it may be impossible to verify the child’s -//! process ID. This may happen when the child disassociates from the parent -//! with a double fork(), and the actual client is the parent’s grandchild. In -//! this case, the child would not check in, but the grandchild, in possession -//! of the token, would check in. +//! some process architectures, it may be impossible to verify the client’s +//! process ID. //! //! The shared pipe serves another purpose: the server monitors it for an //! end-of-file (no readers) condition. Once detected, it will stop its blocking -//! wait for a client to check in. This mechanism was chosen over monitoring a -//! child process directly for exit to account for the possibility that the -//! child might disassociate with a double fork(). +//! wait for a client to check in. This mechanism was also chosen for its +//! ability to function properly in diverse process architectures. //! -//! This class can be used to allow a child process to provide its parent with -//! a send right to its task port, in cases where it is desirable for the parent -//! to have such access. It can also be used to allow a child process to -//! establish its own server and provide its parent with a send right to that -//! server, for cases where a service is provided and it is undesirable or -//! impossible to provide it via the bootstrap or launchd interfaces. -class ChildPortHandshake : public ChildPortServer::Interface { +//! This class can be used to allow a child process to provide its parent with a +//! send right to its task port, in cases where it is desirable for the parent +//! to have such access. It can also be used to allow a parent process to +//! transfer a receive right to a child process that implements the server for +//! that right, or for a child process to establish its own server and provide +//! its parent with a send right to that server, for cases where a service is +//! provided and it is undesirable or impossible to provide it via the bootstrap +//! or launchd interfaces. +//! +//! Example parent process, running a client that sends a receive right to its +//! child: +//! \code +//! ChildPortHandshake child_port_handshake; +//! base::ScopedFD server_write_fd = child_port_handshake.ServerWriteFD(); +//! std::string server_write_fd_string = +//! base::StringPrintf("%d", server_write_fd.get()); +//! +//! pid_t pid = fork(); +//! if (pid == 0) { +//! // Child +//! +//! // Close all file descriptors above STDERR_FILENO except for +//! // server_write_fd. Let the child know what file descriptor to use for +//! // server_write_fd by passing it as argv[1]. Example code for the child +//! // process is below. +//! CloseMultipleNowOrOnExec(STDERR_FILENO + 1, server_write_fd.get()); +//! execlp("./child", "child", server_write_fd_string.c_str(), nullptr); +//! } +//! +//! // Parent +//! +//! // Close the child’s end of the pipe. +//! server_write_fd.reset(); +//! +//! // Make a new Mach receive right. +//! base::mac::ScopedMachReceiveRight +//! receive_right(NewMachPort(MACH_PORT_RIGHT_RECEIVE)); +//! +//! // Make a send right corresponding to the receive right. +//! mach_port_t send_right; +//! mach_msg_type_name_t send_right_type; +//! mach_port_extract_right(mach_task_self(), +//! receive_right.get(), +//! MACH_MSG_TYPE_MAKE_SEND, +//! &send_right, +//! &send_right_type); +//! base::mac::ScopedMachSendRight send_right_owner(send_right); +//! +//! // Send the receive right to the child process, retaining the send right +//! // for use in the parent process. +//! if (child_port_handshake.RunClient(receive_right.get(), +//! MACH_MSG_TYPE_MOVE_RECEIVE)) { +//! ignore_result(receive_right.release()); +//! } +//! \endcode +//! +//! Example child process, running a server that receives a receive right from +//! its parent: +//! \code +//! int main(int argc, char* argv[]) { +//! // The parent passed server_write_fd in argv[1]. +//! base::ScopedFD server_write_fd(atoi(argv[1])); +//! +//! // Obtain a receive right from the parent process. +//! base::mac::ScopedMachReceiveRight receive_right( +//! ChildPortHandshake::RunServerForFD( +//! server_write_fd.Pass(), +//! ChildPortHandshake::PortRightType::kReceiveRight)); +//! } +//! \endcode +class ChildPortHandshake { public: - //! \brief Initializes the server. - //! - //! This creates the pipe so that the “read” side can be obtained by calling - //! ReadPipeFD(). - ChildPortHandshake(); + //! \brief Controls whether a receive or send right is expected to be + //! obtained from the client by the server’s call to RunServer(). + enum class PortRightType { + //! \brief The server expects to receive a receive right. + kReceiveRight = 0, + //! \brief The server expects to receive a send or send-once right. + kSendRight, + }; + + ChildPortHandshake(); ~ChildPortHandshake(); //! \brief Obtains the “read” side of the pipe, to be used by the client. //! - //! Callers must obtain this file descriptor and arrange for the caller to - //! have access to it before calling RunServer(). + //! This file descriptor must be passed to RunClientForFD(). //! //! \return The file descriptor that the client should read from. - int ReadPipeFD() const; + base::ScopedFD ClientReadFD(); + + //! \brief Obtains the “write” side of the pipe, to be used by the server. + //! + //! This file descriptor must be passed to RunServerForFD(). + //! + //! \return The file descriptor that the server should write to. + base::ScopedFD ServerWriteFD(); //! \brief Runs the server. //! - //! This method performs these tasks: - //! - Closes the “read” side of the pipe in-process, so that the client - //! process holds the only file descriptor that can read from the pipe. + //! This method closes the “read” side of the pipe in-process, so that the + //! client process holds the only file descriptor that can read from the pipe. + //! It then calls RunServerForFD() using the “write” side of the pipe. If + //! ClientReadFD() has already been called in the server process, the caller + //! must ensure that the file descriptor returned by ClientReadFD() is closed + //! prior to calling this method. + mach_port_t RunServer(PortRightType port_right_type); + + //! \brief Runs the client. + //! + //! This method closes the “write” side of the pipe in-process, so that the + //! server process holds the only file descriptor that can write to the pipe. + //! It then calls RunClientForFD() using the “read” side of the pipe. If + //! ServerWriteFD() has already been called in the client process, the caller + //! must ensure that the file descriptor returned by ServerWriteFD() is closed + //! prior to calling this method. + //! + //! \return `true` on success, `false` on failure with a message logged. + bool RunClient(mach_port_t port, mach_msg_type_name_t right_type); + + //! \brief Runs the server. + //! + //! If a ChildPortHandshake object is available, don’t call this static + //! function. Instead, call RunServer(), which wraps this function. When using + //! this function, the caller is responsible for ensuring that the client + //! “read” side of the pipe is closed in the server process prior to calling + //! this function. + //! + //! This function performs these tasks: //! - Creates a random token and sends it via the pipe. //! - Checks its service in with the bootstrap server, and sends the name //! of its bootstrap service mapping via the pipe. @@ -114,33 +211,35 @@ class ChildPortHandshake : public ChildPortServer::Interface { //! interpret and validate it, and if the message is valid, returns the //! port right extracted from the message. If the message is not valid, //! this method will continue waiting for a valid message. Valid messages - //! are properly formatted and have the correct token. If a valid message - //! carries a send or send-once right, it will be returned. If a valid - //! message contains a receive right, it will be destroyed and - //! `MACH_PORT_NULL` will be returned. If a message is not valid, this + //! are properly formatted and have the correct token. The right carried in + //! a valid message will be returned. If a message is not valid, this //! method will continue waiting for pipe EOF or a valid message. //! - When notified of pipe EOF, returns `MACH_PORT_NULL`. //! - Regardless of return value, destroys the server’s receive right and //! closes the pipe. //! - //! \return On success, the send or send-once right to the port provided by - //! the client. The caller takes ownership of this right. On failure, - //! `MACH_PORT_NULL`, indicating that the client did not check in properly - //! before terminating, where termination is detected by noticing that the - //! read side of the shared pipe has closed. On failure, a message - //! indiciating the nature of the failure will be logged. - mach_port_t RunServer(); - - // ChildPortServer::Interface: - kern_return_t HandleChildPortCheckIn(child_port_server_t server, - child_port_token_t token, - mach_port_t port, - mach_msg_type_name_t right_type, - const mach_msg_trailer_t* trailer, - bool* destroy_request) override; + //! \param[in] port_right_type The port right type expected to be received + //! from the client. If the port right received from the client does not + //! match the expected type, the received port right will be destroyed, + //! and `MACH_PORT_NULL` will be returned. + //! + //! \return On success, the port right provided by the client. The caller + //! takes ownership of this right. On failure, `MACH_PORT_NULL`, + //! indicating that the client did not check in properly before + //! terminating, where termination is detected by detecting that the read + //! side of the shared pipe has closed. On failure, a message indicating + //! the nature of the failure will be logged. + static mach_port_t RunServerForFD(base::ScopedFD server_write_fd, + PortRightType port_right_type); //! \brief Runs the client. //! + //! If a ChildPortHandshake object is available, don’t call this static + //! function. Instead, call RunClient(), which wraps this function. When using + //! this function, the caller is responsible for ensuring that the server + //! “write” side of the pipe is closed in the client process prior to calling + //! this function. + //! //! This function performs these tasks: //! - Reads the token from the pipe. //! - Reads the bootstrap service name from the pipe. @@ -155,32 +254,42 @@ class ChildPortHandshake : public ChildPortServer::Interface { //! check-in to occur without blocking to wait for a reply. //! //! \param[in] pipe_read The “read” side of the pipe shared with the server - //! process. - //! \param[in] port The port that will be passed to the server by + //! process. This function takes ownership of this file descriptor, and + //! will close it prior to returning. + //! \param[in] port The port right that will be passed to the server by //! `child_port_check_in()`. - //! \param[in] right_type The right type to furnish the parent with. If \a + //! \param[in] right_type The right type to furnish the server with. If \a //! port is a send right, this can be `MACH_MSG_TYPE_COPY_SEND` or //! `MACH_MSG_TYPE_MOVE_SEND`. If \a port is a send-once right, this can - //! be `MACH_MSG_TYPE_MOVE_SEND_ONCE`. If \a port is a receive right, - //! this can be `MACH_MSG_TYPE_MAKE_SEND`. `MACH_MSG_TYPE_MOVE_RECEIVE` - //! is supported by the client interface but will be silently rejected by - //! server run by RunServer(), which expects to receive only send or - //! send-once rights. - static void RunClient(int pipe_read, - mach_port_t port, - mach_msg_type_name_t right_type); + //! be `MACH_MSG_TYPE_MOVE_SEND_ONCE`. If \a port is a receive right, this + //! can be `MACH_MSG_TYPE_MAKE_SEND`, `MACH_MSG_TYPE_MAKE_SEND_ONCE`, or + //! `MACH_MSG_TYPE_MOVE_RECEIVE`. + //! + //! \return `true` on success, `false` on failure with a message logged. On + //! failure, the port right corresponding to a \a right_type of + //! `MACH_MSG_TYPE_MOVE_*` is not consumed, and the caller must dispose of + //! the right if necessary. + static bool RunClientForFD(base::ScopedFD client_read_fd, + mach_port_t port, + mach_msg_type_name_t right_type); private: //! \brief Runs the read-from-pipe portion of the client’s side of the //! handshake. This is an implementation detail of RunClient and is only //! exposed for testing purposes. //! + //! When using this function and RunClientInternal_SendCheckIn(), the caller + //! is responsible for closing \a pipe_read at an appropriate time, normally + //! after calling RunClientInternal_SendCheckIn(). + //! //! \param[in] pipe_read The “read” side of the pipe shared with the server //! process. //! \param[out] token The token value read from \a pipe_read. //! \param[out] service_name The service name as registered with the bootstrap //! server, read from \a pipe_read. - static void RunClientInternal_ReadPipe(int pipe_read, + //! + //! \return `true` on success, `false` on failure with a message logged. + static bool RunClientInternal_ReadPipe(int pipe_read, child_port_token_t* token, std::string* service_name); @@ -188,34 +297,25 @@ class ChildPortHandshake : public ChildPortServer::Interface { //! This is an implementation detail of RunClient and is only exposed for //! testing purposes. //! + //! When using this RunClientInternal_ReadPipe() and this function, the caller + //! is responsible for closing the “read” side of the pipe at an appropriate + //! time, normally after calling this function. + //! //! \param[in] service_name The service name as registered with the bootstrap //! server, to be looked up with `bootstrap_look_up()`. //! \param[in] token The token value to provide during check-in. //! \param[in] port The port that will be passed to the server by //! `child_port_check_in()`. - //! \param[in] right_type The right type to furnish the parent with. - static void RunClientInternal_SendCheckIn(const std::string& service_name, + //! \param[in] right_type The right type to furnish the server with. + //! + //! \return `true` on success, `false` on failure with a message logged. + static bool RunClientInternal_SendCheckIn(const std::string& service_name, child_port_token_t token, mach_port_t port, mach_msg_type_name_t right_type); - // Communicates the token from RunServer(), where it’s generated, to - // HandleChildPortCheckIn(), where it’s validated. - child_port_token_t token_; - - base::ScopedFD pipe_read_; - base::ScopedFD pipe_write_; - - // Communicates the port received from the client from - // HandleChildPortCheckIn(), where it’s received, to RunServer(), where it’s - // returned. This is strongly-owned, but ownership is transferred to - // RunServer()’s caller. - mach_port_t child_port_; - - // Communicates that a check-in with a valid token was received by - // HandleChildPortCheckIn(), and that the value of child_port_ should be - // returned to RunServer()’s caller. - bool checked_in_; + base::ScopedFD client_read_fd_; + base::ScopedFD server_write_fd_; friend class test::ChildPortHandshakeTest; diff --git a/util/mach/child_port_handshake_test.cc b/util/mach/child_port_handshake_test.cc index 12ca424c..f77107b5 100644 --- a/util/mach/child_port_handshake_test.cc +++ b/util/mach/child_port_handshake_test.cc @@ -26,161 +26,355 @@ namespace { class ChildPortHandshakeTest : public Multiprocess { public: - enum TestType { - kTestTypeChildChecksIn = 0, - kTestTypeChildDoesNotCheckIn_ReadsPipe, - kTestTypeChildDoesNotCheckIn, - kTestTypeTokenIncorrect, - kTestTypeTokenIncorrectThenCorrect, + enum class ClientProcess { + // The child runs the client and the parent runs the server. + kChildClient = 0, + + // The parent runs the client and the child runs the server. + kParentClient, }; - explicit ChildPortHandshakeTest(TestType test_type) - : Multiprocess(), child_port_handshake_(), test_type_(test_type) {} - ~ChildPortHandshakeTest() {} + enum class TestType { + // The client checks in with the server, transferring a receive right. + kClientChecksIn_ReceiveRight = 0, + + // In this test, the client checks in with the server normally. It sends a + // copy of its bootstrap port to the server, because both parent and child + // should have the same bootstrap port, allowing for verification. + kClientChecksIn_SendRight, + + // The client checks in with the server, transferring a send-once right. + kClientChecksIn_SendOnceRight, + + // In this test, the client reads from its pipe, and subsequently exits + // without checking in. This tests that the server properly detects that it + // has lost its client after sending instructions to it via the pipe, while + // waiting for a check-in message. + kClientDoesNotCheckIn, + + // In this test, the client exits without checking in. This tests that the + // server properly detects that it has lost a client. Whether or not the + // client closes the pipe before the server writes to it is a race, and the + // server needs to be able to detect client loss in both cases, so the + // ClientDoesNotCheckIn_ReadsPipe and NoClient tests also exist to test + // these individual cases more deterministically. + kClientDoesNotCheckIn_ReadsPipe, + + // In this test, the client checks in with the server with an incorrect + // token value and a copy of its own task port. The server should reject the + // message because of the invalid token, and return MACH_PORT_NULL to its + // caller. + kTokenIncorrect, + + // In this test, the client checks in with the server with an incorrect + // token value and a copy of its own task port, and subsequently, the + // correct token value and a copy of its bootstrap port. The server should + // reject the first because of the invalid token, but it should continue + // waiting for a message with a valid token as long as the pipe remains + // open. It should wind wind up returning the bootstrap port, allowing for + // verification. + kTokenIncorrectThenCorrect, + + // The server dies. The failure should be reported in the client. This test + // type is only compatible with ClientProcess::kParentClient. + kServerDies, + }; + + ChildPortHandshakeTest(ClientProcess client_process, TestType test_type) + : Multiprocess(), + child_port_handshake_(), + client_process_(client_process), + test_type_(test_type) { + } + + ~ChildPortHandshakeTest() { + } private: + void RunServer() { + if (test_type_ == TestType::kServerDies) { + return; + } + + base::mac::ScopedMachReceiveRight receive_right; + base::mac::ScopedMachSendRight send_right; + if (test_type_ == TestType::kClientChecksIn_ReceiveRight) { + receive_right.reset(child_port_handshake_.RunServer( + ChildPortHandshake::PortRightType::kReceiveRight)); + } else { + send_right.reset(child_port_handshake_.RunServer( + ChildPortHandshake::PortRightType::kSendRight)); + } + + switch (test_type_) { + case TestType::kClientChecksIn_ReceiveRight: + EXPECT_TRUE(receive_right.is_valid()); + break; + + case TestType::kClientChecksIn_SendRight: + case TestType::kTokenIncorrectThenCorrect: + EXPECT_EQ(bootstrap_port, send_right); + break; + + case TestType::kClientChecksIn_SendOnceRight: + EXPECT_TRUE(send_right.is_valid()); + EXPECT_NE(bootstrap_port, send_right); + break; + + case TestType::kClientDoesNotCheckIn: + case TestType::kClientDoesNotCheckIn_ReadsPipe: + case TestType::kTokenIncorrect: + EXPECT_FALSE(send_right.is_valid()); + break; + + case TestType::kServerDies: + // This was special-cased as an early return above. + FAIL(); + break; + } + } + + void RunClient() { + switch (test_type_) { + case TestType::kClientChecksIn_SendRight: { + ASSERT_TRUE(child_port_handshake_.RunClient(bootstrap_port, + MACH_MSG_TYPE_COPY_SEND)); + break; + } + + case TestType::kClientChecksIn_ReceiveRight: { + mach_port_t receive_right = NewMachPort(MACH_PORT_RIGHT_RECEIVE); + ASSERT_TRUE(child_port_handshake_.RunClient( + receive_right, MACH_MSG_TYPE_MOVE_RECEIVE)); + break; + } + + case TestType::kClientChecksIn_SendOnceRight: { + base::mac::ScopedMachReceiveRight receive_right( + NewMachPort(MACH_PORT_RIGHT_RECEIVE)); + ASSERT_TRUE(child_port_handshake_.RunClient( + receive_right.get(), MACH_MSG_TYPE_MAKE_SEND_ONCE)); + break; + } + + case TestType::kClientDoesNotCheckIn: { + child_port_handshake_.ServerWriteFD().reset(); + child_port_handshake_.ClientReadFD().reset(); + break; + } + + case TestType::kClientDoesNotCheckIn_ReadsPipe: { + // Don’t run the standard client routine. Instead, drain the pipe, which + // will get the parent to the point that it begins waiting for a + // check-in message. Then, exit. The pipe is drained using the same + // implementation that the real client would use. + child_port_handshake_.ServerWriteFD().reset(); + base::ScopedFD client_read_fd = child_port_handshake_.ClientReadFD(); + child_port_token_t token; + std::string service_name; + ASSERT_TRUE(ChildPortHandshake::RunClientInternal_ReadPipe( + client_read_fd.get(), &token, &service_name)); + break; + } + + case TestType::kTokenIncorrect: { + // Don’t run the standard client routine. Instead, read the token and + // service name, mutate the token, and then check in with the bad token. + // The parent should reject the message. + child_port_handshake_.ServerWriteFD().reset(); + base::ScopedFD client_read_fd = child_port_handshake_.ClientReadFD(); + child_port_token_t token; + std::string service_name; + ASSERT_TRUE(ChildPortHandshake::RunClientInternal_ReadPipe( + client_read_fd.get(), &token, &service_name)); + child_port_token_t bad_token = ~token; + ASSERT_TRUE(ChildPortHandshake::RunClientInternal_SendCheckIn( + service_name, + bad_token, + mach_task_self(), + MACH_MSG_TYPE_COPY_SEND)); + break; + } + + case TestType::kTokenIncorrectThenCorrect: { + // Don’t run the standard client routine. Instead, read the token and + // service name. Mutate the token, and check in with the bad token, + // expecting the parent to reject the message. Then, check in with the + // correct token, expecting the parent to accept it. + child_port_handshake_.ServerWriteFD().reset(); + base::ScopedFD client_read_fd = child_port_handshake_.ClientReadFD(); + child_port_token_t token; + std::string service_name; + ASSERT_TRUE(ChildPortHandshake::RunClientInternal_ReadPipe( + client_read_fd.release(), &token, &service_name)); + child_port_token_t bad_token = ~token; + ASSERT_TRUE(ChildPortHandshake::RunClientInternal_SendCheckIn( + service_name, + bad_token, + mach_task_self(), + MACH_MSG_TYPE_COPY_SEND)); + ASSERT_TRUE(ChildPortHandshake::RunClientInternal_SendCheckIn( + service_name, token, bootstrap_port, MACH_MSG_TYPE_COPY_SEND)); + break; + } + + case TestType::kServerDies: { + ASSERT_EQ(ClientProcess::kParentClient, client_process_); + ASSERT_FALSE(child_port_handshake_.RunClient(bootstrap_port, + MACH_MSG_TYPE_COPY_SEND)); + break; + } + } + } + // Multiprocess: void MultiprocessParent() override { - base::mac::ScopedMachSendRight child_port( - child_port_handshake_.RunServer()); - switch (test_type_) { - case kTestTypeChildChecksIn: - case kTestTypeTokenIncorrectThenCorrect: - EXPECT_EQ(bootstrap_port, child_port); + switch (client_process_) { + case ClientProcess::kChildClient: + RunServer(); break; - - case kTestTypeChildDoesNotCheckIn_ReadsPipe: - case kTestTypeChildDoesNotCheckIn: - case kTestTypeTokenIncorrect: - EXPECT_EQ(kMachPortNull, child_port); + case ClientProcess::kParentClient: + RunClient(); break; } } void MultiprocessChild() override { - int read_pipe = child_port_handshake_.ReadPipeFD(); - switch (test_type_) { - case kTestTypeChildChecksIn: - ChildPortHandshake::RunClient( - read_pipe, bootstrap_port, MACH_MSG_TYPE_COPY_SEND); + switch (client_process_) { + case ClientProcess::kChildClient: + RunClient(); break; - - case kTestTypeChildDoesNotCheckIn_ReadsPipe: { - // Don’t run the standard client routine. Instead, drain the pipe, which - // will get the parent to the point that it begins waiting for a - // check-in message. Then, exit. The pipe is drained using the same - // implementation that the real client would use. - child_port_token_t token; - std::string service_name; - ChildPortHandshake::RunClientInternal_ReadPipe( - read_pipe, &token, &service_name); + case ClientProcess::kParentClient: + RunServer(); break; - } - - case kTestTypeChildDoesNotCheckIn: - break; - - case kTestTypeTokenIncorrect: { - // Don’t run the standard client routine. Instead, read the token and - // service name, mutate the token, and then check in with the bad token. - // The parent should reject the message. - child_port_token_t token; - std::string service_name; - ChildPortHandshake::RunClientInternal_ReadPipe( - read_pipe, &token, &service_name); - child_port_token_t bad_token = ~token; - ChildPortHandshake::RunClientInternal_SendCheckIn( - service_name, bad_token, mach_task_self(), MACH_MSG_TYPE_COPY_SEND); - break; - } - - case kTestTypeTokenIncorrectThenCorrect: { - // Don’t run the standard client routine. Instead, read the token and - // service name. Mutate the token, and check in with the bad token, - // expecting the parent to reject the message. Then, check in with the - // correct token, expecting the parent to accept it. - child_port_token_t token; - std::string service_name; - ChildPortHandshake::RunClientInternal_ReadPipe( - read_pipe, &token, &service_name); - child_port_token_t bad_token = ~token; - ChildPortHandshake::RunClientInternal_SendCheckIn( - service_name, bad_token, mach_task_self(), MACH_MSG_TYPE_COPY_SEND); - ChildPortHandshake::RunClientInternal_SendCheckIn( - service_name, token, bootstrap_port, MACH_MSG_TYPE_COPY_SEND); - break; - } } } private: ChildPortHandshake child_port_handshake_; + ClientProcess client_process_; TestType test_type_; DISALLOW_COPY_AND_ASSIGN(ChildPortHandshakeTest); }; -TEST(ChildPortHandshake, ChildChecksIn) { - // In this test, the client checks in with the server normally. It sends a - // copy of its bootstrap port to the server, because both parent and child - // should have the same bootstrap port, allowing for verification. - ChildPortHandshakeTest test(ChildPortHandshakeTest::kTestTypeChildChecksIn); - test.Run(); -} - -TEST(ChildPortHandshake, ChildDoesNotCheckIn) { - // In this test, the client exits without checking in. This tests that the - // server properly detects that it has lost a client. Whether or not the - // client closes the pipe before the server writes to it is a race, and the - // server needs to be able to detect client loss in both cases, so the - // ChildDoesNotCheckIn_ReadsPipe and NoChild tests also exist to test these - // individual cases more deterministically. +TEST(ChildPortHandshake, ChildClientChecksIn_ReceiveRight) { ChildPortHandshakeTest test( - ChildPortHandshakeTest::kTestTypeChildDoesNotCheckIn); + ChildPortHandshakeTest::ClientProcess::kChildClient, + ChildPortHandshakeTest::TestType::kClientChecksIn_ReceiveRight); test.Run(); } -TEST(ChildPortHandshake, ChildDoesNotCheckIn_ReadsPipe) { - // In this test, the client reads from its pipe, and subsequently exits - // without checking in. This tests that the server properly detects that it - // has lost its client after sending instructions to it via the pipe, while - // waiting for a check-in message. +TEST(ChildPortHandshake, ChildClientChecksIn_SendRight) { ChildPortHandshakeTest test( - ChildPortHandshakeTest::kTestTypeChildDoesNotCheckIn_ReadsPipe); + ChildPortHandshakeTest::ClientProcess::kChildClient, + ChildPortHandshakeTest::TestType::kClientChecksIn_SendRight); test.Run(); } -TEST(ChildPortHandshake, TokenIncorrect) { - // In this test, the client checks in with the server with an incorrect token - // value and a copy of its own task port. The server should reject the message - // because of the invalid token, and return MACH_PORT_NULL to its caller. - ChildPortHandshakeTest test(ChildPortHandshakeTest::kTestTypeTokenIncorrect); - test.Run(); -} - -TEST(ChildPortHandshake, TokenIncorrectThenCorrect) { - // In this test, the client checks in with the server with an incorrect token - // value and a copy of its own task port, and subsequently, the correct token - // value and a copy of its bootstrap port. The server should reject the first - // because of the invalid token, but it should continue waiting for a message - // with a valid token as long as the pipe remains open. It should wind wind up - // returning the bootstrap port, allowing for verification. +TEST(ChildPortHandshake, ChildClientChecksIn_SendOnceRight) { ChildPortHandshakeTest test( - ChildPortHandshakeTest::kTestTypeTokenIncorrectThenCorrect); + ChildPortHandshakeTest::ClientProcess::kChildClient, + ChildPortHandshakeTest::TestType::kClientChecksIn_SendOnceRight); test.Run(); } -TEST(ChildPortHandshake, NoChild) { - // In this test, the client never checks in with the parent because the child - // never even runs. This tests that the server properly detects that it has - // no client at all, and does not terminate execution with an error such as +TEST(ChildPortHandshake, ChildClientDoesNotCheckIn) { + ChildPortHandshakeTest test( + ChildPortHandshakeTest::ClientProcess::kChildClient, + ChildPortHandshakeTest::TestType::kClientDoesNotCheckIn); + test.Run(); +} + +TEST(ChildPortHandshake, ChildClientDoesNotCheckIn_ReadsPipe) { + ChildPortHandshakeTest test( + ChildPortHandshakeTest::ClientProcess::kChildClient, + ChildPortHandshakeTest::TestType::kClientDoesNotCheckIn_ReadsPipe); + test.Run(); +} + +TEST(ChildPortHandshake, ChildClientTokenIncorrect) { + ChildPortHandshakeTest test( + ChildPortHandshakeTest::ClientProcess::kChildClient, + ChildPortHandshakeTest::TestType::kTokenIncorrect); + test.Run(); +} + +TEST(ChildPortHandshake, ChildClientTokenIncorrectThenCorrect) { + ChildPortHandshakeTest test( + ChildPortHandshakeTest::ClientProcess::kChildClient, + ChildPortHandshakeTest::TestType::kTokenIncorrectThenCorrect); + test.Run(); +} + +TEST(ChildPortHandshake, ParentClientChecksIn_ReceiveRight) { + ChildPortHandshakeTest test( + ChildPortHandshakeTest::ClientProcess::kParentClient, + ChildPortHandshakeTest::TestType::kClientChecksIn_ReceiveRight); + test.Run(); +} + +TEST(ChildPortHandshake, ParentClientChecksIn_SendRight) { + ChildPortHandshakeTest test( + ChildPortHandshakeTest::ClientProcess::kParentClient, + ChildPortHandshakeTest::TestType::kClientChecksIn_SendRight); + test.Run(); +} + +TEST(ChildPortHandshake, ParentClientChecksIn_SendOnceRight) { + ChildPortHandshakeTest test( + ChildPortHandshakeTest::ClientProcess::kParentClient, + ChildPortHandshakeTest::TestType::kClientChecksIn_SendOnceRight); + test.Run(); +} + +TEST(ChildPortHandshake, ParentClientDoesNotCheckIn) { + ChildPortHandshakeTest test( + ChildPortHandshakeTest::ClientProcess::kParentClient, + ChildPortHandshakeTest::TestType::kClientDoesNotCheckIn); + test.Run(); +} + +TEST(ChildPortHandshake, ParentClientDoesNotCheckIn_ReadsPipe) { + ChildPortHandshakeTest test( + ChildPortHandshakeTest::ClientProcess::kParentClient, + ChildPortHandshakeTest::TestType::kClientDoesNotCheckIn_ReadsPipe); + test.Run(); +} + +TEST(ChildPortHandshake, ParentClientTokenIncorrect) { + ChildPortHandshakeTest test( + ChildPortHandshakeTest::ClientProcess::kParentClient, + ChildPortHandshakeTest::TestType::kTokenIncorrect); + test.Run(); +} + +TEST(ChildPortHandshake, ParentClientTokenIncorrectThenCorrect) { + ChildPortHandshakeTest test( + ChildPortHandshakeTest::ClientProcess::kParentClient, + ChildPortHandshakeTest::TestType::kTokenIncorrectThenCorrect); + test.Run(); +} + +TEST(ChildPortHandshake, ParentClientServerDies) { + ChildPortHandshakeTest test( + ChildPortHandshakeTest::ClientProcess::kParentClient, + ChildPortHandshakeTest::TestType::kServerDies); + test.Run(); +} + +TEST(ChildPortHandshake, NoClient) { + // In this test, the client never checks in with the server because it never + // even runs. This tests that the server properly detects that it has no + // client at all, and does not terminate execution with an error such as // “broken pipe” when attempting to send instructions to the client. This test - // is similar to ChildDoesNotCheckIn, but because there’s no child at all, the - // server is guaranteed to see that its pipe partner is gone. + // is similar to kClientDoesNotCheckIn, but because there’s no client at all, + // the server is guaranteed to see that its pipe partner is gone. ChildPortHandshake child_port_handshake; - base::mac::ScopedMachSendRight child_port(child_port_handshake.RunServer()); - EXPECT_EQ(kMachPortNull, child_port); + base::mac::ScopedMachSendRight child_port(child_port_handshake.RunServer( + ChildPortHandshake::PortRightType::kSendRight)); + EXPECT_FALSE(child_port.is_valid()); } } // namespace diff --git a/util/mach/mach_extensions_test.cc b/util/mach/mach_extensions_test.cc index 04af5dfe..99ddee97 100644 --- a/util/mach/mach_extensions_test.cc +++ b/util/mach/mach_extensions_test.cc @@ -138,7 +138,7 @@ TEST(MachExtensions, BootstrapCheckInAndLookUp) { report_crash(BootstrapLookUp("com.apple.ReportCrash")); EXPECT_NE(report_crash, kMachPortNull); - std::string service_name = "com.googlecode.crashpad.test.bootstrap_check_in."; + std::string service_name = "org.chromium.crashpad.test.bootstrap_check_in."; for (int index = 0; index < 16; ++index) { service_name.append(1, base::RandInt('A', 'Z')); } diff --git a/util/mach/mach_message.cc b/util/mach/mach_message.cc index 698bd1ff..30c3a8cf 100644 --- a/util/mach/mach_message.cc +++ b/util/mach/mach_message.cc @@ -20,6 +20,7 @@ #include #include "base/logging.h" +#include "base/mac/mach_logging.h" #include "util/misc/clock.h" #include "util/misc/implicit_cast.h" @@ -249,4 +250,37 @@ pid_t AuditPIDFromMachMessageTrailer(const mach_msg_trailer_t* trailer) { return audit_pid; } +bool MachMessageDestroyReceivedPort(mach_port_t port, + mach_msg_type_name_t port_right_type) { + // This implements a subset of 10.10.5 + // xnu-2782.40.9/libsyscall/mach/mach_msg.c mach_msg_destroy_port() that deals + // only with port rights that can be received in Mach messages. + switch (port_right_type) { + case MACH_MSG_TYPE_PORT_RECEIVE: { + kern_return_t kr = mach_port_mod_refs( + mach_task_self(), port, MACH_PORT_RIGHT_RECEIVE, -1); + if (kr != KERN_SUCCESS) { + MACH_LOG(ERROR, kr) << "mach_port_mod_refs"; + return false; + } + return true; + } + + case MACH_MSG_TYPE_PORT_SEND: + case MACH_MSG_TYPE_PORT_SEND_ONCE: { + kern_return_t kr = mach_port_deallocate(mach_task_self(), port); + if (kr != KERN_SUCCESS) { + MACH_LOG(ERROR, kr) << "mach_port_deallocate"; + return false; + } + return true; + } + + default: { + LOG(ERROR) << "unexpected port right type " << port_right_type; + return false; + } + } +} + } // namespace crashpad diff --git a/util/mach/mach_message.h b/util/mach/mach_message.h index be882c17..2fd81511 100644 --- a/util/mach/mach_message.h +++ b/util/mach/mach_message.h @@ -174,6 +174,21 @@ const mach_msg_trailer_t* MachMessageTrailerFromHeader( //! audit information. pid_t AuditPIDFromMachMessageTrailer(const mach_msg_trailer_t* trailer); +//! \brief Destroys or deallocates a Mach port received in a Mach message. +//! +//! This function disposes of port rights received in a Mach message. Receive +//! rights will be destroyed with `mach_port_mod_refs()`. Send and send-once +//! rights will be deallocated with `mach_port_deallocate()`. +//! +//! \param[in] port The port to destroy or deallocate. +//! \param[in] port_right_type The right type held for \a port: +//! `MACH_MSG_TYPE_PORT_RECEIVE`, `MACH_MSG_TYPE_PORT_SEND`, or +//! `MACH_MSG_TYPE_PORT_SEND_ONCE`. +//! +//! \return `true` on success, or `false` on failure with a message logged. +bool MachMessageDestroyReceivedPort(mach_port_t port, + mach_msg_type_name_t port_right_type); + } // namespace crashpad #endif // CRASHPAD_UTIL_MACH_MACH_MESSAGE_H_ diff --git a/util/mach/mach_message_server.h b/util/mach/mach_message_server.h index 65de85c0..484ee8bb 100644 --- a/util/mach/mach_message_server.h +++ b/util/mach/mach_message_server.h @@ -157,8 +157,8 @@ class MachMessageServer { //! #kPersistent, the timeout applies to the overall duration of this //! function, not to any individual `mach_msg()` call. //! - //! \return On success, `KERN_SUCCESS` (when \a persistent is #kOneShot) or - //! `MACH_RCV_TIMED_OUT` (when \a persistent is #kOneShot and \a + //! \return On success, `MACH_MSG_SUCCESS` (when \a persistent is #kOneShot) + //! or `MACH_RCV_TIMED_OUT` (when \a persistent is #kOneShot and \a //! timeout_ms is not #kMachMessageTimeoutWaitIndefinitely). This function //! has no successful return value when \a persistent is #kPersistent and //! \a timeout_ms is #kMachMessageTimeoutWaitIndefinitely. On failure, diff --git a/util/mach/mach_message_server_test.cc b/util/mach/mach_message_server_test.cc index daa64a93..a59554b3 100644 --- a/util/mach/mach_message_server_test.cc +++ b/util/mach/mach_message_server_test.cc @@ -216,8 +216,8 @@ class TestMachMessageServer : public MachMessageServer::Interface, MACH_MSGH_BITS(MACH_MSG_TYPE_MOVE_SEND, MACH_MSG_TYPE_MOVE_SEND) | (options_.client_send_complex ? MACH_MSGH_BITS_COMPLEX : 0); EXPECT_EQ(expect_msgh_bits, request->header.msgh_bits); - EXPECT_EQ(options_.client_send_large ? sizeof(LargeRequestMessage) : - sizeof(RequestMessage), + EXPECT_EQ(options_.client_send_large ? sizeof(LargeRequestMessage) + : sizeof(RequestMessage), request->header.msgh_size); if (options_.client_reply_port_type == Options::kReplyPortNormal) { EXPECT_EQ(RemotePort(), request->header.msgh_remote_port); @@ -277,8 +277,8 @@ class TestMachMessageServer : public MachMessageServer::Interface, std::set MachMessageServerRequestIDs() override { const mach_msg_id_t request_ids[] = {kRequestMessageID}; - return std::set( - &request_ids[0], &request_ids[arraysize(request_ids)]); + return std::set(&request_ids[0], + &request_ids[arraysize(request_ids)]); } mach_msg_size_t MachMessageServerRequestSize() override { @@ -368,8 +368,8 @@ class TestMachMessageServer : public MachMessageServer::Interface, EXPECT_EQ(MACH_PORT_TYPE_SEND, type); // Destroy the resources here. - kr = mach_port_deallocate( - mach_task_self(), parent_complex_message_port_); + kr = mach_port_deallocate(mach_task_self(), + parent_complex_message_port_); EXPECT_EQ(KERN_SUCCESS, kr) << MachErrorMessage(kr, "mach_port_deallocate"); } @@ -378,8 +378,8 @@ class TestMachMessageServer : public MachMessageServer::Interface, // this task so soon. It’s possible that something else in this task could // have reused the name, but it’s unlikely for that to have happened in // this test environment. - kr = mach_port_type( - mach_task_self(), parent_complex_message_port_, &type); + kr = + mach_port_type(mach_task_self(), parent_complex_message_port_, &type); EXPECT_EQ(KERN_INVALID_NAME, kr) << MachErrorMessage(kr, "mach_port_type"); } diff --git a/util/mach/mach_message_test.cc b/util/mach/mach_message_test.cc index 5e79b212..d77f0dd3 100644 --- a/util/mach/mach_message_test.cc +++ b/util/mach/mach_message_test.cc @@ -16,6 +16,7 @@ #include +#include "base/basictypes.h" #include "base/mac/scoped_mach_port.h" #include "gtest/gtest.h" #include "test/mac/mach_errors.h" @@ -148,6 +149,55 @@ TEST(MachMessage, AuditPIDFromMachMessageTrailer) { EXPECT_EQ(getpid(), AuditPIDFromMachMessageTrailer(&receive.trailer)); } +TEST(MachMessage, MachMessageDestroyReceivedPort) { + mach_port_t port = NewMachPort(MACH_PORT_RIGHT_RECEIVE); + ASSERT_NE(kMachPortNull, port); + EXPECT_TRUE(MachMessageDestroyReceivedPort(port, MACH_MSG_TYPE_PORT_RECEIVE)); + + base::mac::ScopedMachReceiveRight receive( + NewMachPort(MACH_PORT_RIGHT_RECEIVE)); + mach_msg_type_name_t right_type; + kern_return_t kr = mach_port_extract_right(mach_task_self(), + receive.get(), + MACH_MSG_TYPE_MAKE_SEND, + &port, + &right_type); + ASSERT_EQ(KERN_SUCCESS, kr) + << MachErrorMessage(kr, "mach_port_extract_right"); + ASSERT_EQ(receive, port); + ASSERT_EQ(implicit_cast(MACH_MSG_TYPE_PORT_SEND), + right_type); + EXPECT_TRUE(MachMessageDestroyReceivedPort(port, MACH_MSG_TYPE_PORT_SEND)); + + kr = mach_port_extract_right(mach_task_self(), + receive.get(), + MACH_MSG_TYPE_MAKE_SEND_ONCE, + &port, + &right_type); + ASSERT_EQ(KERN_SUCCESS, kr) + << MachErrorMessage(kr, "mach_port_extract_right"); + ASSERT_NE(kMachPortNull, port); + EXPECT_NE(receive, port); + ASSERT_EQ(implicit_cast(MACH_MSG_TYPE_PORT_SEND_ONCE), + right_type); + EXPECT_TRUE( + MachMessageDestroyReceivedPort(port, MACH_MSG_TYPE_PORT_SEND_ONCE)); + + kr = mach_port_extract_right(mach_task_self(), + receive.get(), + MACH_MSG_TYPE_MAKE_SEND, + &port, + &right_type); + ASSERT_EQ(KERN_SUCCESS, kr) + << MachErrorMessage(kr, "mach_port_extract_right"); + ASSERT_EQ(receive, port); + ASSERT_EQ(implicit_cast(MACH_MSG_TYPE_PORT_SEND), + right_type); + EXPECT_TRUE(MachMessageDestroyReceivedPort(port, MACH_MSG_TYPE_PORT_RECEIVE)); + ignore_result(receive.release()); + EXPECT_TRUE(MachMessageDestroyReceivedPort(port, MACH_MSG_TYPE_PORT_SEND)); +} + } // namespace } // namespace test } // namespace crashpad diff --git a/util/win/capture_context.asm b/util/win/capture_context.asm index 933f8828..56efec7f 100644 --- a/util/win/capture_context.asm +++ b/util/win/capture_context.asm @@ -351,8 +351,7 @@ $FXSave: ; Free the stack space used for the temporary fxsave area. lea esp, [ebp-8] - ; TODO(mark): AVX/xsave support. - ; https://code.google.com/p/crashpad/issues/detail?id=58 + ; TODO(mark): AVX/xsave support. https://crashpad.chromium.org/bug/58 $FXSaveDone: ; fnsave reinitializes the FPU with an implicit finit operation, so use frstor @@ -491,8 +490,7 @@ CAPTURECONTEXT_SYMBOL proc frame ; declared as 16-byte-aligned, which is correct for this operation. fxsave [rcx.CONTEXT].c_FltSave - ; TODO(mark): AVX/xsave support. - ; https://code.google.com/p/crashpad/issues/detail?id=58 + ; TODO(mark): AVX/xsave support. https://crashpad.chromium.org/bug/58 ; The register parameter home address fields aren’t used, so zero them out. mov [rcx.CONTEXT].c_P1Home, 0 diff --git a/util/win/exception_handler_server.cc b/util/win/exception_handler_server.cc index aba3ffa0..234d3149 100644 --- a/util/win/exception_handler_server.cc +++ b/util/win/exception_handler_server.cc @@ -234,8 +234,9 @@ class ClientData { ExceptionHandlerServer::Delegate::~Delegate() { } -ExceptionHandlerServer::ExceptionHandlerServer() - : port_(CreateIoCompletionPort(INVALID_HANDLE_VALUE, nullptr, 0, 1)), +ExceptionHandlerServer::ExceptionHandlerServer(const std::string& pipe_name) + : pipe_name_(pipe_name), + port_(CreateIoCompletionPort(INVALID_HANDLE_VALUE, nullptr, 0, 1)), clients_lock_(), clients_() { } @@ -243,13 +244,12 @@ ExceptionHandlerServer::ExceptionHandlerServer() ExceptionHandlerServer::~ExceptionHandlerServer() { } -void ExceptionHandlerServer::Run(Delegate* delegate, - const std::string& pipe_name) { +void ExceptionHandlerServer::Run(Delegate* delegate) { uint64_t shutdown_token = base::RandUint64(); // We create two pipe instances, so that there's one listening while the // PipeServiceProc is processing a registration. ScopedKernelHANDLE thread_handles[2]; - base::string16 pipe_name_16(base::UTF8ToUTF16(pipe_name)); + base::string16 pipe_name_16(base::UTF8ToUTF16(pipe_name_)); for (int i = 0; i < arraysize(thread_handles); ++i) { HANDLE pipe = CreateNamedPipe(pipe_name_16.c_str(), @@ -311,7 +311,7 @@ void ExceptionHandlerServer::Run(Delegate* delegate, message.type = ClientToServerMessage::kShutdown; message.shutdown.token = shutdown_token; ServerToClientMessage response; - SendToCrashHandlerServer(base::UTF8ToUTF16(pipe_name), + SendToCrashHandlerServer(pipe_name_16, reinterpret_cast(message), &response); } diff --git a/util/win/exception_handler_server.h b/util/win/exception_handler_server.h index 6928a6d9..d1243406 100644 --- a/util/win/exception_handler_server.h +++ b/util/win/exception_handler_server.h @@ -61,16 +61,18 @@ class ExceptionHandlerServer { }; //! \brief Constructs the exception handling server. - ExceptionHandlerServer(); + //! + //! \param[in] pipe_name The name of the pipe to listen on. Must be of the + //! form "\\.\pipe\". + explicit ExceptionHandlerServer(const std::string& pipe_name); + ~ExceptionHandlerServer(); //! \brief Runs the exception-handling server. //! //! \param[in] delegate The interface to which the exceptions are delegated //! when they are caught in Run(). Ownership is not transferred. - //! \param[in] pipe_name The name of the pipe to listen on. Must be of the - //! form "\\.\pipe\". - void Run(Delegate* delegate, const std::string& pipe_name); + void Run(Delegate* delegate); //! \brief Stops the exception-handling server. Returns immediately. The //! object must not be destroyed until Run() returns. @@ -84,6 +86,7 @@ class ExceptionHandlerServer { static void __stdcall OnNonCrashDumpEvent(void* ctx, BOOLEAN); static void __stdcall OnProcessEnd(void* ctx, BOOLEAN); + std::string pipe_name_; ScopedKernelHANDLE port_; base::Lock clients_lock_; diff --git a/util/win/exception_handler_server_test.cc b/util/win/exception_handler_server_test.cc index 0d87086b..9fe593b5 100644 --- a/util/win/exception_handler_server_test.cc +++ b/util/win/exception_handler_server_test.cc @@ -36,20 +36,18 @@ namespace { // Runs the ExceptionHandlerServer on a background thread. class RunServerThread : public Thread { public: - // Instantiates a thread which will invoke server->Run(delegate, pipe_name). + // Instantiates a thread which will invoke server->Run(delegate). RunServerThread(ExceptionHandlerServer* server, - ExceptionHandlerServer::Delegate* delegate, - const std::string& pipe_name) - : server_(server), delegate_(delegate), pipe_name_(pipe_name) {} + ExceptionHandlerServer::Delegate* delegate) + : server_(server), delegate_(delegate) {} ~RunServerThread() override {} private: // Thread: - void ThreadMain() override { server_->Run(delegate_, pipe_name_); } + void ThreadMain() override { server_->Run(delegate_); } ExceptionHandlerServer* server_; ExceptionHandlerServer::Delegate* delegate_; - std::string pipe_name_; DISALLOW_COPY_AND_ASSIGN(RunServerThread); }; @@ -85,8 +83,8 @@ class ExceptionHandlerServerTest : public testing::Test { base::StringPrintf("%08x", GetCurrentProcessId())), server_ready_(CreateEvent(nullptr, false, false, nullptr)), delegate_(server_ready_.get()), - server_(), - server_thread_(&server_, &delegate_, pipe_name_) {} + server_(pipe_name_), + server_thread_(&server_, &delegate_) {} TestDelegate& delegate() { return delegate_; } ExceptionHandlerServer& server() { return server_; } diff --git a/util/win/process_info.h b/util/win/process_info.h index f8ed4ae4..ca1935bb 100644 --- a/util/win/process_info.h +++ b/util/win/process_info.h @@ -180,7 +180,7 @@ class ProcessInfo { std::vector memory_info_; // Handles() is logically const, but updates this member on first retrieval. - // See https://code.google.com/p/crashpad/issues/detail?id=9. + // See https://crashpad.chromium.org/bug/9. mutable std::vector handles_; bool is_64_bit_;