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

4 files changed, 57 insertions, 20 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) {
diff --git a/ipc/ipc_channel_nacl.h b/ipc/ipc_channel_nacl.h
index a21730e..29a9d57 100644
--- a/ipc/ipc_channel_nacl.h
+++ b/ipc/ipc_channel_nacl.h
@@ -40,6 +40,7 @@ class Channel::ChannelImpl : public internal::ChannelReader {
   virtual ~ChannelImpl();
 
   // Channel implementation.
+  base::ProcessId peer_pid() const;
   bool Connect();
   void Close();
   bool Send(Message* message);
@@ -53,6 +54,7 @@ class Channel::ChannelImpl : public internal::ChannelReader {
 
   bool CreatePipe(const IPC::ChannelHandle& channel_handle);
   bool ProcessOutgoingMessages();
+  void CallOnChannelConnected();
 
   // ChannelReader implementation.
   virtual ReadState ReadData(char* buffer,
diff --git a/ipc/ipc_channel_proxy.cc b/ipc/ipc_channel_proxy.cc
index 50431d5..ca09146 100644
--- a/ipc/ipc_channel_proxy.cc
+++ b/ipc/ipc_channel_proxy.cc
@@ -156,7 +156,7 @@ void ChannelProxy::Context::ClearIPCTaskRunner() {
 
 void ChannelProxy::Context::CreateChannel(const IPC::ChannelHandle& handle,
                                           const Channel::Mode& mode) {
-  DCHECK(channel_.get() == NULL);
+  DCHECK(!channel_);
   channel_id_ = handle.name;
   channel_.reset(new Channel(handle, mode, this));
 }
@@ -196,17 +196,15 @@ bool ChannelProxy::Context::OnMessageReceivedNoFilter(const Message& message) {
 
 // Called on the IPC::Channel thread
 void ChannelProxy::Context::OnChannelConnected(int32 peer_pid) {
+  // We cache off the peer_pid so it can be safely accessed from both threads.
+  peer_pid_ = channel_->peer_pid();
+
   // Add any pending filters.  This avoids a race condition where someone
   // creates a ChannelProxy, calls AddFilter, and then right after starts the
   // peer process.  The IO thread could receive a message before the task to add
   // the filter is run on the IO thread.
   OnAddFilter();
 
-  // We cache off the peer_pid so it can be safely accessed from both threads.
-  peer_pid_ = channel_->peer_pid();
-  for (size_t i = 0; i < filters_.size(); ++i)
-    filters_[i]->OnChannelConnected(peer_pid);
-
   // See above comment about using listener_task_runner_ here.
   listener_task_runner_->PostTask(
       FROM_HERE, base::Bind(&Context::OnDispatchConnected, this));
@@ -243,7 +241,7 @@ void ChannelProxy::Context::OnChannelOpened() {
 void ChannelProxy::Context::OnChannelClosed() {
   // It's okay for IPC::ChannelProxy::Close to be called more than once, which
   // would result in this branch being taken.
-  if (!channel_.get())
+  if (!channel_)
     return;
 
   for (size_t i = 0; i < filters_.size(); ++i) {
@@ -254,6 +252,9 @@ void ChannelProxy::Context::OnChannelClosed() {
   // We don't need the filters anymore.
   message_filter_router_->Clear();
   filters_.clear();
+  // We don't need the lock, because at this point, the listener thread can't
+  // access it any more.
+  pending_filters_.clear();
 
   channel_.reset();
 
@@ -268,16 +269,24 @@ void ChannelProxy::Context::Clear() {
 
 // Called on the IPC::Channel thread
 void ChannelProxy::Context::OnSendMessage(scoped_ptr<Message> message) {
-  if (!channel_.get()) {
+  if (!channel_) {
     OnChannelClosed();
     return;
   }
+
   if (!channel_->Send(message.release()))
     OnChannelError();
 }
 
 // Called on the IPC::Channel thread
 void ChannelProxy::Context::OnAddFilter() {
+  // Our OnChannelConnected method has not yet been called, so we can't be
+  // sure that channel_ is valid yet. When OnChannelConnected *is* called,
+  // it invokes OnAddFilter, so any pending filter(s) will be added at that
+  // time.
+  if (peer_pid_ == base::kNullProcessId)
+    return;
+
   std::vector<scoped_refptr<MessageFilter> > new_filters;
   {
     base::AutoLock auto_lock(pending_filters_lock_);
@@ -289,19 +298,28 @@ void ChannelProxy::Context::OnAddFilter() {
 
     message_filter_router_->AddFilter(new_filters[i].get());
 
-    // If the channel has already been created, then we need to send this
-    // message so that the filter gets access to the Channel.
-    if (channel_.get())
-      new_filters[i]->OnFilterAdded(channel_.get());
-    // Ditto for if the channel has been connected.
-    if (peer_pid_)
-      new_filters[i]->OnChannelConnected(peer_pid_);
+    // The channel has already been created and connected, so we need to
+    // inform the filters right now.
+    new_filters[i]->OnFilterAdded(channel_.get());
+    new_filters[i]->OnChannelConnected(peer_pid_);
   }
 }
 
 // Called on the IPC::Channel thread
 void ChannelProxy::Context::OnRemoveFilter(MessageFilter* filter) {
-  if (!channel_.get())
+  if (peer_pid_ == base::kNullProcessId) {
+    // The channel is not yet connected, so any filters are still pending.
+    base::AutoLock auto_lock(pending_filters_lock_);
+    for (size_t i = 0; i < pending_filters_.size(); ++i) {
+      if (pending_filters_[i].get() == filter) {
+        filter->OnFilterRemoved();
+        pending_filters_.erase(pending_filters_.begin() + i);
+        return;
+      }
+    }
+    return;
+  }
+  if (!channel_)
     return;  // The filters have already been deleted.
 
   message_filter_router_->RemoveFilter(filter);
diff --git a/ipc/ipc_channel_proxy.h b/ipc/ipc_channel_proxy.h
index dca8c97..b0247c4 100644
--- a/ipc/ipc_channel_proxy.h
+++ b/ipc/ipc_channel_proxy.h
@@ -233,6 +233,10 @@ class IPC_EXPORT ChannelProxy : public Sender, public base::NonThreadSafe {
     // List of filters.  This is only accessed on the IPC thread.
     std::vector<scoped_refptr<MessageFilter> > filters_;
     scoped_refptr<base::SingleThreadTaskRunner> ipc_task_runner_;
+
+    // Note, channel_ may be set on the Listener thread or the IPC thread.
+    // But once it has been set, it must only be read or cleared on the IPC
+    // thread.
     scoped_ptr<Channel> channel_;
     std::string channel_id_;
     bool channel_connected_called_;