From 88a681e7479fb7c6458fb87416c1d3981ceb3f6e Mon Sep 17 00:00:00 2001 From: Mark Mentovai Date: Tue, 9 Sep 2014 09:59:05 -0400 Subject: [PATCH] Enhance the MachMessageServer test to cover port right ownership management. TEST=util_test MachMessageServer.* R=rsesek@chromium.org Review URL: https://codereview.chromium.org/555663002 --- util/mach/mach_message_server.cc | 6 + util/mach/mach_message_server_test.cc | 232 ++++++++++++++++++++++++-- 2 files changed, 223 insertions(+), 15 deletions(-) diff --git a/util/mach/mach_message_server.cc b/util/mach/mach_message_server.cc index dfaaa974..98e41041 100644 --- a/util/mach/mach_message_server.cc +++ b/util/mach/mach_message_server.cc @@ -206,6 +206,12 @@ mach_msg_return_t MachMessageServer::Run(Interface* interface, request_header, reply_header, &destroy_complex_request); if (!(reply_header->msgh_bits & MACH_MSGH_BITS_COMPLEX)) { + // This only works if the reply message is not complex, because otherwise, + // the location of the RetCode field is not known. It should be possible + // to locate the RetCode field by looking beyond the descriptors in a + // complex reply message, but this is not currently done. This behavior + // has not proven itself necessary in practice, and it’s not done by + // mach_msg_server() or mach_msg_server_once() either. mig_reply_error_t* reply_mig = reinterpret_cast(reply_header); if (reply_mig->RetCode == MIG_NO_REPLY) { diff --git a/util/mach/mach_message_server_test.cc b/util/mach/mach_message_server_test.cc index dcbbae38..fe01cd1f 100644 --- a/util/mach/mach_message_server_test.cc +++ b/util/mach/mach_message_server_test.cc @@ -60,10 +60,15 @@ class TestMachMessageServer : public MachMessageServer::Interface, server_nonblocking(MachMessageServer::kBlocking), server_timeout_ms(MACH_MSG_TIMEOUT_NONE), server_mig_retcode(KERN_SUCCESS), + server_destroy_complex(true), + expect_server_destroyed_complex(true), expect_server_result(KERN_SUCCESS), client_send_request_count(1), + client_send_complex(false), client_reply_port_type(kReplyPortNormal), - child_send_all_requests_before_receiving_any_replies(false) { + client_expect_reply(true), + child_send_all_requests_before_receiving_any_replies(false), + child_wait_for_parent_pipe(false) { } // true if MachMessageServerFunction() is expected to be called. @@ -90,28 +95,60 @@ class TestMachMessageServer : public MachMessageServer::Interface, // a Mach RPC return value. kern_return_t server_mig_retcode; + // The value that the server function should set its destroy_complex_request + // parameter to. This is true if resources sent in complex request messages + // should be destroyed, and false if they should not be destroyed, assuming + // that the server function indicates success. + bool server_destroy_complex; + + // Whether to expect the server to destroy a complex message. Even if + // server_destroy_complex is false, a complex message will be destroyed if + // the MIG return code was unsuccessful. + bool expect_server_destroyed_complex; + // The expected return value from MachMessageServer::Run(). kern_return_t expect_server_result; // The number of requests that the client should send to the server. size_t client_send_request_count; + // true if the client should send a complex message, one that carries a port + // descriptor in its body. Normally false. + bool client_send_complex; + // The type of reply port that the client should provide in its request’s // mach_msg_header_t::msgh_local_port, which will appear to the server as // mach_msg_header_t::msgh_remote_port. ReplyPortType client_reply_port_type; + // true if the client should wait for a reply from the server. For + // non-normal reply ports or requests which the server responds to with no + // reply (MIG_NO_REPLY), the server will either not send a reply or not + // succeed in sending a reply, and the child process should not wait for + // one. + bool client_expect_reply; + // true if the client should send all requests before attempting to receive // any replies from the server. This is used for the persistent nonblocking // test, which requires the client to fill the server’s queue before the // server can attempt processing it. bool child_send_all_requests_before_receiving_any_replies; + + // true if the child should wait to receive a byte from the parent before + // exiting. This can be used to keep a receive right in the child alive + // until the parent has a chance to verify that it’s holding a send right. + // Otherwise, the right might appear in the parent as a dead name if the + // child exited before the parent had a chance to examine it. This would be + // a race. + bool child_wait_for_parent_pipe; }; explicit TestMachMessageServer(const Options& options) : MachMessageServer::Interface(), MachMultiprocess(), - options_(options) { + options_(options), + child_complex_message_port_(), + parent_complex_message_port_(MACH_PORT_NULL) { } // Runs the test. @@ -131,7 +168,7 @@ class TestMachMessageServer : public MachMessageServer::Interface, mach_msg_header_t* in, mach_msg_header_t* out, bool* destroy_complex_request) override { - *destroy_complex_request = true; + *destroy_complex_request = options_.server_destroy_complex; EXPECT_TRUE(options_.expect_server_interface_method_called); if (!options_.expect_server_interface_method_called) { @@ -144,15 +181,33 @@ class TestMachMessageServer : public MachMessageServer::Interface, const ReceiveRequestMessage* request = reinterpret_cast(in); - EXPECT_EQ(static_cast( - MACH_MSGH_BITS(MACH_MSG_TYPE_MOVE_SEND, MACH_MSG_TYPE_MOVE_SEND)), - request->header.msgh_bits); + const mach_msg_bits_t expect_msgh_bits = + 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(sizeof(RequestMessage), request->header.msgh_size); if (options_.client_reply_port_type == Options::kReplyPortNormal) { EXPECT_EQ(RemotePort(), request->header.msgh_remote_port); } EXPECT_EQ(LocalPort(), request->header.msgh_local_port); EXPECT_EQ(kRequestMessageId, request->header.msgh_id); + if (options_.client_send_complex) { + EXPECT_EQ(1u, request->body.msgh_descriptor_count); + EXPECT_NE(static_cast(MACH_PORT_NULL), + request->port_descriptor.name); + parent_complex_message_port_ = request->port_descriptor.name; + EXPECT_EQ(static_cast(MACH_MSG_TYPE_MOVE_SEND), + request->port_descriptor.disposition); + EXPECT_EQ( + static_cast(MACH_MSG_PORT_DESCRIPTOR), + request->port_descriptor.type); + } else { + EXPECT_EQ(0u, request->body.msgh_descriptor_count); + EXPECT_EQ(static_cast(MACH_PORT_NULL), + request->port_descriptor.name); + EXPECT_EQ(0u, request->port_descriptor.disposition); + EXPECT_EQ(0u, request->port_descriptor.type); + } EXPECT_EQ(0, memcmp(&request->ndr, &NDR_record, sizeof(NDR_record))); EXPECT_EQ(requests_, request->number); EXPECT_EQ(static_cast(MACH_MSG_TRAILER_FORMAT_0), @@ -184,8 +239,8 @@ class TestMachMessageServer : public MachMessageServer::Interface, } private: - struct RequestMessage { - mach_msg_header_t header; + struct RequestMessage : public mach_msg_base_t { + mach_msg_port_descriptor_t port_descriptor; NDR_record_t ndr; uint32_t number; }; @@ -214,6 +269,43 @@ class TestMachMessageServer : public MachMessageServer::Interface, options_.server_nonblocking, options_.server_timeout_ms))) << MachErrorMessage(kr, "MachMessageServer"); + + if (options_.client_send_complex) { + EXPECT_NE(static_cast(MACH_PORT_NULL), + parent_complex_message_port_); + mach_port_type_t type; + + if (!options_.expect_server_destroyed_complex) { + // MachMessageServer should not have destroyed the resources sent in the + // complex request message. + kr = mach_port_type( + mach_task_self(), parent_complex_message_port_, &type); + EXPECT_EQ(KERN_SUCCESS, kr) << MachErrorMessage(kr, "mach_port_type"); + EXPECT_EQ(MACH_PORT_TYPE_SEND, type); + + // Destroy the resources here. + kr = mach_port_deallocate( + mach_task_self(), parent_complex_message_port_); + EXPECT_EQ(KERN_SUCCESS, kr) + << MachErrorMessage(kr, "mach_port_deallocate"); + } + + // The kernel won’t have reused the same name for another Mach port in + // 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); + EXPECT_EQ(KERN_INVALID_NAME, kr) + << MachErrorMessage(kr, "mach_port_type"); + } + + if (options_.child_wait_for_parent_pipe) { + // Let the child know it’s safe to exit. + char c = '\0'; + ssize_t rv = WriteFD(WritePipeFD(), &c, 1); + EXPECT_EQ(1, rv) << ErrnoMessage("write"); + } } virtual void MachMultiprocessChild() override { @@ -253,6 +345,13 @@ class TestMachMessageServer : public MachMessageServer::Interface, } } } + + if (options_.child_wait_for_parent_pipe) { + char c; + ssize_t rv = ReadFD(ReadPipeFD(), &c, 1); + ASSERT_EQ(1, rv) << ErrnoMessage("read"); + ASSERT_EQ('\0', c); + } } // In the child process, sends a request message to the server. @@ -264,7 +363,8 @@ class TestMachMessageServer : public MachMessageServer::Interface, RequestMessage request = {}; request.header.msgh_bits = - MACH_MSGH_BITS(MACH_MSG_TYPE_COPY_SEND, MACH_MSG_TYPE_MAKE_SEND); + MACH_MSGH_BITS(MACH_MSG_TYPE_COPY_SEND, MACH_MSG_TYPE_MAKE_SEND) | + (options_.client_send_complex ? MACH_MSGH_BITS_COMPLEX : 0); request.header.msgh_size = sizeof(request); request.header.msgh_remote_port = RemotePort(); kern_return_t kr; @@ -290,8 +390,27 @@ class TestMachMessageServer : public MachMessageServer::Interface, } } request.header.msgh_id = kRequestMessageId; - request.number = requests_++; + if (options_.client_send_complex) { + // Allocate a new receive right in this process and make a send right that + // will appear in the parent process. This is used to test that the server + // properly handles ownership of resources received in complex messages. + request.body.msgh_descriptor_count = 1; + kr = mach_port_allocate(mach_task_self(), + MACH_PORT_RIGHT_RECEIVE, + &request.port_descriptor.name); + ASSERT_EQ(KERN_SUCCESS, kr) + << MachErrorMessage(kr, "mach_port_allocate"); + child_complex_message_port_.reset(request.port_descriptor.name); + request.port_descriptor.disposition = MACH_MSG_TYPE_MAKE_SEND; + request.port_descriptor.type = MACH_MSG_PORT_DESCRIPTOR; + } else { + request.body.msgh_descriptor_count = 0; + request.port_descriptor.name = MACH_PORT_NULL; + request.port_descriptor.disposition = 0; + request.port_descriptor.type = 0; + } request.ndr = NDR_record; + request.number = requests_++; kr = mach_msg(&request.header, MACH_SEND_MSG | MACH_SEND_TIMEOUT, @@ -305,9 +424,10 @@ class TestMachMessageServer : public MachMessageServer::Interface, // In the child process, waits for a reply message from the server. void ChildWaitForReply() { - if (options_.client_reply_port_type != Options::kReplyPortNormal) { + if (!options_.client_expect_reply) { // The client shouldn’t expect a reply when it didn’t send a good reply - // port with its request. + // port with its request, or when testing the server behaving in a way + // that doesn’t send replies. return; } @@ -375,6 +495,16 @@ class TestMachMessageServer : public MachMessageServer::Interface, const Options& options_; + // A receive right allocated in the child process. A send right will be + // created from this right and sent to the parent parent process in the + // request message. + base::mac::ScopedMachReceiveRight child_complex_message_port_; + + // The send right received in the parent process. This right is stored in a + // member variable to test that resources carried in complex messages are + // properly destroyed in the server when expected. + mach_port_t parent_complex_message_port_; + static uint32_t requests_; static uint32_t replies_; @@ -482,8 +612,18 @@ TEST(MachMessageServer, ReturnCodeInvalidArgument) { // This tests that the mig_reply_error_t::RetCode field is properly returned // to the client. TestMachMessageServer::Options options; - TestMachMessageServer test_mach_message_server(options); options.server_mig_retcode = KERN_INVALID_ARGUMENT; + TestMachMessageServer test_mach_message_server(options); + test_mach_message_server.Test(); +} + +TEST(MachMessageServer, ReturnCodeNoReply) { + // This tests that when mig_reply_error_t::RetCode is set to MIG_NO_REPLY, no + // response is sent to the client. + TestMachMessageServer::Options options; + options.server_mig_retcode = MIG_NO_REPLY; + options.client_expect_reply = false; + TestMachMessageServer test_mach_message_server(options); test_mach_message_server.Test(); } @@ -492,9 +632,10 @@ TEST(MachMessageServer, ReplyPortNull) { // this and avoid sending a message to the null port. No reply message is // sent and the server returns success. TestMachMessageServer::Options options; - TestMachMessageServer test_mach_message_server(options); options.client_reply_port_type = TestMachMessageServer::Options::kReplyPortNull; + options.client_expect_reply = false; + TestMachMessageServer test_mach_message_server(options); test_mach_message_server.Test(); } @@ -506,11 +647,72 @@ TEST(MachMessageServer, ReplyPortDead) { // MACH_SEND_INVALID_DEST because it’s not possible to send a message to a // dead name. TestMachMessageServer::Options options; - TestMachMessageServer test_mach_message_server(options); options.parent_wait_for_child_pipe = true; options.expect_server_result = MACH_SEND_INVALID_DEST; options.client_reply_port_type = TestMachMessageServer::Options::kReplyPortDead; + options.client_expect_reply = false; + TestMachMessageServer test_mach_message_server(options); + test_mach_message_server.Test(); +} + +TEST(MachMessageServer, Complex) { + // The client allocates a new receive right and sends a complex request + // message to the server with a send right made out of this receive right. The + // server receives this message and is instructed to destroy the send right + // when it is done handling the request-reply transaction. The former send + // right is verified to be invalid after the server runs. This test ensures + // that resources transferred to a server process temporarily aren’t leaked. + TestMachMessageServer::Options options; + options.client_send_complex = true; + options.child_wait_for_parent_pipe = true; + TestMachMessageServer test_mach_message_server(options); + test_mach_message_server.Test(); +} + +TEST(MachMessageServer, ComplexNotDestroyed) { + // As in MachMessageServer.Complex, but the server is instructed not to + // destroy the send right. After the server runs, the send right is verified + // to continue to exist in the server task. The client process is then + // signalled by pipe that it’s safe to exit so that the send right in the + // server task doesn’t prematurely become a dead name. This test ensures that + // rights that are expected to be retained in the server task are properly + // retained. + TestMachMessageServer::Options options; + options.server_destroy_complex = false; + options.expect_server_destroyed_complex = false; + options.client_send_complex = true; + options.child_wait_for_parent_pipe = true; + TestMachMessageServer test_mach_message_server(options); + test_mach_message_server.Test(); +} + +TEST(MachMessageServer, ComplexDestroyedInvalidArgument) { + // As in MachMessageServer.ComplexNotDestroyed, but the server does not return + // a successful code via MIG. The server is expected to destroy resources in + // this case, because server_destroy_complex = false is only honored when a + // MIG request is handled successfully or with no reply. + TestMachMessageServer::Options options; + options.server_mig_retcode = KERN_INVALID_TASK; + options.server_destroy_complex = false; + options.client_send_complex = true; + options.child_wait_for_parent_pipe = true; + TestMachMessageServer test_mach_message_server(options); + test_mach_message_server.Test(); +} + +TEST(MachMessageServer, ComplexNotDestroyedNoReply) { + // As in MachMessageServer.ComplexNotDestroyed, but the server does not send + // a reply message and is expected to retain the send right in the server + // task. + TestMachMessageServer::Options options; + options.server_mig_retcode = MIG_NO_REPLY; + options.server_destroy_complex = false; + options.expect_server_destroyed_complex = false; + options.client_send_complex = true; + options.client_expect_reply = false; + options.child_wait_for_parent_pipe = true; + TestMachMessageServer test_mach_message_server(options); test_mach_message_server.Test(); }