From 6c0d42ce9dee55eaa906865191e28df35b32910d Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Tue, 20 Oct 2015 11:03:25 -0400 Subject: [PATCH 01/10] Mach port scopers should use get() instead of type conversion operators MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In https://codereview.chromium.org/1411523006, the Mach port scopers are becoming better ScopedGenerics and are losing the type conversion operators in the process. This is needed to adapt to that change. get() is ugly, but being explicit about conversion isn’t a bad thing, and these scopers will gain functionality such as Pass() as part of the switch. As a bonus, some would-be uses of get() to check for valid port rights are becoming a more descriptive is_valid(). R=rsesek@chromium.org Review URL: https://codereview.chromium.org/1405273002 . --- client/crashpad_client_mac.cc | 10 +- client/simulate_crash_mac.cc | 2 +- handler/mac/crash_report_exception_handler.cc | 4 +- handler/mac/exception_handler_server.cc | 14 +-- handler/mac/exception_handler_server.h | 2 +- test/mac/mach_multiprocess.cc | 22 ++--- tools/mac/catch_exception_tool.cc | 2 +- tools/mac/exception_port_tool.cc | 4 +- util/mach/child_port_handshake.cc | 19 ++-- util/mach/exception_ports_test.cc | 7 +- util/mach/mach_extensions_test.cc | 8 +- util/mach/mach_message_server_test.cc | 8 +- util/mach/mach_message_test.cc | 4 +- util/mach/notify_server_test.cc | 98 ++++++++++--------- 14 files changed, 104 insertions(+), 100 deletions(-) diff --git a/client/crashpad_client_mac.cc b/client/crashpad_client_mac.cc index a180fd19..5797fba8 100644 --- a/client/crashpad_client_mac.cc +++ b/client/crashpad_client_mac.cc @@ -92,7 +92,7 @@ bool CrashpadClient::StartHandler( const std::string& url, const std::map& annotations, const std::vector& arguments) { - DCHECK_EQ(exception_port_, kMachPortNull); + 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(). @@ -211,13 +211,13 @@ bool CrashpadClient::StartHandler( // Rendezvous with the handler running in the grandchild process. exception_port_.reset(child_port_handshake.RunServer()); - return exception_port_ ? true : false; + return exception_port_.is_valid(); } bool CrashpadClient::UseHandler() { - DCHECK_NE(exception_port_, kMachPortNull); + DCHECK(exception_port_.is_valid()); - return SetCrashExceptionPorts(exception_port_); + return SetCrashExceptionPorts(exception_port_.get()); } // static @@ -227,7 +227,7 @@ void CrashpadClient::UseSystemDefaultHandler() { // Proceed even if SystemCrashReporterHandler() failed, setting MACH_PORT_NULL // to clear the current exception ports. - if (!SetCrashExceptionPorts(system_crash_reporter_handler)) { + if (!SetCrashExceptionPorts(system_crash_reporter_handler.get())) { SetCrashExceptionPorts(MACH_PORT_NULL); } } diff --git a/client/simulate_crash_mac.cc b/client/simulate_crash_mac.cc index cda6b76d..71d5d904 100644 --- a/client/simulate_crash_mac.cc +++ b/client/simulate_crash_mac.cc @@ -221,7 +221,7 @@ void SimulateCrash(const NativeCPUContext& cpu_context) { DCHECK_LE(handlers.size(), 1u); if (handlers.size() == 1) { DCHECK(handlers[0].mask & EXC_MASK_CRASH); - success = DeliverException(thread, + success = DeliverException(thread.get(), mach_task_self(), exception, codes, diff --git a/handler/mac/crash_report_exception_handler.cc b/handler/mac/crash_report_exception_handler.cc index a54cd0dd..288148e9 100644 --- a/handler/mac/crash_report_exception_handler.cc +++ b/handler/mac/crash_report_exception_handler.cc @@ -186,7 +186,7 @@ kern_return_t CrashReportExceptionHandler::CatchMachException( // vendor. base::mac::ScopedMachSendRight system_crash_reporter_handler(SystemCrashReporterHandler()); - if (system_crash_reporter_handler) { + if (system_crash_reporter_handler.get()) { // Make copies of mutable out parameters so that the system crash reporter // can’t influence the state returned by this method. thread_state_flavor_t flavor_forward = *flavor; @@ -205,7 +205,7 @@ kern_return_t CrashReportExceptionHandler::CatchMachException( // will be available. kern_return_t kr = UniversalExceptionRaise( EXCEPTION_STATE_IDENTITY | MACH_EXCEPTION_CODES, - system_crash_reporter_handler, + system_crash_reporter_handler.get(), thread, task, exception, diff --git a/handler/mac/exception_handler_server.cc b/handler/mac/exception_handler_server.cc index 39348ec4..52da8faa 100644 --- a/handler/mac/exception_handler_server.cc +++ b/handler/mac/exception_handler_server.cc @@ -41,7 +41,7 @@ class ExceptionHandlerServerRun : public UniversalMachExcServer::Interface, exception_port_(exception_port), notify_port_(NewMachPort(MACH_PORT_RIGHT_RECEIVE)), running_(true) { - CHECK_NE(notify_port_, kMachPortNull); + CHECK(notify_port_.is_valid()); composite_mach_message_server_.AddHandler(&mach_exc_server_); composite_mach_message_server_.AddHandler(¬ify_server_); @@ -61,7 +61,7 @@ class ExceptionHandlerServerRun : public UniversalMachExcServer::Interface, exception_port_, MACH_NOTIFY_NO_SENDERS, 0, - notify_port_, + notify_port_.get(), MACH_MSG_TYPE_MAKE_SEND_ONCE, &previous); MACH_CHECK(kr == KERN_SUCCESS, kr) << "mach_port_request_notification"; @@ -83,11 +83,11 @@ class ExceptionHandlerServerRun : public UniversalMachExcServer::Interface, NewMachPort(MACH_PORT_RIGHT_PORT_SET)); kr = mach_port_insert_member( - mach_task_self(), exception_port_, server_port_set); + mach_task_self(), exception_port_, server_port_set.get()); MACH_CHECK(kr == KERN_SUCCESS, kr) << "mach_port_insert_member"; kr = mach_port_insert_member( - mach_task_self(), notify_port_, server_port_set); + mach_task_self(), notify_port_.get(), server_port_set.get()); MACH_CHECK(kr == KERN_SUCCESS, kr) << "mach_port_insert_member"; // Run the server in kOneShot mode so that running_ can be reevaluated after @@ -98,7 +98,7 @@ class ExceptionHandlerServerRun : public UniversalMachExcServer::Interface, // DoMachNotifyNoSenders() as appropriate. mach_msg_return_t mr = MachMessageServer::Run(&composite_mach_message_server_, - server_port_set, + server_port_set.get(), kMachMessageReceiveAuditTrailer, MachMessageServer::kOneShot, MachMessageServer::kReceiveLargeIgnore, @@ -221,7 +221,7 @@ class ExceptionHandlerServerRun : public UniversalMachExcServer::Interface, ExceptionHandlerServer::ExceptionHandlerServer() : receive_port_(NewMachPort(MACH_PORT_RIGHT_RECEIVE)) { - CHECK_NE(receive_port_, kMachPortNull); + CHECK(receive_port_.is_valid()); } ExceptionHandlerServer::~ExceptionHandlerServer() { @@ -229,7 +229,7 @@ ExceptionHandlerServer::~ExceptionHandlerServer() { void ExceptionHandlerServer::Run( UniversalMachExcServer::Interface* exception_interface) { - ExceptionHandlerServerRun run(receive_port_, exception_interface); + ExceptionHandlerServerRun run(receive_port_.get(), exception_interface); run.Run(); } diff --git a/handler/mac/exception_handler_server.h b/handler/mac/exception_handler_server.h index 37c61a0f..83e131be 100644 --- a/handler/mac/exception_handler_server.h +++ b/handler/mac/exception_handler_server.h @@ -57,7 +57,7 @@ class ExceptionHandlerServer { //! //! 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_; } + mach_port_t receive_port() const { return receive_port_.get(); } private: base::mac::ScopedMachReceiveRight receive_port_; diff --git a/test/mac/mach_multiprocess.cc b/test/mac/mach_multiprocess.cc index 6d410d38..65ea46df 100644 --- a/test/mac/mach_multiprocess.cc +++ b/test/mac/mach_multiprocess.cc @@ -98,22 +98,22 @@ void MachMultiprocess::PreFork() { } info_->local_port = BootstrapCheckIn(info_->service_name); - ASSERT_NE(kMachPortNull, info_->local_port); + ASSERT_TRUE(info_->local_port.is_valid()); } mach_port_t MachMultiprocess::LocalPort() const { - EXPECT_NE(kMachPortNull, info_->local_port); - return info_->local_port; + EXPECT_TRUE(info_->local_port.is_valid()); + return info_->local_port.get(); } mach_port_t MachMultiprocess::RemotePort() const { - EXPECT_NE(kMachPortNull, info_->remote_port); - return info_->remote_port; + EXPECT_TRUE(info_->remote_port.is_valid()); + return info_->remote_port.get(); } task_t MachMultiprocess::ChildTask() const { - EXPECT_NE(TASK_NULL, info_->child_task); - return info_->child_task; + EXPECT_TRUE(info_->child_task.is_valid()); + return info_->child_task.get(); } void MachMultiprocess::MultiprocessParent() { @@ -123,7 +123,7 @@ void MachMultiprocess::MultiprocessParent() { MACH_RCV_MSG | kMachMessageReceiveAuditTrailer, 0, sizeof(message), - info_->local_port, + info_->local_port.get(), MACH_MSG_TIMEOUT_NONE, MACH_PORT_NULL); ASSERT_EQ(MACH_MSG_SUCCESS, kr) << MachErrorMessage(kr, "mach_msg"); @@ -198,7 +198,7 @@ void MachMultiprocess::MultiprocessParent() { // Verify that the child’s task port is what it purports to be. int mach_pid; - kr = pid_for_task(info_->child_task, &mach_pid); + kr = pid_for_task(info_->child_task.get(), &mach_pid); ASSERT_EQ(KERN_SUCCESS, kr) << MachErrorMessage(kr, "pid_for_task"); ASSERT_EQ(ChildPID(), mach_pid); @@ -229,8 +229,8 @@ void MachMultiprocess::MultiprocessChild() { MACH_MSGH_BITS(MACH_MSG_TYPE_COPY_SEND, MACH_MSG_TYPE_MAKE_SEND) | MACH_MSGH_BITS_COMPLEX; message.header.msgh_size = sizeof(message); - message.header.msgh_remote_port = info_->remote_port; - message.header.msgh_local_port = info_->local_port; + message.header.msgh_remote_port = info_->remote_port.get(); + message.header.msgh_local_port = info_->local_port.get(); message.body.msgh_descriptor_count = 1; message.port_descriptor.name = mach_task_self(); message.port_descriptor.disposition = MACH_MSG_TYPE_COPY_SEND; diff --git a/tools/mac/catch_exception_tool.cc b/tools/mac/catch_exception_tool.cc index b70ce0a5..32c6aed2 100644 --- a/tools/mac/catch_exception_tool.cc +++ b/tools/mac/catch_exception_tool.cc @@ -301,7 +301,7 @@ int CatchExceptionToolMain(int argc, char* argv[]) { } mach_msg_return_t mr = MachMessageServer::Run(&universal_mach_exc_server, - service_port, + service_port.get(), MACH_MSG_OPTION_NONE, options.persistent, receive_large, diff --git a/tools/mac/exception_port_tool.cc b/tools/mac/exception_port_tool.cc index e26c1bb0..b873e68b 100644 --- a/tools/mac/exception_port_tool.cc +++ b/tools/mac/exception_port_tool.cc @@ -195,7 +195,7 @@ void ShowBootstrapService(const std::string& service_name, return; } - mach_send_right_pool->AddSendRight(service_port); + mach_send_right_pool->AddSendRight(service_port.get()); printf("service %s %#x\n", service_name.c_str(), service_port.get()); } @@ -300,7 +300,7 @@ bool SetExceptionPort(const ExceptionHandlerDescription* description, ExceptionPorts exception_ports(description->target_type, target_port); if (!exception_ports.SetExceptionPort(description->mask, - service_port, + service_port.get(), description->behavior, description->flavor)) { return false; diff --git a/util/mach/child_port_handshake.cc b/util/mach/child_port_handshake.cc index b01af7a7..119a53c0 100644 --- a/util/mach/child_port_handshake.cc +++ b/util/mach/child_port_handshake.cc @@ -105,7 +105,7 @@ mach_port_t ChildPortHandshake::RunServer() { // Check the new service in with the bootstrap server, obtaining a receive // right for it. base::mac::ScopedMachReceiveRight server_port(BootstrapCheckIn(service_name)); - CHECK_NE(server_port, kMachPortNull); + CHECK(server_port.is_valid()); // Share the service name with the client via the pipe. uint32_t service_name_length = service_name.size(); @@ -125,10 +125,10 @@ mach_port_t ChildPortHandshake::RunServer() { // requires a port set. Create a new port set and add the receive right to it. base::mac::ScopedMachPortSet server_port_set( NewMachPort(MACH_PORT_RIGHT_PORT_SET)); - CHECK_NE(server_port_set, kMachPortNull); + CHECK(server_port_set.is_valid()); - kern_return_t kr = - mach_port_insert_member(mach_task_self(), server_port, server_port_set); + kern_return_t kr = mach_port_insert_member( + mach_task_self(), server_port.get(), server_port_set.get()); MACH_CHECK(kr == KERN_SUCCESS, kr) << "mach_port_insert_member"; // Set up a kqueue to monitor both the server’s receive right and the write @@ -140,7 +140,7 @@ mach_port_t ChildPortHandshake::RunServer() { struct kevent changelist[2]; EV_SET(&changelist[0], - server_port_set, + server_port_set.get(), EVFILT_MACHPORT, EV_ADD | EV_CLEAR, 0, @@ -194,7 +194,7 @@ mach_port_t ChildPortHandshake::RunServer() { switch (event.filter) { case EVFILT_MACHPORT: { // There’s something to receive on the port set. - DCHECK_EQ(event.ident, server_port_set); + DCHECK_EQ(event.ident, server_port_set.get()); // Run the message server in an inner loop instead of using // MachMessageServer::kPersistent. This allows the loop to exit as soon @@ -207,7 +207,7 @@ mach_port_t ChildPortHandshake::RunServer() { // this will call HandleChildPortCheckIn(). mach_msg_return_t mr = MachMessageServer::Run(&child_port_server, - server_port_set, + server_port_set.get(), MACH_MSG_OPTION_NONE, MachMessageServer::kOneShot, MachMessageServer::kReceiveLargeIgnore, @@ -329,10 +329,11 @@ 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_NE(server_port, kMachPortNull); + CHECK(server_port.is_valid()); // Check in with the server. - kern_return_t kr = child_port_check_in(server_port, token, port, right_type); + 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"; } diff --git a/util/mach/exception_ports_test.cc b/util/mach/exception_ports_test.cc index 7384c022..e6e1d56d 100644 --- a/util/mach/exception_ports_test.cc +++ b/util/mach/exception_ports_test.cc @@ -376,9 +376,9 @@ class TestExceptionPorts : public MachMultiprocess, threads_need_owners.Disarm(); ExceptionPorts main_thread_ports(ExceptionPorts::kTargetTypeThread, - main_thread); + main_thread.get()); ExceptionPorts other_thread_ports(ExceptionPorts::kTargetTypeThread, - other_thread); + other_thread.get()); EXPECT_STREQ("thread", main_thread_ports.TargetTypeName()); EXPECT_STREQ("thread", other_thread_ports.TargetTypeName()); @@ -586,7 +586,8 @@ TEST(ExceptionPorts, HostExceptionPorts) { const bool expect_success = geteuid() == 0; base::mac::ScopedMachSendRight host(mach_host_self()); - ExceptionPorts explicit_host_ports(ExceptionPorts::kTargetTypeHost, host); + ExceptionPorts explicit_host_ports(ExceptionPorts::kTargetTypeHost, + host.get()); EXPECT_STREQ("host", explicit_host_ports.TargetTypeName()); ExceptionPorts::ExceptionHandlerVector explicit_handlers; diff --git a/util/mach/mach_extensions_test.cc b/util/mach/mach_extensions_test.cc index 2b987b37..04af5dfe 100644 --- a/util/mach/mach_extensions_test.cc +++ b/util/mach/mach_extensions_test.cc @@ -34,7 +34,7 @@ TEST(MachExtensions, NewMachPort_Receive) { ASSERT_NE(kMachPortNull, port); mach_port_type_t type; - kern_return_t kr = mach_port_type(mach_task_self(), port, &type); + kern_return_t kr = mach_port_type(mach_task_self(), port.get(), &type); ASSERT_EQ(KERN_SUCCESS, kr) << MachErrorMessage(kr, "mach_port_get_type"); EXPECT_EQ(MACH_PORT_TYPE_RECEIVE, type); @@ -45,7 +45,7 @@ TEST(MachExtensions, NewMachPort_PortSet) { ASSERT_NE(kMachPortNull, port); mach_port_type_t type; - kern_return_t kr = mach_port_type(mach_task_self(), port, &type); + kern_return_t kr = mach_port_type(mach_task_self(), port.get(), &type); ASSERT_EQ(KERN_SUCCESS, kr) << MachErrorMessage(kr, "mach_port_get_type"); EXPECT_EQ(MACH_PORT_TYPE_PORT_SET, type); @@ -56,7 +56,7 @@ TEST(MachExtensions, NewMachPort_DeadName) { ASSERT_NE(kMachPortNull, port); mach_port_type_t type; - kern_return_t kr = mach_port_type(mach_task_self(), port, &type); + kern_return_t kr = mach_port_type(mach_task_self(), port.get(), &type); ASSERT_EQ(KERN_SUCCESS, kr) << MachErrorMessage(kr, "mach_port_get_type"); EXPECT_EQ(MACH_PORT_TYPE_DEAD_NAME, type); @@ -173,7 +173,7 @@ TEST(MachExtensions, BootstrapCheckInAndLookUp) { TEST(MachExtensions, SystemCrashReporterHandler) { base::mac::ScopedMachSendRight system_crash_reporter_handler(SystemCrashReporterHandler()); - EXPECT_TRUE(system_crash_reporter_handler); + EXPECT_TRUE(system_crash_reporter_handler.is_valid()); } } // namespace diff --git a/util/mach/mach_message_server_test.cc b/util/mach/mach_message_server_test.cc index 6878cf95..daa64a93 100644 --- a/util/mach/mach_message_server_test.cc +++ b/util/mach/mach_message_server_test.cc @@ -467,8 +467,8 @@ class TestMachMessageServer : public MachMessageServer::Interface, // carried in the request message to the server. By the time the server // looks at the right, it will have become a dead name. local_receive_port_owner.reset(NewMachPort(MACH_PORT_RIGHT_RECEIVE)); - ASSERT_NE(kMachPortNull, local_receive_port_owner); - request.header.msgh_local_port = local_receive_port_owner; + ASSERT_TRUE(local_receive_port_owner.is_valid()); + request.header.msgh_local_port = local_receive_port_owner.get(); break; } } @@ -479,8 +479,8 @@ class TestMachMessageServer : public MachMessageServer::Interface, // properly handles ownership of resources received in complex messages. request.body.msgh_descriptor_count = 1; child_complex_message_port_.reset(NewMachPort(MACH_PORT_RIGHT_RECEIVE)); - ASSERT_NE(kMachPortNull, child_complex_message_port_); - request.port_descriptor.name = child_complex_message_port_; + ASSERT_TRUE(child_complex_message_port_.is_valid()); + request.port_descriptor.name = child_complex_message_port_.get(); request.port_descriptor.disposition = MACH_MSG_TYPE_MAKE_SEND; request.port_descriptor.type = MACH_MSG_PORT_DESCRIPTOR; } else { diff --git a/util/mach/mach_message_test.cc b/util/mach/mach_message_test.cc index a3fca519..5e79b212 100644 --- a/util/mach/mach_message_test.cc +++ b/util/mach/mach_message_test.cc @@ -115,7 +115,7 @@ TEST(MachMessage, AuditPIDFromMachMessageTrailer) { mach_msg_empty_send_t send = {}; send.header.msgh_bits = MACH_MSGH_BITS(MACH_MSG_TYPE_MAKE_SEND_ONCE, 0); send.header.msgh_size = sizeof(send); - send.header.msgh_remote_port = port; + send.header.msgh_remote_port = port.get(); mach_msg_return_t mr = MachMessageWithDeadline(&send.header, MACH_SEND_MSG, @@ -138,7 +138,7 @@ TEST(MachMessage, AuditPIDFromMachMessageTrailer) { mr = MachMessageWithDeadline(&receive.header, MACH_RCV_MSG | kMachMessageReceiveAuditTrailer, sizeof(receive), - port, + port.get(), kMachMessageDeadlineNonblocking, MACH_PORT_NULL, false); diff --git a/util/mach/notify_server_test.cc b/util/mach/notify_server_test.cc index a852bd0e..445e35bf 100644 --- a/util/mach/notify_server_test.cc +++ b/util/mach/notify_server_test.cc @@ -256,12 +256,12 @@ class NotifyServerTestBase : public testing::Test, //! established for the current test. On failure, returns `MACH_PORT_NULL` //! with a gtest failure added. mach_port_t ServerPort() { - if (!server_port_) { + if (!server_port_.is_valid()) { server_port_.reset(NewMachPort(MACH_PORT_RIGHT_RECEIVE)); - EXPECT_NE(kMachPortNull, server_port_); + EXPECT_TRUE(server_port_.is_valid()); } - return server_port_; + return server_port_.get(); } // testing::Test: @@ -309,14 +309,14 @@ TEST_F(NotifyServerTest, NoNotification) { TEST_F(NotifyServerTest, MachNotifyPortDeleted) { base::mac::ScopedMachReceiveRight receive_right( NewMachPort(MACH_PORT_RIGHT_RECEIVE)); - ASSERT_NE(kMachPortNull, receive_right); + ASSERT_TRUE(receive_right.is_valid()); base::mac::ScopedMachSendRight send_once_right( - SendOnceRightFromReceiveRight(receive_right)); - ASSERT_NE(kMachPortNull, send_once_right); + SendOnceRightFromReceiveRight(receive_right.get())); + ASSERT_TRUE(send_once_right.is_valid()); - ASSERT_TRUE( - RequestMachPortNotification(send_once_right, MACH_NOTIFY_DEAD_NAME, 0)); + ASSERT_TRUE(RequestMachPortNotification( + send_once_right.get(), MACH_NOTIFY_DEAD_NAME, 0)); EXPECT_CALL( *this, @@ -336,10 +336,10 @@ TEST_F(NotifyServerTest, MachNotifyPortDeleted) { TEST_F(NotifyServerTest, MachNotifyPortDestroyed) { base::mac::ScopedMachReceiveRight receive_right( NewMachPort(MACH_PORT_RIGHT_RECEIVE)); - ASSERT_NE(kMachPortNull, receive_right); + ASSERT_TRUE(receive_right.is_valid()); ASSERT_TRUE(RequestMachPortNotification( - receive_right, MACH_NOTIFY_PORT_DESTROYED, 0)); + receive_right.get(), MACH_NOTIFY_PORT_DESTROYED, 0)); EXPECT_CALL( *this, @@ -360,10 +360,10 @@ TEST_F(NotifyServerTest, MachNotifyPortDestroyed) { TEST_F(NotifyServerTest, MachNotifyPortDestroyed_NoNotification) { base::mac::ScopedMachReceiveRight receive_right( NewMachPort(MACH_PORT_RIGHT_RECEIVE)); - ASSERT_NE(kMachPortNull, receive_right); + ASSERT_TRUE(receive_right.is_valid()); ASSERT_TRUE(RequestMachPortNotification( - receive_right, MACH_NOTIFY_PORT_DESTROYED, 0)); + receive_right.get(), MACH_NOTIFY_PORT_DESTROYED, 0)); RunServer(); } @@ -373,10 +373,10 @@ TEST_F(NotifyServerTest, MachNotifyPortDestroyed_NoNotification) { TEST_F(NotifyServerTest, MachNotifyNoSenders_NoSendRight) { base::mac::ScopedMachReceiveRight receive_right( NewMachPort(MACH_PORT_RIGHT_RECEIVE)); - ASSERT_NE(kMachPortNull, receive_right); + ASSERT_TRUE(receive_right.is_valid()); - ASSERT_TRUE( - RequestMachPortNotification(receive_right, MACH_NOTIFY_NO_SENDERS, 0)); + ASSERT_TRUE(RequestMachPortNotification( + receive_right.get(), MACH_NOTIFY_NO_SENDERS, 0)); EXPECT_CALL(*this, DoMachNotifyNoSenders( @@ -393,14 +393,14 @@ TEST_F(NotifyServerTest, MachNotifyNoSenders_NoSendRight) { TEST_F(NotifyServerTest, MachNotifyNoSenders_SendRightDeallocated) { base::mac::ScopedMachReceiveRight receive_right( NewMachPort(MACH_PORT_RIGHT_RECEIVE)); - ASSERT_NE(kMachPortNull, receive_right); + ASSERT_TRUE(receive_right.is_valid()); base::mac::ScopedMachSendRight send_right( - SendRightFromReceiveRight(receive_right)); - ASSERT_NE(kMachPortNull, send_right); + SendRightFromReceiveRight(receive_right.get())); + ASSERT_TRUE(send_right.is_valid()); - ASSERT_TRUE( - RequestMachPortNotification(receive_right, MACH_NOTIFY_NO_SENDERS, 1)); + ASSERT_TRUE(RequestMachPortNotification( + receive_right.get(), MACH_NOTIFY_NO_SENDERS, 1)); EXPECT_CALL(*this, DoMachNotifyNoSenders( @@ -418,25 +418,25 @@ TEST_F(NotifyServerTest, MachNotifyNoSenders_SendRightDeallocated) { TEST_F(NotifyServerTest, MachNotifyNoSenders_NoNotification) { base::mac::ScopedMachReceiveRight receive_right( NewMachPort(MACH_PORT_RIGHT_RECEIVE)); - ASSERT_NE(kMachPortNull, receive_right); + ASSERT_TRUE(receive_right.is_valid()); base::mac::ScopedMachSendRight send_right_0( - SendRightFromReceiveRight(receive_right)); - ASSERT_NE(kMachPortNull, send_right_0); + SendRightFromReceiveRight(receive_right.get())); + ASSERT_TRUE(send_right_0.is_valid()); base::mac::ScopedMachSendRight send_right_1( - SendRightFromReceiveRight(receive_right)); - ASSERT_NE(kMachPortNull, send_right_1); + SendRightFromReceiveRight(receive_right.get())); + ASSERT_TRUE(send_right_1.is_valid()); - ASSERT_TRUE( - RequestMachPortNotification(receive_right, MACH_NOTIFY_NO_SENDERS, 1)); + ASSERT_TRUE(RequestMachPortNotification( + receive_right.get(), MACH_NOTIFY_NO_SENDERS, 1)); send_right_1.reset(); RunServer(); - EXPECT_EQ(1u, RightRefCount(receive_right, MACH_PORT_RIGHT_RECEIVE)); - EXPECT_EQ(1u, RightRefCount(receive_right, MACH_PORT_RIGHT_SEND)); + EXPECT_EQ(1u, RightRefCount(receive_right.get(), MACH_PORT_RIGHT_RECEIVE)); + EXPECT_EQ(1u, RightRefCount(receive_right.get(), MACH_PORT_RIGHT_SEND)); } // When a send-once right is deallocated without being used, a send-once @@ -444,7 +444,7 @@ TEST_F(NotifyServerTest, MachNotifyNoSenders_NoNotification) { TEST_F(NotifyServerTest, MachNotifySendOnce_ExplicitDeallocation) { base::mac::ScopedMachSendRight send_once_right( SendOnceRightFromReceiveRight(ServerPort())); - ASSERT_NE(kMachPortNull, send_once_right); + ASSERT_TRUE(send_once_right.is_valid()); EXPECT_CALL(*this, DoMachNotifySendOnce(ServerPort(), @@ -463,13 +463,13 @@ TEST_F(NotifyServerTest, MachNotifySendOnce_ExplicitDeallocation) { TEST_F(NotifyServerTest, MachNotifySendOnce_ImplicitDeallocation) { base::mac::ScopedMachReceiveRight receive_right( NewMachPort(MACH_PORT_RIGHT_RECEIVE)); - ASSERT_NE(kMachPortNull, receive_right); + ASSERT_TRUE(receive_right.is_valid()); mach_msg_empty_send_t message = {}; message.header.msgh_bits = MACH_MSGH_BITS(MACH_MSG_TYPE_MAKE_SEND, MACH_MSG_TYPE_MAKE_SEND_ONCE); message.header.msgh_size = sizeof(message); - message.header.msgh_remote_port = receive_right; + message.header.msgh_remote_port = receive_right.get(); message.header.msgh_local_port = ServerPort(); mach_msg_return_t mr = mach_msg(&message.header, MACH_SEND_MSG | MACH_SEND_TIMEOUT, @@ -497,14 +497,14 @@ TEST_F(NotifyServerTest, MachNotifySendOnce_ImplicitDeallocation) { TEST_F(NotifyServerTest, MachNotifyDeadName) { base::mac::ScopedMachReceiveRight receive_right( NewMachPort(MACH_PORT_RIGHT_RECEIVE)); - ASSERT_NE(kMachPortNull, receive_right); + ASSERT_TRUE(receive_right.is_valid()); base::mac::ScopedMachSendRight send_once_right( - SendOnceRightFromReceiveRight(receive_right)); - ASSERT_NE(kMachPortNull, send_once_right); + SendOnceRightFromReceiveRight(receive_right.get())); + ASSERT_TRUE(send_once_right.is_valid()); - ASSERT_TRUE( - RequestMachPortNotification(send_once_right, MACH_NOTIFY_DEAD_NAME, 0)); + ASSERT_TRUE(RequestMachPortNotification( + send_once_right.get(), MACH_NOTIFY_DEAD_NAME, 0)); // send_once_right becomes a dead name with the send-once right’s original // user reference count of 1, but the dead-name notification increments the @@ -523,10 +523,11 @@ TEST_F(NotifyServerTest, MachNotifyDeadName) { RunServer(); - EXPECT_TRUE(IsRight(send_once_right, MACH_PORT_TYPE_DEAD_NAME)); + EXPECT_TRUE(IsRight(send_once_right.get(), MACH_PORT_TYPE_DEAD_NAME)); - EXPECT_EQ(0u, RightRefCount(send_once_right, MACH_PORT_RIGHT_SEND_ONCE)); - EXPECT_EQ(1u, DeadNameRightRefCount(send_once_right)); + EXPECT_EQ(0u, + RightRefCount(send_once_right.get(), MACH_PORT_RIGHT_SEND_ONCE)); + EXPECT_EQ(1u, DeadNameRightRefCount(send_once_right.get())); } // When the receive right corresponding to a send-once right with a dead-name @@ -535,21 +536,22 @@ TEST_F(NotifyServerTest, MachNotifyDeadName) { TEST_F(NotifyServerTest, MachNotifyDeadName_NoNotification) { base::mac::ScopedMachReceiveRight receive_right( NewMachPort(MACH_PORT_RIGHT_RECEIVE)); - ASSERT_NE(kMachPortNull, receive_right); + ASSERT_TRUE(receive_right.is_valid()); base::mac::ScopedMachSendRight send_once_right( - SendOnceRightFromReceiveRight(receive_right)); - ASSERT_NE(kMachPortNull, send_once_right); + SendOnceRightFromReceiveRight(receive_right.get())); + ASSERT_TRUE(send_once_right.is_valid()); - ASSERT_TRUE( - RequestMachPortNotification(send_once_right, MACH_NOTIFY_DEAD_NAME, 0)); + ASSERT_TRUE(RequestMachPortNotification( + send_once_right.get(), MACH_NOTIFY_DEAD_NAME, 0)); RunServer(); - EXPECT_FALSE(IsRight(send_once_right, MACH_PORT_TYPE_DEAD_NAME)); + EXPECT_FALSE(IsRight(send_once_right.get(), MACH_PORT_TYPE_DEAD_NAME)); - EXPECT_EQ(1u, RightRefCount(send_once_right, MACH_PORT_RIGHT_SEND_ONCE)); - EXPECT_EQ(0u, DeadNameRightRefCount(send_once_right)); + EXPECT_EQ(1u, + RightRefCount(send_once_right.get(), MACH_PORT_RIGHT_SEND_ONCE)); + EXPECT_EQ(0u, DeadNameRightRefCount(send_once_right.get())); } } // namespace From 0ed0106aa4f2b146353275e24de75cdef4fb9f6c Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Wed, 21 Oct 2015 09:42:29 -0400 Subject: [PATCH 02/10] Add /bug redirects to the home page AppEngine app MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit /bug/ redirects to the Crashpad project on Monorail. /bug/new redirects to the “new issue” screen, and /bug/123 redirects to the named bug. R=andybons@chromium.org Review URL: https://codereview.chromium.org/1415063002 . --- doc/appengine/main.go | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/doc/appengine/main.go b/doc/appengine/main.go index 261440d4..e05870f5 100644 --- a/doc/appengine/main.go +++ b/doc/appengine/main.go @@ -31,13 +31,16 @@ import ( "google.golang.org/appengine/urlfetch" ) -const baseURL = "https://chromium.googlesource.com/crashpad/crashpad/+/doc/doc/generated/?format=TEXT" - func init() { http.HandleFunc("/", handler) } func handler(w http.ResponseWriter, r *http.Request) { + const ( + baseURL = "https://chromium.googlesource.com/crashpad/crashpad/+/doc/doc/generated/?format=TEXT" + bugBaseURL = "https://bugs.chromium.org/p/crashpad/" + ) + ctx := appengine.NewContext(r) client := urlfetch.Client(ctx) @@ -47,6 +50,17 @@ func handler(w http.ResponseWriter, r *http.Request) { return } + if r.URL.Path == "/bug" || r.URL.Path == "/bug/" { + http.Redirect(w, r, bugBaseURL, http.StatusFound) + return + } else if r.URL.Path == "/bug/new" { + http.Redirect(w, r, bugBaseURL+"issues/entry", http.StatusFound) + return + } else if strings.HasPrefix(r.URL.Path, "/bug/") { + http.Redirect(w, r, bugBaseURL+"issues/detail?id="+r.URL.Path[5:], http.StatusFound) + return + } + u, err := url.Parse(baseURL) if err != nil { http.Error(w, err.Error(), http.StatusInternalServerError) From af8c7fcbee283ba99ead1676feb442ea7ec83b21 Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Wed, 21 Oct 2015 11:20:10 -0400 Subject: [PATCH 03/10] Add a README for the App Engine app with notes for maintaining it R=andybons@chromium.org Review URL: https://codereview.chromium.org/1416833003 . --- doc/appengine/README | 39 +++++++++++++++++++++++++++++++++++++++ doc/appengine/app.yaml | 1 - 2 files changed, 39 insertions(+), 1 deletion(-) create mode 100644 doc/appengine/README diff --git a/doc/appengine/README b/doc/appengine/README new file mode 100644 index 00000000..d3f4d227 --- /dev/null +++ b/doc/appengine/README @@ -0,0 +1,39 @@ +This is the App Engine app that serves https://crashpad-home.appspot.com/. + +To work on this app, obtain the App Engine SDK for Go from +https://cloud.google.com/appengine/downloads. Unpacking it produces a +go_appengine directory. This may be added to your $PATH for convenience, +although it is not necessary. + +The commands in this README are expected to be run from the directory containing +app.yaml. + +The App Engine SDK for Go provides App Engine packages at the “appengine” import +path, but not the newer “google.golang.org/appengine” path. The Crashpad app +uses the newer paths. See +https://github.com/golang/appengine#2-update-import-paths and +https://code.google.com/p/googleappengine/issues/detail?id=11670. To make these +available, obtain a Go release from https://golang.org/dl/, and run: + +$ GOROOT=…/go_appengine/goroot GOPATH=…/go_appengine/gopath go get -d + +To test locally: + +$ goapp serve + +Look for the “Starting module "default" running at: http://localhost:8080” line, +which tells you the URL of the local running instance of the app. + +To deploy: + +$ version=$(git rev-parse --short=12 HEAD) +$ [[ -n "$(git status --porcelain)" ]] && version+=-dirty +$ goapp deploy -version "${version}" + +Note that app.yaml does not name a “version” to encourage you to use a git hash +as the version, as above. + +Activate a newly-deployed version by visiting the App Engine console at +https://appengine.google.com/deployment?&app_id=s~crashpad-home, selecting it, +and choosing “Make Default”. It is also possible to delete old versions from +this page when they are no longer needed. diff --git a/doc/appengine/app.yaml b/doc/appengine/app.yaml index bcf370c8..3d3fbdff 100644 --- a/doc/appengine/app.yaml +++ b/doc/appengine/app.yaml @@ -1,5 +1,4 @@ application: crashpad-home -version: 1 runtime: go api_version: go1 From 3ac40a54d0e8e7bbccfcf88cabd67b9659032865 Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Wed, 21 Oct 2015 13:22:51 -0400 Subject: [PATCH 04/10] doc: Add generate_git.sh, which updates the checked-in generated docs BUG=crashpad:67 R=agable@chromium.org Review URL: https://codereview.chromium.org/1399623002 . --- doc/support/generate_git.sh | 85 +++++++++++++++++++++++++++++++++++++ 1 file changed, 85 insertions(+) create mode 100755 doc/support/generate_git.sh diff --git a/doc/support/generate_git.sh b/doc/support/generate_git.sh new file mode 100755 index 00000000..209c11b9 --- /dev/null +++ b/doc/support/generate_git.sh @@ -0,0 +1,85 @@ +#!/bin/bash + +# Copyright 2015 The Crashpad Authors. All rights reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +set -e + +# Run from the Crashpad project root directory. +cd "$(dirname "${0}")/../.." + +source doc/support/compat.sh + +basename="$(basename "${0}")" + +status="$(git status --porcelain)" +if [[ -n "${status}" ]]; then + echo "${basename}: the working directory must be clean" >& 2 + git status + exit 1 +fi + +# git symbolic-ref gives the current branch name, but fails for detached HEAD. +# In that case, git rev-parse will give the current hash. +original_branch="$(git symbolic-ref --short --quiet HEAD || git rev-parse HEAD)" + +local_branch="\ +$(${sed_ext} -e 's/(.*)\..*/\1/' <<< "${basename}").${$}.${RANDOM}" + +remote_name=origin +remote_master_branch_name=master +remote_master_branch="${remote_name}/${remote_master_branch_name}" +remote_doc_branch_name=doc +remote_doc_branch="${remote_name}/${remote_doc_branch_name}" + +git fetch + +git checkout -b "${local_branch}" "${remote_doc_branch}" + +dirty= + +function cleanup() { + if [[ "${dirty}" ]]; then + git reset --hard HEAD + git clean --force + fi + + git checkout "${original_branch}" + git branch -D "${local_branch}" +} + +trap cleanup EXIT + +master_hash=$(git rev-parse --short=12 "${remote_master_branch}") +git merge "${remote_master_branch}" \ + -m "Merge ${remote_master_branch_name} ${master_hash} into doc" + +dirty=y + +doc/support/generate.sh + +git add -A doc/generated + +count="$(git diff --staged --numstat | wc -l)" +if [[ $count -gt 0 ]]; then + git commit \ + -m "Update documentation to ${remote_master_branch_name} ${master_hash}" + dirty= + + git push "${remote_name}" "HEAD:${remote_doc_branch_name}" +else + dirty= +fi + +# cleanup will run on exit From 3261edd997a4309123951dc6f9be60a9066bec24 Mon Sep 17 00:00:00 2001 From: Scott Graham Date: Wed, 21 Oct 2015 10:43:42 -0700 Subject: [PATCH 05/10] Write MINIDUMP_HANDLE_DATA_STREAM to minidump R=mark@chromium.org BUG=crashpad:21, crashpad:52 Review URL: https://codereview.chromium.org/1419623003 . --- compat/non_win/dbghelp.h | 3 + minidump/minidump.gyp | 2 + minidump/minidump_extensions.h | 5 + minidump/minidump_file_writer.cc | 8 ++ minidump/minidump_handle_writer.cc | 115 +++++++++++++++++ minidump/minidump_handle_writer.h | 75 +++++++++++ minidump/minidump_handle_writer_test.cc | 127 +++++++++++++++++++ minidump/minidump_memory_info_writer_test.cc | 2 +- minidump/minidump_string_writer.h | 7 + minidump/minidump_string_writer_test.cc | 17 +++ minidump/minidump_test.gyp | 1 + minidump/test/minidump_writable_test_util.cc | 16 +++ minidump/test/minidump_writable_test_util.h | 6 + snapshot/win/end_to_end_test.py | 10 +- 14 files changed, 391 insertions(+), 3 deletions(-) create mode 100644 minidump/minidump_handle_writer.cc create mode 100644 minidump/minidump_handle_writer.h create mode 100644 minidump/minidump_handle_writer_test.cc diff --git a/compat/non_win/dbghelp.h b/compat/non_win/dbghelp.h index baf088ba..08f07ae8 100644 --- a/compat/non_win/dbghelp.h +++ b/compat/non_win/dbghelp.h @@ -157,6 +157,9 @@ enum MINIDUMP_STREAM_TYPE { //! \brief The stream type for MINIDUMP_SYSTEM_INFO. SystemInfoStream = 7, + //! \brief The stream contains information about active `HANDLE`s. + HandleDataStream = 12, + //! \brief The stream type for MINIDUMP_MISC_INFO, MINIDUMP_MISC_INFO_2, //! MINIDUMP_MISC_INFO_3, and MINIDUMP_MISC_INFO_4. //! diff --git a/minidump/minidump.gyp b/minidump/minidump.gyp index 21041a9e..0e7fb0ad 100644 --- a/minidump/minidump.gyp +++ b/minidump/minidump.gyp @@ -44,6 +44,8 @@ 'minidump_extensions.h', 'minidump_file_writer.cc', 'minidump_file_writer.h', + 'minidump_handle_writer.cc', + 'minidump_handle_writer.h', 'minidump_memory_info_writer.cc', 'minidump_memory_info_writer.h', 'minidump_memory_writer.cc', diff --git a/minidump/minidump_extensions.h b/minidump/minidump_extensions.h index 3cb39dc2..5e43055a 100644 --- a/minidump/minidump_extensions.h +++ b/minidump/minidump_extensions.h @@ -71,6 +71,11 @@ enum MinidumpStreamType : uint32_t { //! \sa SystemInfoStream kMinidumpStreamTypeSystemInfo = SystemInfoStream, + //! \brief The stream type for MINIDUMP_HANDLE_DATA_STREAM. + //! + //! \sa HandleDataStream + kMinidumpStreamTypeHandleData = HandleDataStream, + //! \brief The stream type for MINIDUMP_MISC_INFO, MINIDUMP_MISC_INFO_2, //! MINIDUMP_MISC_INFO_3, and MINIDUMP_MISC_INFO_4. //! diff --git a/minidump/minidump_file_writer.cc b/minidump/minidump_file_writer.cc index 88986561..ff353191 100644 --- a/minidump/minidump_file_writer.cc +++ b/minidump/minidump_file_writer.cc @@ -17,6 +17,7 @@ #include "base/logging.h" #include "minidump/minidump_crashpad_info_writer.h" #include "minidump/minidump_exception_writer.h" +#include "minidump/minidump_handle_writer.h" #include "minidump/minidump_memory_info_writer.h" #include "minidump/minidump_memory_writer.h" #include "minidump/minidump_misc_info_writer.h" @@ -108,6 +109,13 @@ void MinidumpFileWriter::InitializeFromSnapshot( AddStream(memory_info_list.Pass()); } + std::vector handles_snapshot = process_snapshot->Handles(); + if (!handles_snapshot.empty()) { + auto handle_data_writer = make_scoped_ptr(new MinidumpHandleDataWriter()); + handle_data_writer->InitializeFromSnapshot(handles_snapshot); + AddStream(handle_data_writer.Pass()); + } + memory_list->AddFromSnapshot(process_snapshot->ExtraMemory()); AddStream(memory_list.Pass()); diff --git a/minidump/minidump_handle_writer.cc b/minidump/minidump_handle_writer.cc new file mode 100644 index 00000000..235d71b0 --- /dev/null +++ b/minidump/minidump_handle_writer.cc @@ -0,0 +1,115 @@ +// Copyright 2015 The Crashpad Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include "minidump/minidump_handle_writer.h" + +#include + +#include "base/logging.h" +#include "minidump/minidump_extensions.h" +#include "util/file/file_writer.h" + +namespace crashpad { + +MinidumpHandleDataWriter::MinidumpHandleDataWriter() + : handle_data_stream_base_(), handle_descriptors_(), strings_() { +} + +MinidumpHandleDataWriter::~MinidumpHandleDataWriter() { +} + +void MinidumpHandleDataWriter::InitializeFromSnapshot( + const std::vector& handle_snapshots) { + DCHECK_EQ(state(), kStateMutable); + + DCHECK(handle_descriptors_.empty()); + // Because we RegisterRVA() on the string writer below, we preallocate and + // never resize the handle_descriptors_ vector. + handle_descriptors_.resize(handle_snapshots.size()); + strings_.reserve(handle_snapshots.size()); + for (size_t i = 0; i < handle_snapshots.size(); ++i) { + const HandleSnapshot& handle_snapshot = handle_snapshots[i]; + MINIDUMP_HANDLE_DESCRIPTOR& descriptor = handle_descriptors_[i]; + + descriptor.Handle = handle_snapshot.handle; + + if (handle_snapshot.type_name.empty()) { + descriptor.TypeNameRva = 0; + } else { + // TODO(scottmg): There is often a number of repeated type names here, the + // strings ought to be pooled. + strings_.push_back(new internal::MinidumpUTF16StringWriter()); + strings_.back()->SetUTF16(handle_snapshot.type_name); + strings_.back()->RegisterRVA(&descriptor.TypeNameRva); + } + + descriptor.ObjectNameRva = 0; + descriptor.Attributes = handle_snapshot.attributes; + descriptor.GrantedAccess = handle_snapshot.granted_access; + descriptor.HandleCount = handle_snapshot.handle_count; + descriptor.PointerCount = handle_snapshot.pointer_count; + } +} + +bool MinidumpHandleDataWriter::Freeze() { + DCHECK_EQ(state(), kStateMutable); + + if (!MinidumpStreamWriter::Freeze()) + return false; + + handle_data_stream_base_.SizeOfHeader = sizeof(handle_data_stream_base_); + handle_data_stream_base_.SizeOfDescriptor = sizeof(handle_descriptors_[0]); + handle_data_stream_base_.NumberOfDescriptors = handle_descriptors_.size(); + handle_data_stream_base_.Reserved = 0; + + return true; +} + +size_t MinidumpHandleDataWriter::SizeOfObject() { + DCHECK_GE(state(), kStateFrozen); + return sizeof(handle_data_stream_base_) + + sizeof(handle_descriptors_[0]) * handle_descriptors_.size(); +} + +std::vector MinidumpHandleDataWriter::Children() { + DCHECK_GE(state(), kStateFrozen); + + std::vector children; + for (auto* string : strings_) + children.push_back(string); + return children; +} + +bool MinidumpHandleDataWriter::WriteObject(FileWriterInterface* file_writer) { + DCHECK_EQ(state(), kStateWritable); + + WritableIoVec iov; + iov.iov_base = &handle_data_stream_base_; + iov.iov_len = sizeof(handle_data_stream_base_); + std::vector iovecs(1, iov); + + for (const auto& descriptor : handle_descriptors_) { + iov.iov_base = &descriptor; + iov.iov_len = sizeof(descriptor); + iovecs.push_back(iov); + } + + return file_writer->WriteIoVec(&iovecs); +} + +MinidumpStreamType MinidumpHandleDataWriter::StreamType() const { + return kMinidumpStreamTypeHandleData; +} + +} // namespace crashpad diff --git a/minidump/minidump_handle_writer.h b/minidump/minidump_handle_writer.h new file mode 100644 index 00000000..daf1fbcd --- /dev/null +++ b/minidump/minidump_handle_writer.h @@ -0,0 +1,75 @@ +// Copyright 2015 The Crashpad Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#ifndef CRASHPAD_MINIDUMP_MINIDUMP_HANDLE_WRITER_H_ +#define CRASHPAD_MINIDUMP_MINIDUMP_HANDLE_WRITER_H_ + +#include +#include + +#include + +#include "minidump/minidump_stream_writer.h" +#include "minidump/minidump_string_writer.h" +#include "minidump/minidump_writable.h" +#include "snapshot/handle_snapshot.h" +#include "util/stdlib/pointer_container.h" + +namespace crashpad { + +//! \brief The writer for a MINIDUMP_HANDLE_DATA_STREAM stream in a minidump +//! and its contained MINIDUMP_HANDLE_DESCRIPTOR s. +//! +//! As we currently do not track any data beyond what MINIDUMP_HANDLE_DESCRIPTOR +//! supports, we only write that type of record rather than the newer +//! MINIDUMP_HANDLE_DESCRIPTOR_2. +//! +//! Note that this writer writes both the header (MINIDUMP_HANDLE_DATA_STREAM) +//! and the list of objects (MINIDUMP_HANDLE_DESCRIPTOR), which is different +//! from some of the other list writers. +class MinidumpHandleDataWriter final : public internal::MinidumpStreamWriter { + public: + MinidumpHandleDataWriter(); + ~MinidumpHandleDataWriter() override; + + //! \brief Adds a MINIDUMP_HANDLE_DESCRIPTOR for each handle in \a + //! handle_snapshot to the MINIDUMP_HANDLE_DATA_STREAM. + //! + //! \param[in] handle_snapshots The handle snapshots to use as source data. + //! + //! \note Valid in #kStateMutable. + void InitializeFromSnapshot( + const std::vector& handle_snapshots); + + protected: + // MinidumpWritable: + bool Freeze() override; + size_t SizeOfObject() override; + std::vector Children() override; + bool WriteObject(FileWriterInterface* file_writer) override; + + // MinidumpStreamWriter: + MinidumpStreamType StreamType() const override; + + private: + MINIDUMP_HANDLE_DATA_STREAM handle_data_stream_base_; + std::vector handle_descriptors_; + PointerVector strings_; + + DISALLOW_COPY_AND_ASSIGN(MinidumpHandleDataWriter); +}; + +} // namespace crashpad + +#endif // CRASHPAD_MINIDUMP_MINIDUMP_HANDLE_WRITER_H_ diff --git a/minidump/minidump_handle_writer_test.cc b/minidump/minidump_handle_writer_test.cc new file mode 100644 index 00000000..f2692068 --- /dev/null +++ b/minidump/minidump_handle_writer_test.cc @@ -0,0 +1,127 @@ +// Copyright 2015 The Crashpad Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include "minidump/minidump_handle_writer.h" + +#include + +#include "gtest/gtest.h" +#include "minidump/minidump_file_writer.h" +#include "minidump/test/minidump_file_writer_test_util.h" +#include "minidump/test/minidump_string_writer_test_util.h" +#include "minidump/test/minidump_writable_test_util.h" +#include "util/file/string_file.h" + +namespace crashpad { +namespace test { +namespace { + +// The handle data stream is expected to be the only stream. +void GetHandleDataStream( + const std::string& file_contents, + const MINIDUMP_HANDLE_DATA_STREAM** handle_data_stream) { + const size_t kDirectoryOffset = sizeof(MINIDUMP_HEADER); + const size_t kHandleDataStreamOffset = + kDirectoryOffset + sizeof(MINIDUMP_DIRECTORY); + + const MINIDUMP_DIRECTORY* directory; + const MINIDUMP_HEADER* header = + MinidumpHeaderAtStart(file_contents, &directory); + ASSERT_NO_FATAL_FAILURE(VerifyMinidumpHeader(header, 1, 0)); + ASSERT_TRUE(directory); + + const size_t kDirectoryIndex = 0; + + ASSERT_EQ(kMinidumpStreamTypeHandleData, + directory[kDirectoryIndex].StreamType); + EXPECT_EQ(kHandleDataStreamOffset, directory[kDirectoryIndex].Location.Rva); + + *handle_data_stream = + MinidumpWritableAtLocationDescriptor( + file_contents, directory[kDirectoryIndex].Location); + ASSERT_TRUE(*handle_data_stream); +} + +TEST(MinidumpHandleDataWriter, Empty) { + MinidumpFileWriter minidump_file_writer; + auto handle_data_writer = make_scoped_ptr(new MinidumpHandleDataWriter()); + minidump_file_writer.AddStream(handle_data_writer.Pass()); + + StringFile string_file; + ASSERT_TRUE(minidump_file_writer.WriteEverything(&string_file)); + + ASSERT_EQ(sizeof(MINIDUMP_HEADER) + sizeof(MINIDUMP_DIRECTORY) + + sizeof(MINIDUMP_HANDLE_DATA_STREAM), + string_file.string().size()); + + const MINIDUMP_HANDLE_DATA_STREAM* handle_data_stream = nullptr; + ASSERT_NO_FATAL_FAILURE( + GetHandleDataStream(string_file.string(), &handle_data_stream)); + + EXPECT_EQ(0u, handle_data_stream->NumberOfDescriptors); +} + +TEST(MinidumpHandleDataWriter, OneHandle) { + MinidumpFileWriter minidump_file_writer; + auto handle_data_writer = make_scoped_ptr(new MinidumpHandleDataWriter()); + + HandleSnapshot handle_snapshot; + handle_snapshot.handle = 0x1234; + handle_snapshot.type_name = L"Something"; + handle_snapshot.attributes = 0x12345678; + handle_snapshot.granted_access = 0x9abcdef0; + handle_snapshot.pointer_count = 4567; + handle_snapshot.handle_count = 9876; + + std::vector snapshot; + snapshot.push_back(handle_snapshot); + + handle_data_writer->InitializeFromSnapshot(snapshot); + + minidump_file_writer.AddStream(handle_data_writer.Pass()); + + StringFile string_file; + ASSERT_TRUE(minidump_file_writer.WriteEverything(&string_file)); + + const size_t kTypeNameStringDataLength = + (handle_snapshot.type_name.size() + 1) * + sizeof(handle_snapshot.type_name[0]); + ASSERT_EQ(sizeof(MINIDUMP_HEADER) + sizeof(MINIDUMP_DIRECTORY) + + sizeof(MINIDUMP_HANDLE_DATA_STREAM) + + sizeof(MINIDUMP_HANDLE_DESCRIPTOR) + sizeof(MINIDUMP_STRING) + + kTypeNameStringDataLength, + string_file.string().size()); + + const MINIDUMP_HANDLE_DATA_STREAM* handle_data_stream = nullptr; + ASSERT_NO_FATAL_FAILURE( + GetHandleDataStream(string_file.string(), &handle_data_stream)); + + EXPECT_EQ(1u, handle_data_stream->NumberOfDescriptors); + const MINIDUMP_HANDLE_DESCRIPTOR* handle_descriptor = + reinterpret_cast( + &handle_data_stream[1]); + EXPECT_EQ(handle_snapshot.handle, handle_descriptor->Handle); + EXPECT_EQ(handle_snapshot.type_name, + MinidumpStringAtRVAAsString(string_file.string(), + handle_descriptor->TypeNameRva)); + EXPECT_EQ(0u, handle_descriptor->ObjectNameRva); + EXPECT_EQ(handle_snapshot.attributes, handle_descriptor->Attributes); + EXPECT_EQ(handle_snapshot.granted_access, handle_descriptor->GrantedAccess); + EXPECT_EQ(handle_snapshot.handle_count, handle_descriptor->HandleCount); + EXPECT_EQ(handle_snapshot.pointer_count, handle_descriptor->PointerCount); +} + +} // namespace +} // namespace test +} // namespace crashpad diff --git a/minidump/minidump_memory_info_writer_test.cc b/minidump/minidump_memory_info_writer_test.cc index 21081d53..b2ff4a86 100644 --- a/minidump/minidump_memory_info_writer_test.cc +++ b/minidump/minidump_memory_info_writer_test.cc @@ -51,7 +51,7 @@ void GetMemoryInfoListStream( *memory_info_list = MinidumpWritableAtLocationDescriptor( file_contents, directory[kDirectoryIndex].Location); - ASSERT_TRUE(memory_info_list); + ASSERT_TRUE(*memory_info_list); } TEST(MinidumpMemoryInfoWriter, Empty) { diff --git a/minidump/minidump_string_writer.h b/minidump/minidump_string_writer.h index ec57179e..5b82bfd2 100644 --- a/minidump/minidump_string_writer.h +++ b/minidump/minidump_string_writer.h @@ -99,6 +99,13 @@ class MinidumpUTF16StringWriter final //! \note Valid in #kStateMutable. void SetUTF8(const std::string& string_utf8); + //! \brief Sets the given UTF-16 string as the string to be written. + //! + //! \note Valid in #kStateMutable. + void SetUTF16(const base::string16& string_utf16) { + set_string(string_utf16); + } + private: DISALLOW_COPY_AND_ASSIGN(MinidumpUTF16StringWriter); }; diff --git a/minidump/minidump_string_writer_test.cc b/minidump/minidump_string_writer_test.cc index aa8e48c6..c5bcc49c 100644 --- a/minidump/minidump_string_writer_test.cc +++ b/minidump/minidump_string_writer_test.cc @@ -53,6 +53,23 @@ TEST(MinidumpStringWriter, MinidumpUTF16StringWriter) { MinidumpStringAtRVAAsString(string_file.string(), 0)); } + { + SCOPED_TRACE("no conversion"); + string_file.Reset(); + crashpad::internal::MinidumpUTF16StringWriter string_writer; + const base::string16 kString(L"oóöőo"); + string_writer.SetUTF16(kString); + EXPECT_TRUE(string_writer.WriteEverything(&string_file)); + ASSERT_EQ( + sizeof(MINIDUMP_STRING) + (kString.size() + 1) * sizeof(kString[0]), + string_file.string().size()); + + const MINIDUMP_STRING* minidump_string = + MinidumpStringAtRVA(string_file.string(), 0); + EXPECT_TRUE(minidump_string); + EXPECT_EQ(kString, MinidumpStringAtRVAAsString(string_file.string(), 0)); + } + const struct { size_t input_length; const char* input_string; diff --git a/minidump/minidump_test.gyp b/minidump/minidump_test.gyp index 09b80cc2..6f8dc198 100644 --- a/minidump/minidump_test.gyp +++ b/minidump/minidump_test.gyp @@ -36,6 +36,7 @@ 'minidump_context_writer_test.cc', 'minidump_crashpad_info_writer_test.cc', 'minidump_exception_writer_test.cc', + 'minidump_handle_writer_test.cc', 'minidump_file_writer_test.cc', 'minidump_memory_info_writer_test.cc', 'minidump_memory_writer_test.cc', diff --git a/minidump/test/minidump_writable_test_util.cc b/minidump/test/minidump_writable_test_util.cc index ef00f40a..c8ea4f44 100644 --- a/minidump/test/minidump_writable_test_util.cc +++ b/minidump/test/minidump_writable_test_util.cc @@ -181,6 +181,14 @@ struct MinidumpThreadListTraits { } }; +struct MinidumpHandleDataStreamTraits { + using ListType = MINIDUMP_HANDLE_DATA_STREAM; + enum : size_t { kElementSize = sizeof(MINIDUMP_HANDLE_DESCRIPTOR) }; + static size_t ElementCount(const ListType* list) { + return static_cast(list->NumberOfDescriptors); + } +}; + struct MinidumpMemoryInfoListTraits { using ListType = MINIDUMP_MEMORY_INFO_LIST; enum : size_t { kElementSize = sizeof(MINIDUMP_MEMORY_INFO) }; @@ -252,6 +260,14 @@ const MINIDUMP_THREAD_LIST* MinidumpWritableAtLocationDescriptor< file_contents, location); } +template <> +const MINIDUMP_HANDLE_DATA_STREAM* MinidumpWritableAtLocationDescriptor< + MINIDUMP_HANDLE_DATA_STREAM>(const std::string& file_contents, + const MINIDUMP_LOCATION_DESCRIPTOR& location) { + return MinidumpListAtLocationDescriptor( + file_contents, location); +} + template <> const MINIDUMP_MEMORY_INFO_LIST* MinidumpWritableAtLocationDescriptor< MINIDUMP_MEMORY_INFO_LIST>(const std::string& file_contents, diff --git a/minidump/test/minidump_writable_test_util.h b/minidump/test/minidump_writable_test_util.h index 9cba8be0..f6190283 100644 --- a/minidump/test/minidump_writable_test_util.h +++ b/minidump/test/minidump_writable_test_util.h @@ -90,6 +90,7 @@ MINIDUMP_ALLOW_OVERSIZED_DATA(MINIDUMP_DIRECTORY); MINIDUMP_ALLOW_OVERSIZED_DATA(MINIDUMP_MEMORY_LIST); MINIDUMP_ALLOW_OVERSIZED_DATA(MINIDUMP_MODULE_LIST); MINIDUMP_ALLOW_OVERSIZED_DATA(MINIDUMP_THREAD_LIST); +MINIDUMP_ALLOW_OVERSIZED_DATA(MINIDUMP_HANDLE_DATA_STREAM); MINIDUMP_ALLOW_OVERSIZED_DATA(MINIDUMP_MEMORY_INFO_LIST); MINIDUMP_ALLOW_OVERSIZED_DATA(MinidumpModuleCrashpadInfoList); MINIDUMP_ALLOW_OVERSIZED_DATA(MinidumpRVAList); @@ -190,6 +191,11 @@ const MINIDUMP_THREAD_LIST* MinidumpWritableAtLocationDescriptor< MINIDUMP_THREAD_LIST>(const std::string& file_contents, const MINIDUMP_LOCATION_DESCRIPTOR& location); +template <> +const MINIDUMP_HANDLE_DATA_STREAM* MinidumpWritableAtLocationDescriptor< + MINIDUMP_HANDLE_DATA_STREAM>(const std::string& file_contents, + const MINIDUMP_LOCATION_DESCRIPTOR& location); + template <> const MINIDUMP_MEMORY_INFO_LIST* MinidumpWritableAtLocationDescriptor< MINIDUMP_MEMORY_INFO_LIST>(const std::string& file_contents, diff --git a/snapshot/win/end_to_end_test.py b/snapshot/win/end_to_end_test.py index a09edd46..2252bd58 100644 --- a/snapshot/win/end_to_end_test.py +++ b/snapshot/win/end_to_end_test.py @@ -183,10 +183,14 @@ def RunTests(cdb_path, dump_path, pipe_name): out = CdbRun(cdb_path, dump_path, '!locks') out.Check(r'CritSec crashy_program!crashpad::`anonymous namespace\'::' r'g_test_critical_section', 'lock was captured') - if float(platform.win32_ver()[0]) != 7: + if platform.win32_ver()[0] != '7': # We can't allocate CRITICAL_SECTIONs with .DebugInfo on Win 7. out.Check(r'\*\*\* Locked', 'lock debug info was captured, and is locked') + out = CdbRun(cdb_path, dump_path, '!handle') + out.Check(r'\d+ Handles', 'captured handles') + out.Check(r'Event\s+\d+', 'capture some event handles') + out.Check(r'File\s+\d+', 'capture some file handles') def main(args): try: @@ -202,8 +206,10 @@ def main(args): # Make sure we can download Windows symbols. if not os.environ.get('_NT_SYMBOL_PATH'): symbol_dir = MakeTempDir() + protocol = 'https' if platform.win32_ver()[0] != 'XP' else 'http' os.environ['_NT_SYMBOL_PATH'] = ( - 'SRV*' + symbol_dir + '*https://msdl.microsoft.com/download/symbols') + 'SRV*' + symbol_dir + '*' + + protocol + '://msdl.microsoft.com/download/symbols') pipe_name = r'\\.\pipe\end-to-end_%s_%s' % ( os.getpid(), str(random.getrandbits(64))) From fe49473b3d88f00c8796c063b7362e27d58567e0 Mon Sep 17 00:00:00 2001 From: Scott Graham Date: Wed, 21 Oct 2015 11:39:53 -0700 Subject: [PATCH 06/10] Fix mac after https://codereview.chromium.org/1419623003/ L"" and wstring are a bit of a mess cross-platform, so just store the type name as UTF8 instead. R=mark@chromium.org BUG=crashpad:21, crashpad:52 Review URL: https://codereview.chromium.org/1421473005 . --- minidump/minidump_handle_writer.cc | 2 +- minidump/minidump_handle_writer_test.cc | 10 +++++----- minidump/minidump_string_writer.h | 7 ------- minidump/minidump_string_writer_test.cc | 17 ----------------- snapshot/handle_snapshot.h | 4 ++-- snapshot/win/process_snapshot_win.cc | 6 +++++- 6 files changed, 13 insertions(+), 33 deletions(-) diff --git a/minidump/minidump_handle_writer.cc b/minidump/minidump_handle_writer.cc index 235d71b0..73c518c1 100644 --- a/minidump/minidump_handle_writer.cc +++ b/minidump/minidump_handle_writer.cc @@ -50,7 +50,7 @@ void MinidumpHandleDataWriter::InitializeFromSnapshot( // TODO(scottmg): There is often a number of repeated type names here, the // strings ought to be pooled. strings_.push_back(new internal::MinidumpUTF16StringWriter()); - strings_.back()->SetUTF16(handle_snapshot.type_name); + strings_.back()->SetUTF8(handle_snapshot.type_name); strings_.back()->RegisterRVA(&descriptor.TypeNameRva); } diff --git a/minidump/minidump_handle_writer_test.cc b/minidump/minidump_handle_writer_test.cc index f2692068..602ce1c3 100644 --- a/minidump/minidump_handle_writer_test.cc +++ b/minidump/minidump_handle_writer_test.cc @@ -16,6 +16,7 @@ #include +#include "base/strings/utf_string_conversions.h" #include "gtest/gtest.h" #include "minidump/minidump_file_writer.h" #include "minidump/test/minidump_file_writer_test_util.h" @@ -78,7 +79,7 @@ TEST(MinidumpHandleDataWriter, OneHandle) { HandleSnapshot handle_snapshot; handle_snapshot.handle = 0x1234; - handle_snapshot.type_name = L"Something"; + handle_snapshot.type_name = "Something"; handle_snapshot.attributes = 0x12345678; handle_snapshot.granted_access = 0x9abcdef0; handle_snapshot.pointer_count = 4567; @@ -95,8 +96,7 @@ TEST(MinidumpHandleDataWriter, OneHandle) { ASSERT_TRUE(minidump_file_writer.WriteEverything(&string_file)); const size_t kTypeNameStringDataLength = - (handle_snapshot.type_name.size() + 1) * - sizeof(handle_snapshot.type_name[0]); + (handle_snapshot.type_name.size() + 1) * sizeof(base::char16); ASSERT_EQ(sizeof(MINIDUMP_HEADER) + sizeof(MINIDUMP_DIRECTORY) + sizeof(MINIDUMP_HANDLE_DATA_STREAM) + sizeof(MINIDUMP_HANDLE_DESCRIPTOR) + sizeof(MINIDUMP_STRING) + @@ -113,8 +113,8 @@ TEST(MinidumpHandleDataWriter, OneHandle) { &handle_data_stream[1]); EXPECT_EQ(handle_snapshot.handle, handle_descriptor->Handle); EXPECT_EQ(handle_snapshot.type_name, - MinidumpStringAtRVAAsString(string_file.string(), - handle_descriptor->TypeNameRva)); + base::UTF16ToUTF8(MinidumpStringAtRVAAsString( + string_file.string(), handle_descriptor->TypeNameRva))); EXPECT_EQ(0u, handle_descriptor->ObjectNameRva); EXPECT_EQ(handle_snapshot.attributes, handle_descriptor->Attributes); EXPECT_EQ(handle_snapshot.granted_access, handle_descriptor->GrantedAccess); diff --git a/minidump/minidump_string_writer.h b/minidump/minidump_string_writer.h index 5b82bfd2..ec57179e 100644 --- a/minidump/minidump_string_writer.h +++ b/minidump/minidump_string_writer.h @@ -99,13 +99,6 @@ class MinidumpUTF16StringWriter final //! \note Valid in #kStateMutable. void SetUTF8(const std::string& string_utf8); - //! \brief Sets the given UTF-16 string as the string to be written. - //! - //! \note Valid in #kStateMutable. - void SetUTF16(const base::string16& string_utf16) { - set_string(string_utf16); - } - private: DISALLOW_COPY_AND_ASSIGN(MinidumpUTF16StringWriter); }; diff --git a/minidump/minidump_string_writer_test.cc b/minidump/minidump_string_writer_test.cc index c5bcc49c..aa8e48c6 100644 --- a/minidump/minidump_string_writer_test.cc +++ b/minidump/minidump_string_writer_test.cc @@ -53,23 +53,6 @@ TEST(MinidumpStringWriter, MinidumpUTF16StringWriter) { MinidumpStringAtRVAAsString(string_file.string(), 0)); } - { - SCOPED_TRACE("no conversion"); - string_file.Reset(); - crashpad::internal::MinidumpUTF16StringWriter string_writer; - const base::string16 kString(L"oóöőo"); - string_writer.SetUTF16(kString); - EXPECT_TRUE(string_writer.WriteEverything(&string_file)); - ASSERT_EQ( - sizeof(MINIDUMP_STRING) + (kString.size() + 1) * sizeof(kString[0]), - string_file.string().size()); - - const MINIDUMP_STRING* minidump_string = - MinidumpStringAtRVA(string_file.string(), 0); - EXPECT_TRUE(minidump_string); - EXPECT_EQ(kString, MinidumpStringAtRVAAsString(string_file.string(), 0)); - } - const struct { size_t input_length; const char* input_string; diff --git a/snapshot/handle_snapshot.h b/snapshot/handle_snapshot.h index b86706e7..1346b41d 100644 --- a/snapshot/handle_snapshot.h +++ b/snapshot/handle_snapshot.h @@ -25,8 +25,8 @@ struct HandleSnapshot { HandleSnapshot(); ~HandleSnapshot(); - //! \brief A string representation of the handle's type. - std::wstring type_name; + //! \brief A UTF-8 string representation of the handle's type. + std::string type_name; //! \brief The handle's value. uint32_t handle; diff --git a/snapshot/win/process_snapshot_win.cc b/snapshot/win/process_snapshot_win.cc index ca26d4b8..e5ee48a8 100644 --- a/snapshot/win/process_snapshot_win.cc +++ b/snapshot/win/process_snapshot_win.cc @@ -18,6 +18,7 @@ #include "base/logging.h" #include "base/strings/stringprintf.h" +#include "base/strings/utf_string_conversions.h" #include "snapshot/win/memory_snapshot_win.h" #include "snapshot/win/module_snapshot_win.h" #include "util/win/registration_protocol_win.h" @@ -211,7 +212,10 @@ std::vector ProcessSnapshotWin::Handles() const { std::vector result; for (const auto& handle : process_reader_.GetProcessInfo().Handles()) { HandleSnapshot snapshot; - snapshot.type_name = handle.type_name; + // This is probably not strictly correct, but these are not localized so we + // expect them all to be in ASCII range anyway. This will need to be more + // carefully done if the object name is added. + snapshot.type_name = base::UTF16ToUTF8(handle.type_name); snapshot.handle = handle.handle; snapshot.attributes = handle.attributes; snapshot.granted_access = handle.granted_access; From 1407b21d695fbebdb2ae2f3c326360fa91d97e7c Mon Sep 17 00:00:00 2001 From: Scott Graham Date: Wed, 21 Oct 2015 13:25:48 -0700 Subject: [PATCH 07/10] Pool TypeName strings when writing MINIDUMP_HANDLE_DESCRIPTOR Follow up to TODO in https://codereview.chromium.org/1419623003/. R=mark@chromium.org BUG=crashpad:21, crashpad:52 Review URL: https://codereview.chromium.org/1411793005 . --- minidump/minidump_handle_writer.cc | 22 +++++--- minidump/minidump_handle_writer.h | 5 +- minidump/minidump_handle_writer_test.cc | 74 +++++++++++++++++++++++++ 3 files changed, 91 insertions(+), 10 deletions(-) diff --git a/minidump/minidump_handle_writer.cc b/minidump/minidump_handle_writer.cc index 73c518c1..3d8a969e 100644 --- a/minidump/minidump_handle_writer.cc +++ b/minidump/minidump_handle_writer.cc @@ -17,6 +17,7 @@ #include #include "base/logging.h" +#include "base/stl_util.h" #include "minidump/minidump_extensions.h" #include "util/file/file_writer.h" @@ -27,6 +28,7 @@ MinidumpHandleDataWriter::MinidumpHandleDataWriter() } MinidumpHandleDataWriter::~MinidumpHandleDataWriter() { + STLDeleteContainerPairSecondPointers(strings_.begin(), strings_.end()); } void MinidumpHandleDataWriter::InitializeFromSnapshot( @@ -37,7 +39,6 @@ void MinidumpHandleDataWriter::InitializeFromSnapshot( // Because we RegisterRVA() on the string writer below, we preallocate and // never resize the handle_descriptors_ vector. handle_descriptors_.resize(handle_snapshots.size()); - strings_.reserve(handle_snapshots.size()); for (size_t i = 0; i < handle_snapshots.size(); ++i) { const HandleSnapshot& handle_snapshot = handle_snapshots[i]; MINIDUMP_HANDLE_DESCRIPTOR& descriptor = handle_descriptors_[i]; @@ -47,11 +48,16 @@ void MinidumpHandleDataWriter::InitializeFromSnapshot( if (handle_snapshot.type_name.empty()) { descriptor.TypeNameRva = 0; } else { - // TODO(scottmg): There is often a number of repeated type names here, the - // strings ought to be pooled. - strings_.push_back(new internal::MinidumpUTF16StringWriter()); - strings_.back()->SetUTF8(handle_snapshot.type_name); - strings_.back()->RegisterRVA(&descriptor.TypeNameRva); + auto it = strings_.lower_bound(handle_snapshot.type_name); + internal::MinidumpUTF16StringWriter* writer; + if (it != strings_.end() && it->first == handle_snapshot.type_name) { + writer = it->second; + } else { + writer = new internal::MinidumpUTF16StringWriter(); + strings_.insert(it, std::make_pair(handle_snapshot.type_name, writer)); + writer->SetUTF8(handle_snapshot.type_name); + } + writer->RegisterRVA(&descriptor.TypeNameRva); } descriptor.ObjectNameRva = 0; @@ -86,8 +92,8 @@ std::vector MinidumpHandleDataWriter::Children() { DCHECK_GE(state(), kStateFrozen); std::vector children; - for (auto* string : strings_) - children.push_back(string); + for (const auto& pair : strings_) + children.push_back(pair.second); return children; } diff --git a/minidump/minidump_handle_writer.h b/minidump/minidump_handle_writer.h index daf1fbcd..43bdac6b 100644 --- a/minidump/minidump_handle_writer.h +++ b/minidump/minidump_handle_writer.h @@ -18,13 +18,14 @@ #include #include +#include +#include #include #include "minidump/minidump_stream_writer.h" #include "minidump/minidump_string_writer.h" #include "minidump/minidump_writable.h" #include "snapshot/handle_snapshot.h" -#include "util/stdlib/pointer_container.h" namespace crashpad { @@ -65,7 +66,7 @@ class MinidumpHandleDataWriter final : public internal::MinidumpStreamWriter { private: MINIDUMP_HANDLE_DATA_STREAM handle_data_stream_base_; std::vector handle_descriptors_; - PointerVector strings_; + std::map strings_; DISALLOW_COPY_AND_ASSIGN(MinidumpHandleDataWriter); }; diff --git a/minidump/minidump_handle_writer_test.cc b/minidump/minidump_handle_writer_test.cc index 602ce1c3..e8f23eb5 100644 --- a/minidump/minidump_handle_writer_test.cc +++ b/minidump/minidump_handle_writer_test.cc @@ -122,6 +122,80 @@ TEST(MinidumpHandleDataWriter, OneHandle) { EXPECT_EQ(handle_snapshot.pointer_count, handle_descriptor->PointerCount); } +TEST(MinidumpHandleDataWriter, RepeatedTypeName) { + MinidumpFileWriter minidump_file_writer; + auto handle_data_writer = make_scoped_ptr(new MinidumpHandleDataWriter()); + + HandleSnapshot handle_snapshot; + handle_snapshot.handle = 0x1234; + handle_snapshot.type_name = "Something"; + handle_snapshot.attributes = 0x12345678; + handle_snapshot.granted_access = 0x9abcdef0; + handle_snapshot.pointer_count = 4567; + handle_snapshot.handle_count = 9876; + + HandleSnapshot handle_snapshot2; + handle_snapshot2.handle = 0x4321; + handle_snapshot2.type_name = "Something"; // Note: same as above. + handle_snapshot2.attributes = 0x87654321; + handle_snapshot2.granted_access = 0x0fedcba9; + handle_snapshot2.pointer_count = 7654; + handle_snapshot2.handle_count = 6789; + + std::vector snapshot; + snapshot.push_back(handle_snapshot); + snapshot.push_back(handle_snapshot2); + + handle_data_writer->InitializeFromSnapshot(snapshot); + + minidump_file_writer.AddStream(handle_data_writer.Pass()); + + StringFile string_file; + ASSERT_TRUE(minidump_file_writer.WriteEverything(&string_file)); + + const size_t kTypeNameStringDataLength = + (handle_snapshot.type_name.size() + 1) * sizeof(base::char16); + ASSERT_EQ(sizeof(MINIDUMP_HEADER) + sizeof(MINIDUMP_DIRECTORY) + + sizeof(MINIDUMP_HANDLE_DATA_STREAM) + + (sizeof(MINIDUMP_HANDLE_DESCRIPTOR) * 2) + + sizeof(MINIDUMP_STRING) + kTypeNameStringDataLength, + string_file.string().size()); + + const MINIDUMP_HANDLE_DATA_STREAM* handle_data_stream = nullptr; + ASSERT_NO_FATAL_FAILURE( + GetHandleDataStream(string_file.string(), &handle_data_stream)); + + EXPECT_EQ(2u, handle_data_stream->NumberOfDescriptors); + const MINIDUMP_HANDLE_DESCRIPTOR* handle_descriptor = + reinterpret_cast( + &handle_data_stream[1]); + EXPECT_EQ(handle_snapshot.handle, handle_descriptor->Handle); + EXPECT_EQ(handle_snapshot.type_name, + base::UTF16ToUTF8(MinidumpStringAtRVAAsString( + string_file.string(), handle_descriptor->TypeNameRva))); + EXPECT_EQ(0u, handle_descriptor->ObjectNameRva); + EXPECT_EQ(handle_snapshot.attributes, handle_descriptor->Attributes); + EXPECT_EQ(handle_snapshot.granted_access, handle_descriptor->GrantedAccess); + EXPECT_EQ(handle_snapshot.handle_count, handle_descriptor->HandleCount); + EXPECT_EQ(handle_snapshot.pointer_count, handle_descriptor->PointerCount); + + const MINIDUMP_HANDLE_DESCRIPTOR* handle_descriptor2 = + reinterpret_cast( + reinterpret_cast(&handle_data_stream[1]) + + sizeof(MINIDUMP_HANDLE_DESCRIPTOR)); + EXPECT_EQ(handle_snapshot2.handle, handle_descriptor2->Handle); + EXPECT_EQ(handle_snapshot2.type_name, + base::UTF16ToUTF8(MinidumpStringAtRVAAsString( + string_file.string(), handle_descriptor2->TypeNameRva))); + EXPECT_EQ(0u, handle_descriptor2->ObjectNameRva); + EXPECT_EQ(handle_snapshot2.attributes, handle_descriptor2->Attributes); + EXPECT_EQ(handle_snapshot2.granted_access, handle_descriptor2->GrantedAccess); + EXPECT_EQ(handle_snapshot2.handle_count, handle_descriptor2->HandleCount); + EXPECT_EQ(handle_snapshot2.pointer_count, handle_descriptor2->PointerCount); + + EXPECT_EQ(handle_descriptor->TypeNameRva, handle_descriptor2->TypeNameRva); +} + } // namespace } // namespace test } // namespace crashpad From 38b7e919f87cc4537be1ac451af1fb130fe0e4f1 Mon Sep 17 00:00:00 2001 From: Scott Graham Date: Wed, 21 Oct 2015 15:35:57 -0700 Subject: [PATCH 08/10] win: Fix x64 compile error in handle writer R=mark@chromium.org BUG=crashpad:52 Review URL: https://codereview.chromium.org/1408073005 . --- minidump/minidump_handle_writer.cc | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/minidump/minidump_handle_writer.cc b/minidump/minidump_handle_writer.cc index 3d8a969e..73eeb0c7 100644 --- a/minidump/minidump_handle_writer.cc +++ b/minidump/minidump_handle_writer.cc @@ -20,6 +20,7 @@ #include "base/stl_util.h" #include "minidump/minidump_extensions.h" #include "util/file/file_writer.h" +#include "util/numeric/safe_assignment.h" namespace crashpad { @@ -76,7 +77,12 @@ bool MinidumpHandleDataWriter::Freeze() { handle_data_stream_base_.SizeOfHeader = sizeof(handle_data_stream_base_); handle_data_stream_base_.SizeOfDescriptor = sizeof(handle_descriptors_[0]); - handle_data_stream_base_.NumberOfDescriptors = handle_descriptors_.size(); + const size_t handle_count = handle_descriptors_.size(); + if (!AssignIfInRange(&handle_data_stream_base_.NumberOfDescriptors, + handle_count)) { + LOG(ERROR) << "handle_count " << handle_count << " out of range"; + return false; + } handle_data_stream_base_.Reserved = 0; return true; From 0615a592852893f6f25d2c09d8236f86918eaf1a Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Wed, 21 Oct 2015 18:41:39 -0400 Subject: [PATCH 09/10] doc: Update status to reflect Windows and Android progress R=scottmg@chromium.org Review URL: https://codereview.chromium.org/1415313003 . --- doc/status.ad | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/doc/status.ad b/doc/status.ad index 1d5933b5..301195fc 100644 --- a/doc/status.ad +++ b/doc/status.ad @@ -28,15 +28,18 @@ 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]. With the -exception of the snapshot library, most major components are now working on -Windows, and work is progressing on the Windows snapshot implementation. +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. + +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. == Future There are plans to bring Crashpad clients to other operating systems in the -future, including -https://code.google.com/p/crashpad/issues/detail?id=30[Android] and, more -generically, Linux. There are also plans to implement a +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. From 90ef7475cdb131f56c23b494d4fe60b22138239e Mon Sep 17 00:00:00 2001 From: Scott Graham Date: Wed, 21 Oct 2015 16:07:03 -0700 Subject: [PATCH 10/10] win: Validate readability of memory ranges added to minidump R=mark@chromium.org BUG=crashpad:59 Review URL: https://codereview.chromium.org/1412243005 . --- handler/handler.gyp | 18 ++++ handler/win/self_destroying_test_program.cc | 97 +++++++++++++++++++++ snapshot/win/end_to_end_test.py | 38 ++++++-- snapshot/win/process_snapshot_win.cc | 15 +--- snapshot/win/thread_snapshot_win.cc | 22 +++-- util/win/process_info.cc | 20 +++++ util/win/process_info.h | 10 +++ util/win/process_info_test.cc | 14 +++ 8 files changed, 209 insertions(+), 25 deletions(-) create mode 100644 handler/win/self_destroying_test_program.cc diff --git a/handler/handler.gyp b/handler/handler.gyp index 28bc9e35..04433ac8 100644 --- a/handler/handler.gyp +++ b/handler/handler.gyp @@ -91,6 +91,24 @@ 'win/crashy_test_program.cc', ], }, + { + 'target_name': 'self_destroying_program', + 'type': 'executable', + 'dependencies': [ + '../client/client.gyp:crashpad_client', + '../compat/compat.gyp:crashpad_compat', + '../snapshot/snapshot.gyp:crashpad_snapshot', + '../third_party/mini_chromium/mini_chromium.gyp:base', + '../tools/tools.gyp:crashpad_tool_support', + '../util/util.gyp:crashpad_util', + ], + 'include_dirs': [ + '..', + ], + 'sources': [ + 'win/self_destroying_test_program.cc', + ], + }, ], }, { 'targets': [], diff --git a/handler/win/self_destroying_test_program.cc b/handler/win/self_destroying_test_program.cc new file mode 100644 index 00000000..d00fa476 --- /dev/null +++ b/handler/win/self_destroying_test_program.cc @@ -0,0 +1,97 @@ +// Copyright 2015 The Crashpad Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include +#include +#include +#include + +#include "base/logging.h" +#include "base/strings/stringprintf.h" +#include "client/crashpad_client.h" +#include "snapshot/win/process_reader_win.h" +#include "tools/tool_support.h" + +namespace crashpad { +namespace { + +// We VirtualFree a region in ourselves (the stack) to confirm that the +// exception reporter captures as much as possible in the minidump and doesn't +// abort. __debugbreak() immediately after doing so because the process is +// clearly in a very broken state at this point. +bool FreeOwnStackAndBreak() { + ProcessReaderWin process_reader; + if (!process_reader.Initialize(GetCurrentProcess(), + ProcessSuspensionState::kRunning)) { + LOG(ERROR) << "ProcessReaderWin Initialize"; + return false; + } + + const std::vector threads = + process_reader.Threads(); + if (threads.empty()) { + LOG(ERROR) << "no threads"; + return false; + } + + // Push the stack up a bit so that hopefully the crash handler can succeed, + // but won't be able to read the base of the stack. + _alloca(16384); + + // We can't succeed at MEM_RELEASEing this memory, but MEM_DECOMMIT is good + // enough to make it inaccessible. + if (!VirtualFree(reinterpret_cast(threads[0].stack_region_address), + 100, + MEM_DECOMMIT)) { + PLOG(ERROR) << "VirtualFree"; + return false; + } + + // If the VirtualFree() succeeds, we may have already crashed. __debugbreak() + // just to be sure. + __debugbreak(); + + return true; +} + +int SelfDestroyingMain(int argc, char* argv[]) { + if (argc != 2) { + fprintf(stderr, "Usage: %s \n", argv[0]); + return EXIT_FAILURE; + } + + CrashpadClient client; + if (!client.SetHandler(argv[1])) { + LOG(ERROR) << "SetHandler"; + return EXIT_FAILURE; + } + if (!client.UseHandler()) { + LOG(ERROR) << "UseHandler"; + return EXIT_FAILURE; + } + + if (!FreeOwnStackAndBreak()) + return EXIT_FAILURE; + + // This will never be reached. On success, we'll have crashed above, or + // otherwise returned before here. + return EXIT_SUCCESS; +} + +} // namespace +} // namespace crashpad + +int wmain(int argc, wchar_t* argv[]) { + return crashpad::ToolSupport::Wmain(argc, argv, crashpad::SelfDestroyingMain); +} diff --git a/snapshot/win/end_to_end_test.py b/snapshot/win/end_to_end_test.py index 2252bd58..9ed5fda7 100644 --- a/snapshot/win/end_to_end_test.py +++ b/snapshot/win/end_to_end_test.py @@ -76,8 +76,8 @@ def GetCdbPath(): return None -def GetDumpFromCrashyProgram(out_dir, pipe_name): - """Initialize a crash database, run crashpad_handler, run crashy_program +def GetDumpFromProgram(out_dir, pipe_name, executable_name): + """Initialize a crash database, run crashpad_handler, run |executable_name| connecting to the crash_handler. Returns the minidump generated by crash_handler for further testing. """ @@ -97,7 +97,7 @@ def GetDumpFromCrashyProgram(out_dir, pipe_name): '--database=' + test_database ]) - subprocess.call([os.path.join(out_dir, 'crashy_program.exe'), pipe_name]) + subprocess.call([os.path.join(out_dir, executable_name), pipe_name]) out = subprocess.check_output([ os.path.join(out_dir, 'crashpad_database_util.exe'), @@ -113,6 +113,14 @@ def GetDumpFromCrashyProgram(out_dir, pipe_name): handler.kill() +def GetDumpFromCrashyProgram(out_dir, pipe_name): + return GetDumpFromProgram(out_dir, pipe_name, 'crashy_program.exe') + + +def GetDumpFromSelfDestroyingProgram(out_dir, pipe_name): + return GetDumpFromProgram(out_dir, pipe_name, 'self_destroying_program.exe') + + class CdbRun(object): """Run cdb.exe passing it a cdb command and capturing the output. `Check()` searches for regex patterns in sequence allowing verification of @@ -147,7 +155,7 @@ class CdbRun(object): sys.exit(1) -def RunTests(cdb_path, dump_path, pipe_name): +def RunTests(cdb_path, dump_path, destroyed_dump_path, pipe_name): """Runs various tests in sequence. Runs a new cdb instance on the dump for each block of tests to reduce the chances that output from one command is confused for output from another. @@ -192,6 +200,18 @@ def RunTests(cdb_path, dump_path, pipe_name): out.Check(r'Event\s+\d+', 'capture some event handles') out.Check(r'File\s+\d+', 'capture some file handles') + out = CdbRun(cdb_path, destroyed_dump_path, '.ecxr;!peb;k 2') + out.Check(r'Ldr\.InMemoryOrderModuleList:.*\d+ \. \d+', 'PEB_LDR_DATA saved') + out.Check(r'ntdll\.dll', 'ntdll present', re.IGNORECASE) + + # Check that there is no stack trace in the self-destroyed process. Confirm + # that the top is where we expect it (that's based only on IP), but subsequent + # stack entries will not be available. This confirms that we have a mostly + # valid dump, but that the stack was omitted. + out.Check(r'self_destroying_program!crashpad::`anonymous namespace\'::' + r'FreeOwnStackAndBreak.*\nquit:', + 'at correct location, no additional stack entries') + def main(args): try: if len(args) != 1: @@ -214,11 +234,15 @@ def main(args): pipe_name = r'\\.\pipe\end-to-end_%s_%s' % ( os.getpid(), str(random.getrandbits(64))) - dump_path = GetDumpFromCrashyProgram(args[0], pipe_name) - if not dump_path: + crashy_dump_path = GetDumpFromCrashyProgram(args[0], pipe_name) + if not crashy_dump_path: return 1 - RunTests(cdb_path, dump_path, pipe_name) + destroyed_dump_path = GetDumpFromSelfDestroyingProgram(args[0], pipe_name) + if not destroyed_dump_path: + return 1 + + RunTests(cdb_path, crashy_dump_path, destroyed_dump_path, pipe_name) return 0 finally: diff --git a/snapshot/win/process_snapshot_win.cc b/snapshot/win/process_snapshot_win.cc index e5ee48a8..bc3b7b1a 100644 --- a/snapshot/win/process_snapshot_win.cc +++ b/snapshot/win/process_snapshot_win.cc @@ -344,19 +344,8 @@ void ProcessSnapshotWin::AddMemorySnapshot( if (size == 0) return; - // Ensure that the entire range is readable. TODO(scottmg): Consider - // generalizing this as part of - // https://code.google.com/p/crashpad/issues/detail?id=59. - auto ranges = process_reader_.GetProcessInfo().GetReadableRanges( - CheckedRange(address, size)); - if (ranges.size() != 1) { - LOG(ERROR) << base::StringPrintf( - "range at 0x%llx, size 0x%llx fully unreadable", address, size); - return; - } - if (ranges[0].base() != address || ranges[0].size() != size) { - LOG(ERROR) << base::StringPrintf( - "some of range at 0x%llx, size 0x%llx unreadable", address, size); + if (!process_reader_.GetProcessInfo().LoggingRangeIsFullyReadable( + CheckedRange(address, size))) { return; } diff --git a/snapshot/win/thread_snapshot_win.cc b/snapshot/win/thread_snapshot_win.cc index 8a0de39e..3593a7e1 100644 --- a/snapshot/win/thread_snapshot_win.cc +++ b/snapshot/win/thread_snapshot_win.cc @@ -41,11 +41,23 @@ bool ThreadSnapshotWin::Initialize( INITIALIZATION_STATE_SET_INITIALIZING(initialized_); thread_ = process_reader_thread; - // TODO(scottmg): Ensure these regions are readable - // https://code.google.com/p/crashpad/issues/detail?id=59 - stack_.Initialize( - process_reader, thread_.stack_region_address, thread_.stack_region_size); - teb_.Initialize(process_reader, thread_.teb_address, thread_.teb_size); + if (process_reader->GetProcessInfo().LoggingRangeIsFullyReadable( + CheckedRange(thread_.stack_region_address, + thread_.stack_region_size))) { + stack_.Initialize(process_reader, + thread_.stack_region_address, + thread_.stack_region_size); + } else { + stack_.Initialize(process_reader, 0, 0); + } + + if (process_reader->GetProcessInfo().LoggingRangeIsFullyReadable( + CheckedRange(thread_.teb_address, + thread_.teb_size))) { + teb_.Initialize(process_reader, thread_.teb_address, thread_.teb_size); + } else { + teb_.Initialize(process_reader, 0, 0); + } #if defined(ARCH_CPU_X86_64) if (process_reader->Is64Bit()) { diff --git a/util/win/process_info.cc b/util/win/process_info.cc index 7f3ff286..1478d044 100644 --- a/util/win/process_info.cc +++ b/util/win/process_info.cc @@ -600,6 +600,26 @@ ProcessInfo::GetReadableRanges( return GetReadableRangesOfMemoryMap(range, MemoryInfo()); } +bool ProcessInfo::LoggingRangeIsFullyReadable( + const CheckedRange& range) const { + const auto ranges = GetReadableRanges(range); + if (ranges.size() != 1) { + LOG(ERROR) << base::StringPrintf( + "range at 0x%llx, size 0x%llx fully unreadable", + range.base(), + range.size()); + return false; + } + if (ranges[0].base() != range.base() || ranges[0].size() != range.size()) { + LOG(ERROR) << base::StringPrintf( + "some of range at 0x%llx, size 0x%llx unreadable", + range.base(), + range.size()); + return false; + } + return true; +} + const std::vector& ProcessInfo::Handles() const { INITIALIZATION_STATE_DCHECK_VALID(initialized_); if (handles_.empty()) diff --git a/util/win/process_info.h b/util/win/process_info.h index 41b92598..f8ed4ae4 100644 --- a/util/win/process_info.h +++ b/util/win/process_info.h @@ -139,6 +139,16 @@ class ProcessInfo { std::vector> GetReadableRanges( const CheckedRange& range) const; + //! \brief Given a range in the target process, determines if the entire range + //! is readable. + //! + //! \param[in] range The range being inspected. + //! + //! \return `true` if the range is fully readable, otherwise `false` with a + //! message logged. + bool LoggingRangeIsFullyReadable( + const CheckedRange& range) const; + //! \brief Retrieves information about open handles in the target process. const std::vector& Handles() const; diff --git a/util/win/process_info_test.cc b/util/win/process_info_test.cc index 075f8eb4..81e5613e 100644 --- a/util/win/process_info_test.cc +++ b/util/win/process_info_test.cc @@ -623,6 +623,20 @@ TEST(ProcessInfo, Handles) { EXPECT_TRUE(found_mapping_handle); } +TEST(ProcessInfo, OutOfRangeCheck) { + const size_t kAllocationSize = 12345; + scoped_ptr safe_memory(new char[kAllocationSize]); + + ProcessInfo info; + info.Initialize(GetCurrentProcess()); + + EXPECT_TRUE( + info.LoggingRangeIsFullyReadable(CheckedRange( + reinterpret_cast(safe_memory.get()), kAllocationSize))); + EXPECT_FALSE(info.LoggingRangeIsFullyReadable( + CheckedRange(0, 1024))); +} + } // namespace } // namespace test } // namespace crashpad