Eliminate a potential race in IPC::ChannelProxy

Doing the following steps with ChannelProxy leads to a data race:
1) Create the ChannelProxy, but don't initialize it.
2) Add a filter.
3) Init the ChannelProxy.

The problem is, AddFilter() posts a task from the Listener thread to the IPC task runner to do OnAddFilter. Prior to this patch, OnAddFilter will try to read channel_ even though channel_ may not have been initialized, and it's accessed without any synchronization.

This patch only really adds the filter if peer_pid_ has been set on the IPC::Channel thread; otherwise, it waits until the connection has been established to really add filters.

See the bug for more detail.

BUG=244383

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=256188

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

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

1 files changed, 17 insertions, 4 deletions

diff --git a/ipc/ipc_channel_nacl.cc b/ipc/ipc_channel_nacl.cc
index 1ecd571..f796af9 100644
--- a/ipc/ipc_channel_nacl.cc
+++ b/ipc/ipc_channel_nacl.cc
@@ -17,6 +17,7 @@
 #include "base/task_runner_util.h"
 #include "base/threading/simple_thread.h"
 #include "ipc/file_descriptor_set_posix.h"
+#include "ipc/ipc_listener.h"
 #include "ipc/ipc_logging.h"
 #include "native_client/src/public/imc_syscalls.h"
 #include "native_client/src/public/imc_types.h"
@@ -138,9 +139,15 @@ Channel::ChannelImpl::~ChannelImpl() {
   Close();
 }
 
+base::ProcessId Channel::ChannelImpl::peer_pid() const {
+  // This shouldn't actually get used in the untrusted side of the proxy, and we
+  // don't have the real pid anyway.
+  return -1;
+}
+
 bool Channel::ChannelImpl::Connect() {
   if (pipe_ == -1) {
-    DLOG(INFO) << "Channel creation failed: " << pipe_name_;
+    DLOG(WARNING) << "Channel creation failed: " << pipe_name_;
     return false;
   }
 
@@ -164,6 +171,10 @@ bool Channel::ChannelImpl::Connect() {
   waiting_connect_ = false;
   // If there were any messages queued before connection, send them.
   ProcessOutgoingMessages();
+  base::MessageLoopProxy::current()->PostTask(FROM_HERE,
+      base::Bind(&Channel::ChannelImpl::CallOnChannelConnected,
+                 weak_ptr_factory_.GetWeakPtr()));
+
   return true;
 }
 
@@ -293,6 +304,10 @@ bool Channel::ChannelImpl::ProcessOutgoingMessages() {
   return true;
 }
 
+void Channel::ChannelImpl::CallOnChannelConnected() {
+  listener()->OnChannelConnected(peer_pid());
+}
+
 Channel::ChannelImpl::ReadState Channel::ChannelImpl::ReadData(
     char* buffer,
     int buffer_len,
@@ -372,9 +387,7 @@ void Channel::Close() {
 }
 
 base::ProcessId Channel::peer_pid() const {
-  // This shouldn't actually get used in the untrusted side of the proxy, and we
-  // don't have the real pid anyway.
-  return -1;
+  return channel_impl_->peer_pid();
 }
 
 bool Channel::Send(Message* message) {