Do not create a reply IPCMessage for every message received.

This is incompatible with the way xpc_dictionary_create_reply() works, which
may only be called once per message. This would inhibit forwarding messages,
since the receiver of the forwarded XPC message would not be able to reply.
Instead, replies now must be created explicitly via MessageServer::CreateReply().

BUG=382931
R=mark@chromium.org

Review URL: https://codereview.chromium.org/398563002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@283376 0039d316-1c4b-4281-b951-d872f2087c98

5 files changed, 46 insertions, 35 deletions

diff --git a/sandbox/mac/launchd_interception_server.cc b/sandbox/mac/launchd_interception_server.cc
index 781977b..06f1081 100644
--- a/sandbox/mac/launchd_interception_server.cc
+++ b/sandbox/mac/launchd_interception_server.cc
@@ -52,8 +52,7 @@ bool LaunchdInterceptionServer::Initialize(mach_port_t server_receive_right) {
   return message_server_->Initialize();
 }
 
-void LaunchdInterceptionServer::DemuxMessage(IPCMessage request,
-                                             IPCMessage reply) {
+void LaunchdInterceptionServer::DemuxMessage(IPCMessage request) {
   const uint64_t message_id = compat_shim_.ipc_message_get_id(request);
   VLOG(3) << "Incoming message #" << message_id;
 
@@ -71,20 +70,19 @@ void LaunchdInterceptionServer::DemuxMessage(IPCMessage request,
   if (message_id == compat_shim_.msg_id_look_up2) {
     // Filter messages sent via bootstrap_look_up to enforce the sandbox policy
     // over the bootstrap namespace.
-    HandleLookUp(request, reply, policy);
+    HandleLookUp(request, policy);
   } else if (message_id == compat_shim_.msg_id_swap_integer) {
     // Ensure that any vproc_swap_integer requests are safe.
-    HandleSwapInteger(request, reply);
+    HandleSwapInteger(request);
   } else {
     // All other messages are not permitted.
     VLOG(1) << "Rejecting unhandled message #" << message_id;
-    message_server_->RejectMessage(reply, MIG_REMOTE_ERROR);
+    message_server_->RejectMessage(request, MIG_REMOTE_ERROR);
   }
 }
 
 void LaunchdInterceptionServer::HandleLookUp(
     IPCMessage request,
-    IPCMessage reply,
     const BootstrapSandboxPolicy* policy) {
   const std::string request_service_name(
       compat_shim_.look_up2_get_request_name(request));
@@ -108,7 +106,7 @@ void LaunchdInterceptionServer::HandleLookUp(
     // reply to the client. Returning a NULL or unserviced port for a look up
     // can cause clients to crash or hang.
     VLOG(1) << "Denying look_up2 with MIG error: " << request_service_name;
-    message_server_->RejectMessage(reply, BOOTSTRAP_UNKNOWN_SERVICE);
+    message_server_->RejectMessage(request, BOOTSTRAP_UNKNOWN_SERVICE);
   } else if (rule.result == POLICY_DENY_DUMMY_PORT ||
              rule.result == POLICY_SUBSTITUTE_PORT) {
     // The policy result is to deny access to the real service port, replying
@@ -123,6 +121,7 @@ void LaunchdInterceptionServer::HandleLookUp(
     else
       result_port = rule.substitute_port;
 
+    IPCMessage reply = message_server_->CreateReply(request);
     compat_shim_.look_up2_fill_reply(reply, result_port);
     // If the message was sent successfully, clear the result_port out of the
     // message so that it is not destroyed at the end of ReceiveMessage. The
@@ -135,8 +134,7 @@ void LaunchdInterceptionServer::HandleLookUp(
   }
 }
 
-void LaunchdInterceptionServer::HandleSwapInteger(IPCMessage request,
-                                                  IPCMessage reply) {
+void LaunchdInterceptionServer::HandleSwapInteger(IPCMessage request) {
   // Only allow getting information out of launchd. Do not allow setting
   // values. Two commonly observed values that are retrieved are
   // VPROC_GSK_MGR_PID and VPROC_GSK_TRANSACTIONS_ENABLED.
@@ -145,7 +143,7 @@ void LaunchdInterceptionServer::HandleSwapInteger(IPCMessage request,
     ForwardMessage(request);
   } else {
     VLOG(2) << "Rejecting non-read-only swap_integer message.";
-    message_server_->RejectMessage(reply, BOOTSTRAP_NOT_PRIVILEGED);
+    message_server_->RejectMessage(request, BOOTSTRAP_NOT_PRIVILEGED);
   }
 }
 void LaunchdInterceptionServer::ForwardMessage(IPCMessage request) {
diff --git a/sandbox/mac/launchd_interception_server.h b/sandbox/mac/launchd_interception_server.h
index 034cf13..4d5f9db 100644
--- a/sandbox/mac/launchd_interception_server.h
+++ b/sandbox/mac/launchd_interception_server.h
@@ -33,21 +33,19 @@ class LaunchdInterceptionServer : public MessageDemuxer {
   bool Initialize(mach_port_t server_receive_right);
 
   // MessageDemuxer:
-  virtual void DemuxMessage(IPCMessage request, IPCMessage reply) OVERRIDE;
+  virtual void DemuxMessage(IPCMessage request) OVERRIDE;
 
   mach_port_t server_port() const { return message_server_->GetServerPort(); }
 
  private:
   // Given a look_up2 request message, this looks up the appropriate sandbox
   // policy for the service name then formulates and sends the reply message.
-  void HandleLookUp(IPCMessage request,
-                    IPCMessage reply,
-                    const BootstrapSandboxPolicy* policy);
+  void HandleLookUp(IPCMessage request, const BootstrapSandboxPolicy* policy);
 
   // Given a swap_integer request message, this verifies that it is safe, and
   // if so, forwards it on to launchd for servicing. If the request is unsafe,
   // it replies with an error.
-  void HandleSwapInteger(IPCMessage request, IPCMessage reply);
+  void HandleSwapInteger(IPCMessage request);
 
   // Forwards the original |request| on to real bootstrap server for handling.
   void ForwardMessage(IPCMessage request);
diff --git a/sandbox/mac/mach_message_server.cc b/sandbox/mac/mach_message_server.cc
index fd16b6d..cbd9916 100644
--- a/sandbox/mac/mach_message_server.cc
+++ b/sandbox/mac/mach_message_server.cc
@@ -108,6 +108,25 @@ pid_t MachMessageServer::GetMessageSenderPID(IPCMessage request) {
   return sender_pid;
 }
 
+IPCMessage MachMessageServer::CreateReply(IPCMessage request_message) {
+  mach_msg_header_t* request = request_message.mach;
+
+  IPCMessage reply_message;
+  mach_msg_header_t* reply = reply_message.mach =
+      reinterpret_cast<mach_msg_header_t*>(reply_buffer_.address());
+  bzero(reply, buffer_size_);
+
+  reply->msgh_bits = MACH_MSGH_BITS_REMOTE(reply->msgh_bits);
+  // Since mach_msg will automatically swap the request and reply ports,
+  // undo that.
+  reply->msgh_remote_port = request->msgh_remote_port;
+  reply->msgh_local_port = MACH_PORT_NULL;
+  // MIG servers simply add 100 to the request ID to generate the reply ID.
+  reply->msgh_id = request->msgh_id + 100;
+
+  return reply_message;
+}
+
 bool MachMessageServer::SendReply(IPCMessage reply) {
   kern_return_t kr = mach_msg(reply.mach, MACH_SEND_MSG,
       reply.mach->msgh_size, 0, MACH_PORT_NULL, MACH_MSG_TIMEOUT_NONE,
@@ -133,7 +152,8 @@ void MachMessageServer::ForwardMessage(IPCMessage message,
   }
 }
 
-void MachMessageServer::RejectMessage(IPCMessage reply, int error_code) {
+void MachMessageServer::RejectMessage(IPCMessage request, int error_code) {
+  IPCMessage reply = CreateReply(request);
   mig_reply_error_t* error_reply =
       reinterpret_cast<mig_reply_error_t*>(reply.mach);
   error_reply->Head.msgh_size = sizeof(mig_reply_error_t);
@@ -173,19 +193,9 @@ void MachMessageServer::ReceiveMessage() {
     return;
   }
 
-  // Set up a reply message in case it will be used.
-  reply->msgh_bits = MACH_MSGH_BITS_REMOTE(reply->msgh_bits);
-  // Since mach_msg will automatically swap the request and reply ports,
-  // undo that.
-  reply->msgh_remote_port = request->msgh_remote_port;
-  reply->msgh_local_port = MACH_PORT_NULL;
-  // MIG servers simply add 100 to the request ID to generate the reply ID.
-  reply->msgh_id = request->msgh_id + 100;
-
   // Process the message.
   IPCMessage request_message = { request };
-  IPCMessage reply_message = { reply };
-  demuxer_->DemuxMessage(request_message, reply_message);
+  demuxer_->DemuxMessage(request_message);
 
   // Free any descriptors in the message body. If the message was forwarded,
   // any descriptors would have been moved out of the process on send. If the
diff --git a/sandbox/mac/mach_message_server.h b/sandbox/mac/mach_message_server.h
index eb70822..ba0b2c4 100644
--- a/sandbox/mac/mach_message_server.h
+++ b/sandbox/mac/mach_message_server.h
@@ -33,12 +33,13 @@ class MachMessageServer : public MessageServer {
   // MessageServer:
   virtual bool Initialize() OVERRIDE;
   virtual pid_t GetMessageSenderPID(IPCMessage request) OVERRIDE;
+  virtual IPCMessage CreateReply(IPCMessage request) OVERRIDE;
   virtual bool SendReply(IPCMessage reply) OVERRIDE;
   virtual void ForwardMessage(IPCMessage request,
                               mach_port_t destination) OVERRIDE;
   // Replies to the message with the specified |error_code| as a MIG
   // error_reply RetCode.
-  virtual void RejectMessage(IPCMessage reply, int error_code) OVERRIDE;
+  virtual void RejectMessage(IPCMessage request, int error_code) OVERRIDE;
   virtual mach_port_t GetServerPort() const OVERRIDE;
 
  private:
diff --git a/sandbox/mac/message_server.h b/sandbox/mac/message_server.h
index 2a56fd5..1cd40b0 100644
--- a/sandbox/mac/message_server.h
+++ b/sandbox/mac/message_server.h
@@ -24,10 +24,9 @@ union IPCMessage {
 // incoming intercepted IPC messages.
 class MessageDemuxer {
  public:
-  // Handle a |request| message and optionally create a |reply|. Both message
-  // objects are owned by the server. Use the server's methods to send a
-  // reply message.
-  virtual void DemuxMessage(IPCMessage request, IPCMessage reply) = 0;
+  // Handle a |request| message. The message is owned by the server. Use the
+  // server's methods to create and send a reply message.
+  virtual void DemuxMessage(IPCMessage request) = 0;
 
  protected:
   virtual ~MessageDemuxer() {}
@@ -48,15 +47,20 @@ class MessageServer {
   // Given a received request message, returns the PID of the sending process.
   virtual pid_t GetMessageSenderPID(IPCMessage request) = 0;
 
+  // Creates a reply message from a request message. The result is owned by
+  // the server.
+  virtual IPCMessage CreateReply(IPCMessage request) = 0;
+
   // Sends a reply message. Returns true if the message was sent successfully.
   virtual bool SendReply(IPCMessage reply) = 0;
 
   // Forwards the original |request| to the |destination| for handling.
   virtual void ForwardMessage(IPCMessage request, mach_port_t destination) = 0;
 
-  // Replies to the message with the specified |error_code| in an error field
-  // that is interpreted by the underlying IPC system.
-  virtual void RejectMessage(IPCMessage reply, int error_code) = 0;
+  // Replies to the received |request| message by creating a reply and setting
+  // the specified |error_code| in a field that is interpreted by the
+  // underlying IPC system.
+  virtual void RejectMessage(IPCMessage request, int error_code) = 0;
 
   // Returns the Mach port on which the MessageServer is listening.
   virtual mach_port_t GetServerPort() const = 0;