Enhance the MachMessageServer test to cover port right ownership management

util/mach/mach_message_server_test.cc

Index: util/mach/mach_message_server_test.cc
diff --git a/util/mach/mach_message_server_test.cc b/util/mach/mach_message_server_test.cc
index dcbbae388d7d0c30df44ee1905feed75e27b2cc1..fe01cd1fd21f916985d75a7b548d65c7717b0cf0 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<ReceiveRequestMessage*>(in);
-    EXPECT_EQ(static_cast<mach_msg_bits_t>(
-        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_t>(MACH_PORT_NULL),
+                request->port_descriptor.name);
+      parent_complex_message_port_ = request->port_descriptor.name;
+      EXPECT_EQ(static_cast<mach_msg_type_name_t>(MACH_MSG_TYPE_MOVE_SEND),
+                request->port_descriptor.disposition);
+      EXPECT_EQ(
+          static_cast<mach_msg_descriptor_type_t>(MACH_MSG_PORT_DESCRIPTOR),
+          request->port_descriptor.type);
+    } else {
+      EXPECT_EQ(0u, request->body.msgh_descriptor_count);
+      EXPECT_EQ(static_cast<mach_port_t>(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_type_t>(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_t>(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;

[Robert Sesek, 2014/09/09 02:49:30]

This is where my eyes glazed over in the last review.

+  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();
 }