Mojo: Make it not CHECK-fail on receiving a bogus channel control message.

R=tsepez@chromium.org

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

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

5 files changed, 32 insertions, 15 deletions

diff --git a/mojo/system/channel.cc b/mojo/system/channel.cc
index 89035b3..770275a 100644
--- a/mojo/system/channel.cc
+++ b/mojo/system/channel.cc
@@ -102,7 +102,7 @@ MessageInTransit::EndpointId Channel::AttachMessagePipeEndpoint(
   return local_id;
 }
 
-void Channel::RunMessagePipeEndpoint(MessageInTransit::EndpointId local_id,
+bool Channel::RunMessagePipeEndpoint(MessageInTransit::EndpointId local_id,
                                      MessageInTransit::EndpointId remote_id) {
   EndpointInfo endpoint_info;
   {
@@ -110,15 +110,15 @@ void Channel::RunMessagePipeEndpoint(MessageInTransit::EndpointId local_id,
 
     IdToEndpointInfoMap::const_iterator it =
         local_id_to_endpoint_info_map_.find(local_id);
-    // TODO(vtl): FIXME -- This check is wrong if this is in response to a
-    // |kSubtypeChannelRunMessagePipeEndpoint| message. We should report error.
-    CHECK(it != local_id_to_endpoint_info_map_.end());
+    if (it == local_id_to_endpoint_info_map_.end())
+      return false;
     endpoint_info = it->second;
   }
 
   // TODO(vtl): FIXME -- We need to handle the case that message pipe is already
   // running when we're here due to |kSubtypeChannelRunMessagePipeEndpoint|).
   endpoint_info.message_pipe->Run(endpoint_info.port, remote_id);
+  return true;
 }
 
 void Channel::RunRemoteMessagePipeEndpoint(
@@ -251,12 +251,15 @@ void Channel::OnReadMessageForDownstream(
 
   // We need to duplicate the message, because |EnqueueMessage()| will take
   // ownership of it.
-  // TODO(vtl): Need to enforce limits on message size and handle count.
   scoped_ptr<MessageInTransit> message(new MessageInTransit(message_view));
   message->DeserializeDispatchers(this);
   MojoResult result = endpoint_info.message_pipe->EnqueueMessage(
       MessagePipe::GetPeerPort(endpoint_info.port), message.Pass(), NULL);
   if (result != MOJO_RESULT_OK) {
+    // TODO(vtl): This might be a "non-error", e.g., if the destination endpoint
+    // has been closed (in an unavoidable race). This might also be a "remote"
+    // error, e.g., if the remote side is sending invalid control messages (to
+    // the message pipe).
     HandleLocalError(base::StringPrintf(
         "Failed to enqueue message to local destination ID %u (result %d)",
         static_cast<unsigned>(local_id), static_cast<int>(result)));
@@ -275,8 +278,9 @@ void Channel::OnReadMessageForChannel(
       DVLOG(2) << "Handling channel message to run message pipe (local ID = "
                << message_view.destination_id() << ", remote ID = "
                << message_view.source_id() << ")";
-      RunMessagePipeEndpoint(message_view.destination_id(),
-                             message_view.source_id());
+      if (!RunMessagePipeEndpoint(message_view.destination_id(),
+                                  message_view.source_id()))
+        HandleRemoteError("Received invalid channel run message pipe message");
       break;
     default:
       HandleRemoteError("Received invalid channel message");
diff --git a/mojo/system/channel.h b/mojo/system/channel.h
index fe876be..1e66a88 100644
--- a/mojo/system/channel.h
+++ b/mojo/system/channel.h
@@ -73,9 +73,17 @@ class MOJO_SYSTEM_IMPL_EXPORT Channel
   // |kBootstrapEndpointId| as its local ID. (For bootstrapping, this occurs on
   // both sides, so one should use |kBootstrapEndpointId| for the remote ID for
   // the first message pipe across a channel.)
+  // TODO(vtl): Maybe limit the number of attached message pipes and allow this
+  // to fail.
   MessageInTransit::EndpointId AttachMessagePipeEndpoint(
       scoped_refptr<MessagePipe> message_pipe, unsigned port);
-  void RunMessagePipeEndpoint(MessageInTransit::EndpointId local_id,
+
+  // Runs the message pipe with the given |local_id| (previously attached), with
+  // the given |remote_id| (negotiated using some other means, e.g., over an
+  // existing message pipe; see comments above for the bootstrap case). Returns
+  // false on failure, in particular if no message pipe with |local_id| is
+  // attached.
+  bool RunMessagePipeEndpoint(MessageInTransit::EndpointId local_id,
                               MessageInTransit::EndpointId remote_id);
 
   // Tells the other side of the channel to run a message pipe endpoint (which
diff --git a/mojo/system/message_pipe_dispatcher.cc b/mojo/system/message_pipe_dispatcher.cc
index 3559131..364259b 100644
--- a/mojo/system/message_pipe_dispatcher.cc
+++ b/mojo/system/message_pipe_dispatcher.cc
@@ -78,7 +78,12 @@ scoped_refptr<MessagePipeDispatcher> MessagePipeDispatcher::Deserialize(
   DVLOG(2) << "Deserializing message pipe dispatcher (remote ID = "
            << remote_id << ", new local ID = " << local_id << ")";
 
-  channel->RunMessagePipeEndpoint(local_id, remote_id);
+  if (!channel->RunMessagePipeEndpoint(local_id, remote_id)) {
+    // In general, this shouldn't fail, since we generated |local_id| locally.
+    NOTREACHED();
+    return scoped_refptr<MessagePipeDispatcher>();
+  }
+
   // TODO(vtl): FIXME -- Need some error handling here.
   channel->RunRemoteMessagePipeEndpoint(local_id, remote_id);
   return remote_message_pipe.first;
diff --git a/mojo/system/multiprocess_message_pipe_unittest.cc b/mojo/system/multiprocess_message_pipe_unittest.cc
index fe1c042..458fe77 100644
--- a/mojo/system/multiprocess_message_pipe_unittest.cc
+++ b/mojo/system/multiprocess_message_pipe_unittest.cc
@@ -78,8 +78,8 @@ class ChannelThread {
     // message pipe endpoint is attached.
     CHECK_EQ(channel_->AttachMessagePipeEndpoint(message_pipe, 1),
              Channel::kBootstrapEndpointId);
-    channel_->RunMessagePipeEndpoint(Channel::kBootstrapEndpointId,
-                                     Channel::kBootstrapEndpointId);
+    CHECK(channel_->RunMessagePipeEndpoint(Channel::kBootstrapEndpointId,
+                                           Channel::kBootstrapEndpointId));
   }
 
   void ShutdownChannelOnIOThread() {
diff --git a/mojo/system/remote_message_pipe_unittest.cc b/mojo/system/remote_message_pipe_unittest.cc
index 45d757a..4dee577 100644
--- a/mojo/system/remote_message_pipe_unittest.cc
+++ b/mojo/system/remote_message_pipe_unittest.cc
@@ -127,8 +127,8 @@ class RemoteMessagePipeTest : public testing::Test {
     MessageInTransit::EndpointId local_id1 =
         channels_[1]->AttachMessagePipeEndpoint(mp1, 0);
 
-    channels_[0]->RunMessagePipeEndpoint(local_id0, local_id1);
-    channels_[1]->RunMessagePipeEndpoint(local_id1, local_id0);
+    CHECK(channels_[0]->RunMessagePipeEndpoint(local_id0, local_id1));
+    CHECK(channels_[1]->RunMessagePipeEndpoint(local_id1, local_id0));
   }
 
   void BootstrapMessagePipeOnIOThread(unsigned channel_index,
@@ -142,8 +142,8 @@ class RemoteMessagePipeTest : public testing::Test {
     CreateAndInitChannel(channel_index);
     CHECK_EQ(channels_[channel_index]->AttachMessagePipeEndpoint(mp, port),
              Channel::kBootstrapEndpointId);
-    channels_[channel_index]->RunMessagePipeEndpoint(
-        Channel::kBootstrapEndpointId, Channel::kBootstrapEndpointId);
+    CHECK(channels_[channel_index]->RunMessagePipeEndpoint(
+        Channel::kBootstrapEndpointId, Channel::kBootstrapEndpointId));
   }
 
   void RestoreInitialStateOnIOThread() {