From 3ee9d891d9e2985b56be1d9b3c389b360a3cb96d Mon Sep 17 00:00:00 2001 From: Scott Graham Date: Thu, 29 Oct 2015 10:48:23 -0700 Subject: [PATCH 1/6] win: Plumb module PDB name through snapshot R=mark@chromium.org BUG=chromium:546288 Review URL: https://codereview.chromium.org/1415543003 . --- minidump/minidump_module_writer.cc | 10 +------ minidump/minidump_module_writer_test.cc | 9 +++++- snapshot/mac/module_snapshot_mac.cc | 6 ++++ snapshot/mac/module_snapshot_mac.h | 1 + snapshot/minidump/module_snapshot_minidump.cc | 6 ++++ snapshot/minidump/module_snapshot_minidump.h | 1 + snapshot/module_snapshot.h | 12 ++++++++ snapshot/test/test_module_snapshot.cc | 5 ++++ snapshot/test/test_module_snapshot.h | 5 ++++ snapshot/win/module_snapshot_win.cc | 30 ++++++++++++------- snapshot/win/module_snapshot_win.h | 6 +++- 11 files changed, 69 insertions(+), 22 deletions(-) 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/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..6805d71c 100644 --- a/snapshot/minidump/module_snapshot_minidump.cc +++ b/snapshot/minidump/module_snapshot_minidump.cc @@ -118,6 +118,12 @@ void ModuleSnapshotMinidump::UUIDAndAge(crashpad::UUID* uuid, *age = 0; } +std::string ModuleSnapshotMinidump::DebugFileName() const { + INITIALIZATION_STATE_DCHECK_VALID(initialized_); + NOTREACHED(); // https://code.google.com/p/crashpad/issues/detail?id=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/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/module_snapshot_win.cc b/snapshot/win/module_snapshot_win.cc index 86aae48f..f45bedc6 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,16 +148,13 @@ 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 { 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); From 062138106c247191bb611de861ffe2cc83abc560 Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Thu, 29 Oct 2015 14:14:15 -0400 Subject: [PATCH 2/6] mac: ChildPortHandshake: allow receive rights to be sent The intended use is to flip the client-server relationship in CrashpadClient so that the initial client (parent process) furnishes the handler process with a receive right. The parent can optionally receive a port-destroyed notification allowing it to restart the handler if it exits prematurely. R=rsesek@chromium.org Review URL: https://codereview.chromium.org/1408473002 . --- client/crashpad_client_mac.cc | 11 +- handler/main.cc | 10 +- util/mach/child_port.defs | 12 +- util/mach/child_port_handshake.cc | 256 ++++++++++----- util/mach/child_port_handshake.h | 288 +++++++++++------ util/mach/child_port_handshake_test.cc | 426 ++++++++++++++++++------- util/mach/mach_message.cc | 34 ++ util/mach/mach_message.h | 15 + util/mach/mach_message_server_test.cc | 16 +- util/mach/mach_message_test.cc | 50 +++ 10 files changed, 812 insertions(+), 306 deletions(-) diff --git a/client/crashpad_client_mac.cc b/client/crashpad_client_mac.cc index 5797fba8..b95c07aa 100644 --- a/client/crashpad_client_mac.cc +++ b/client/crashpad_client_mac.cc @@ -97,7 +97,7 @@ bool CrashpadClient::StartHandler( // 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(); + base::ScopedFD client_read_fd = child_port_handshake.ClientReadFD(); // Use handler as argv[0], followed by arguments directed by this method’s // parameters and a --handshake-fd argument. |arguments| are added first so @@ -119,7 +119,7 @@ bool CrashpadClient::StartHandler( argv.push_back( FormatArgumentString("annotation", kv.first + '=' + kv.second)); } - argv.push_back(FormatArgumentInt("handshake-fd", handshake_fd)); + argv.push_back(FormatArgumentInt("handshake-fd", client_read_fd.get())); // 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 @@ -181,7 +181,7 @@ bool CrashpadClient::StartHandler( // Grandchild process. - CloseMultipleNowOrOnExec(STDERR_FILENO + 1, handshake_fd); + CloseMultipleNowOrOnExec(STDERR_FILENO + 1, client_read_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 @@ -193,6 +193,8 @@ bool CrashpadClient::StartHandler( // Parent process. + client_read_fd.reset(); + // waitpid() for the child, so that it does not become a zombie process. The // child normally exits quickly. int status; @@ -209,7 +211,8 @@ bool CrashpadClient::StartHandler( } // Rendezvous with the handler running in the grandchild process. - exception_port_.reset(child_port_handshake.RunServer()); + exception_port_.reset(child_port_handshake.RunServer( + ChildPortHandshake::PortRightType::kSendRight)); return exception_port_.is_valid(); } diff --git a/handler/main.cc b/handler/main.cc index 5f192f92..860951dc 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" @@ -219,9 +220,12 @@ int HandlerMain(int argc, char* argv[]) { #if defined(OS_MACOSX) CloseStdinAndStdout(); - ChildPortHandshake::RunClient(options.handshake_fd, - exception_handler_server.receive_port(), - MACH_MSG_TYPE_MAKE_SEND); + if (!ChildPortHandshake::RunClientForFD( + base::ScopedFD(options.handshake_fd), + exception_handler_server.receive_port(), + MACH_MSG_TYPE_MAKE_SEND)) { + return EXIT_FAILURE; + } #endif // OS_MACOSX scoped_ptr database(CrashReportDatabase::Initialize( 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..e45d6aee 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; } @@ -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..c1d7ab9a 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); +//! +//! 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); +//! 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, +//! 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_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_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 From 9d03d54d0ba1aa19d90ee226bb6fec9f901d2725 Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Thu, 29 Oct 2015 15:12:23 -0400 Subject: [PATCH 3/6] win: Construct ExceptionHandlerServer() with its pipe argument MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This allows better code sharing in crashpad_handler’s main(). It doesn’t look like much of an improvement now, but a separate change will cause the Mac ExceptionHandlerServer() to be constructed with an argument. It will be beneficial for Mac and Windows to be able to share the Run() call. R=scottmg@chromium.org Review URL: https://codereview.chromium.org/1402333004 . --- handler/main.cc | 8 +++----- snapshot/win/exception_snapshot_win_test.cc | 18 +++++++----------- util/win/exception_handler_server.cc | 12 ++++++------ util/win/exception_handler_server.h | 11 +++++++---- util/win/exception_handler_server_test.cc | 14 ++++++-------- 5 files changed, 29 insertions(+), 34 deletions(-) diff --git a/handler/main.cc b/handler/main.cc index 860951dc..64a49d56 100644 --- a/handler/main.cc +++ b/handler/main.cc @@ -216,9 +216,9 @@ int HandlerMain(int argc, char* argv[]) { } #endif +#if defined(OS_MACOSX) ExceptionHandlerServer exception_handler_server; -#if defined(OS_MACOSX) CloseStdinAndStdout(); if (!ChildPortHandshake::RunClientForFD( base::ScopedFD(options.handshake_fd), @@ -226,6 +226,8 @@ int HandlerMain(int argc, char* argv[]) { MACH_MSG_TYPE_MAKE_SEND)) { return EXIT_FAILURE; } +#elif defined(OS_WIN) + ExceptionHandlerServer exception_handler_server(options.pipe_name); #endif // OS_MACOSX scoped_ptr database(CrashReportDatabase::Initialize( @@ -241,11 +243,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/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/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_; } From fc7d8b3a27e10bfd400256b941353736383c0979 Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Thu, 29 Oct 2015 18:09:03 -0400 Subject: [PATCH 4/6] mac: Make crashpad_handler get its receive right from its client Previously, crashpad_handler made its own receive right, and transferred a corresponding send right to its client. There are two advantages to making the receive right in the client: - It is possible to monitor the receive right for a port-destroyed notificaiton in the client, allowing the handler to be restarted if it dies. - For the future run-from-launchd mode (bug crashpad:25), the handler will obtain its receive right from the bootstrap server instead of making its own. Having the handler get its receive right from different sources allows more code to be shared than if it were to sometimes get a receive right and sometimes make a receive right and transfer a send right. This includes a restructuring in crashpad_client_mac.cc that will make it easier to give it an option to restart crashpad_handler if it dies. The handler starting logic should all behave the same as before. BUG=crashpad:68 R=rsesek@chromium.org Review URL: https://codereview.chromium.org/1409073013 . --- client/crashpad_client_mac.cc | 317 ++++++++++++-------- handler/mac/exception_handler_server.cc | 13 +- handler/mac/exception_handler_server.h | 28 +- handler/main.cc | 17 +- snapshot/win/exception_snapshot_win_test.cc | 18 +- util/mach/child_port_handshake.h | 8 +- util/mach/mach_message_server.h | 4 +- util/win/exception_handler_server.cc | 12 +- util/win/exception_handler_server.h | 11 +- util/win/exception_handler_server_test.cc | 14 +- 10 files changed, 263 insertions(+), 179 deletions(-) diff --git a/client/crashpad_client_mac.cc b/client/crashpad_client_mac.cc index b95c07aa..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,127 +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; - base::ScopedFD client_read_fd = child_port_handshake.ClientReadFD(); - - // 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", client_read_fd.get())); - - // 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, client_read_fd.get()); - - // &argv_c[0] is a pointer to a pointer to const char data, but because of - // how C (not C++) works, execvp() wants a pointer to a const pointer to - // char data. It modifies neither the data nor the pointers, so the - // const_cast is safe. - execvp(handler.value().c_str(), const_cast(&argv_c[0])); - PLOG(FATAL) << "execvp " << handler.value(); - } - - // Parent process. - - client_read_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. - exception_port_.reset(child_port_handshake.RunServer( - ChildPortHandshake::PortRightType::kSendRight)); - - return exception_port_.is_valid(); + return true; } bool CrashpadClient::UseHandler() { diff --git a/handler/mac/exception_handler_server.cc b/handler/mac/exception_handler_server.cc index 52da8faa..400df562 100644 --- a/handler/mac/exception_handler_server.cc +++ b/handler/mac/exception_handler_server.cc @@ -125,7 +125,7 @@ class ExceptionHandlerServerRun : public UniversalMachExcServer::Interface, bool* destroy_complex_request) override { if (exception_port != exception_port_) { LOG(WARNING) << "exception port mismatch"; - return MIG_BAD_ID; + return KERN_FAILURE; } return exception_interface_->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 64a49d56..de3171ed 100644 --- a/handler/main.cc +++ b/handler/main.cc @@ -34,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" @@ -211,21 +212,21 @@ int HandlerMain(int argc, char* argv[]) { } #if defined(OS_MACOSX) + CloseStdinAndStdout(); + if (options.reset_own_crash_exception_port_to_system_default) { CrashpadClient::UseSystemDefaultHandler(); } -#endif -#if defined(OS_MACOSX) - ExceptionHandlerServer exception_handler_server; - - CloseStdinAndStdout(); - if (!ChildPortHandshake::RunClientForFD( + base::mac::ScopedMachReceiveRight receive_right( + ChildPortHandshake::RunServerForFD( base::ScopedFD(options.handshake_fd), - exception_handler_server.receive_port(), - MACH_MSG_TYPE_MAKE_SEND)) { + ChildPortHandshake::PortRightType::kReceiveRight)); + if (!receive_right.is_valid()) { return EXIT_FAILURE; } + + ExceptionHandlerServer exception_handler_server(receive_right.Pass()); #elif defined(OS_WIN) ExceptionHandlerServer exception_handler_server(options.pipe_name); #endif // OS_MACOSX diff --git a/snapshot/win/exception_snapshot_win_test.cc b/snapshot/win/exception_snapshot_win_test.cc index fe626a38..7c3b3691 100644 --- a/snapshot/win/exception_snapshot_win_test.cc +++ b/snapshot/win/exception_snapshot_win_test.cc @@ -41,16 +41,18 @@ class RunServerThread : public Thread { public: // Instantiates a thread which will invoke server->Run(delegate, pipe_name); RunServerThread(ExceptionHandlerServer* server, - ExceptionHandlerServer::Delegate* delegate) - : server_(server), delegate_(delegate) {} + ExceptionHandlerServer::Delegate* delegate, + const std::string& pipe_name) + : server_(server), delegate_(delegate), pipe_name_(pipe_name) {} ~RunServerThread() override {} private: // Thread: - void ThreadMain() override { server_->Run(delegate_); } + void ThreadMain() override { server_->Run(delegate_, pipe_name_); } ExceptionHandlerServer* server_; ExceptionHandlerServer::Delegate* delegate_; + std::string pipe_name_; DISALLOW_COPY_AND_ASSIGN(RunServerThread); }; @@ -128,8 +130,9 @@ 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(pipe_name); - RunServerThread server_thread(&exception_handler_server, &delegate); + ExceptionHandlerServer exception_handler_server; + RunServerThread server_thread( + &exception_handler_server, &delegate, pipe_name); server_thread.Start(); ScopedStopServerAndJoinThread scoped_stop_server_and_join_thread( &exception_handler_server, &server_thread); @@ -230,8 +233,9 @@ void TestDumpWithoutCrashingChild( ScopedKernelHANDLE completed(CreateEvent(nullptr, false, false, nullptr)); SimulateDelegate delegate(server_ready.get(), completed.get()); - ExceptionHandlerServer exception_handler_server(pipe_name); - RunServerThread server_thread(&exception_handler_server, &delegate); + ExceptionHandlerServer exception_handler_server; + RunServerThread server_thread( + &exception_handler_server, &delegate, pipe_name); server_thread.Start(); ScopedStopServerAndJoinThread scoped_stop_server_and_join_thread( &exception_handler_server, &server_thread); diff --git a/util/mach/child_port_handshake.h b/util/mach/child_port_handshake.h index c1d7ab9a..8956daf7 100644 --- a/util/mach/child_port_handshake.h +++ b/util/mach/child_port_handshake.h @@ -87,7 +87,7 @@ class ChildPortHandshakeTest; //! 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); +//! base::StringPrintf("%d", server_write_fd.get()); //! //! pid_t pid = fork(); //! if (pid == 0) { @@ -97,8 +97,8 @@ class ChildPortHandshakeTest; //! // 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); -//! execlp("child", "child", server_write_fd_string.c_str(), nullptr); +//! CloseMultipleNowOrOnExec(STDERR_FILENO + 1, server_write_fd.get()); +//! execlp("./child", "child", server_write_fd_string.c_str(), nullptr); //! } //! //! // Parent @@ -122,7 +122,7 @@ class ChildPortHandshakeTest; //! //! // 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, +//! if (child_port_handshake.RunClient(receive_right.get(), //! MACH_MSG_TYPE_MOVE_RECEIVE)) { //! ignore_result(receive_right.release()); //! } 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/win/exception_handler_server.cc b/util/win/exception_handler_server.cc index 234d3149..aba3ffa0 100644 --- a/util/win/exception_handler_server.cc +++ b/util/win/exception_handler_server.cc @@ -234,9 +234,8 @@ class ClientData { ExceptionHandlerServer::Delegate::~Delegate() { } -ExceptionHandlerServer::ExceptionHandlerServer(const std::string& pipe_name) - : pipe_name_(pipe_name), - port_(CreateIoCompletionPort(INVALID_HANDLE_VALUE, nullptr, 0, 1)), +ExceptionHandlerServer::ExceptionHandlerServer() + : port_(CreateIoCompletionPort(INVALID_HANDLE_VALUE, nullptr, 0, 1)), clients_lock_(), clients_() { } @@ -244,12 +243,13 @@ ExceptionHandlerServer::ExceptionHandlerServer(const std::string& pipe_name) ExceptionHandlerServer::~ExceptionHandlerServer() { } -void ExceptionHandlerServer::Run(Delegate* delegate) { +void ExceptionHandlerServer::Run(Delegate* delegate, + const std::string& pipe_name) { 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(pipe_name_16, + SendToCrashHandlerServer(base::UTF8ToUTF16(pipe_name), reinterpret_cast(message), &response); } diff --git a/util/win/exception_handler_server.h b/util/win/exception_handler_server.h index d1243406..6928a6d9 100644 --- a/util/win/exception_handler_server.h +++ b/util/win/exception_handler_server.h @@ -61,18 +61,16 @@ class ExceptionHandlerServer { }; //! \brief Constructs the exception handling server. - //! - //! \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(); ~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. - void Run(Delegate* delegate); + //! \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); //! \brief Stops the exception-handling server. Returns immediately. The //! object must not be destroyed until Run() returns. @@ -86,7 +84,6 @@ 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 9fe593b5..0d87086b 100644 --- a/util/win/exception_handler_server_test.cc +++ b/util/win/exception_handler_server_test.cc @@ -36,18 +36,20 @@ namespace { // Runs the ExceptionHandlerServer on a background thread. class RunServerThread : public Thread { public: - // Instantiates a thread which will invoke server->Run(delegate). + // Instantiates a thread which will invoke server->Run(delegate, pipe_name). RunServerThread(ExceptionHandlerServer* server, - ExceptionHandlerServer::Delegate* delegate) - : server_(server), delegate_(delegate) {} + ExceptionHandlerServer::Delegate* delegate, + const std::string& pipe_name) + : server_(server), delegate_(delegate), pipe_name_(pipe_name) {} ~RunServerThread() override {} private: // Thread: - void ThreadMain() override { server_->Run(delegate_); } + void ThreadMain() override { server_->Run(delegate_, pipe_name_); } ExceptionHandlerServer* server_; ExceptionHandlerServer::Delegate* delegate_; + std::string pipe_name_; DISALLOW_COPY_AND_ASSIGN(RunServerThread); }; @@ -83,8 +85,8 @@ class ExceptionHandlerServerTest : public testing::Test { base::StringPrintf("%08x", GetCurrentProcessId())), server_ready_(CreateEvent(nullptr, false, false, nullptr)), delegate_(server_ready_.get()), - server_(pipe_name_), - server_thread_(&server_, &delegate_) {} + server_(), + server_thread_(&server_, &delegate_, pipe_name_) {} TestDelegate& delegate() { return delegate_; } ExceptionHandlerServer& server() { return server_; } From 06ad1945718ecc95a2c353ae5b002c126c2f61d1 Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Thu, 29 Oct 2015 18:19:37 -0400 Subject: [PATCH 5/6] win: Construct ExceptionHandlerServer() with its pipe argument (again) This re-lands 9d03d54d0ba1, which was partially un-done by an apparent bad rebase leading up to fc7d8b3a27e1. Review URL: https://codereview.chromium.org/1424213005 . --- snapshot/win/exception_snapshot_win_test.cc | 18 +++++++----------- util/win/exception_handler_server.cc | 12 ++++++------ util/win/exception_handler_server.h | 11 +++++++---- util/win/exception_handler_server_test.cc | 14 ++++++-------- 4 files changed, 26 insertions(+), 29 deletions(-) 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/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_; } From cd0e25f1ba1d17b626ab5ca00533ae84e583a107 Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Thu, 29 Oct 2015 18:31:20 -0400 Subject: [PATCH 6/6] Update all URLs to point to https://crashpad.chromium.org/ All other links to code.google.com and googlecode.com are fixed to point to their proper new homes as well. R=rsesek@chromium.org Review URL: https://codereview.chromium.org/1414243005 . --- client/crash_report_database_mac.mm | 48 ++++++++++++++----- client/prune_crash_reports.cc | 7 ++- doc/appengine/README | 2 +- doc/developing.ad | 13 ++--- doc/index.ad | 10 ++-- doc/status.ad | 15 +++--- doc/support/generate.sh | 2 +- doc/support/man_footer.ad | 4 +- package.h | 4 +- snapshot/minidump/module_snapshot_minidump.cc | 18 +++---- .../minidump/process_snapshot_minidump.cc | 24 +++++----- snapshot/win/exception_snapshot_win.cc | 2 +- snapshot/win/module_snapshot_win.cc | 2 +- snapshot/win/pe_image_reader.cc | 2 +- snapshot/win/process_snapshot_win.cc | 2 +- snapshot/win/thread_snapshot_win.cc | 3 +- test/mac/mach_multiprocess.cc | 2 +- test/scoped_temp_dir_posix.cc | 2 +- third_party/gmock/README.crashpad | 2 +- third_party/gtest/README.crashpad | 2 +- third_party/gyp/README.crashpad | 2 +- util/mac/service_management_test.mm | 4 +- util/mach/child_port_handshake.cc | 2 +- util/mach/mach_extensions_test.cc | 2 +- util/win/capture_context.asm | 6 +-- util/win/process_info.h | 2 +- 26 files changed, 101 insertions(+), 83 deletions(-) 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/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%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/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/module_snapshot_win.cc b/snapshot/win/module_snapshot_win.cc index f45bedc6..3876fb1e 100644 --- a/snapshot/win/module_snapshot_win.cc +++ b/snapshot/win/module_snapshot_win.cc @@ -161,7 +161,7 @@ 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/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_handshake.cc b/util/mach/child_port_handshake.cc index e45d6aee..ff779df2 100644 --- a/util/mach/child_port_handshake.cc +++ b/util/mach/child_port_handshake.cc @@ -97,7 +97,7 @@ mach_port_t ChildPortHandshakeServer::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()); 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/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/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_;