Eliminate a potential race in IPC::ChannelProxy

ipc/ipc_channel_proxy.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "base/bind.h"
 #include "base/compiler_specific.h"
 #include "base/debug/trace_event.h"
 #include "base/location.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
@@ skipping 138 matching lines @@
 
 ChannelProxy::Context::~Context() {
 }
 
 void ChannelProxy::Context::ClearIPCTaskRunner() {
   ipc_task_runner_ = NULL;
 }
 
 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));
 }
 
 bool ChannelProxy::Context::TryFilters(const Message& message) {
   DCHECK(message_filter_router_);
 #ifdef IPC_MESSAGE_LOG_ENABLED
   Logging* logger = Logging::GetInstance();
   if (logger->Enabled())
     logger->OnPreDispatchMessage(message);
@@ skipping 19 matching lines @@
 
 // Called on the IPC::Channel thread
 bool ChannelProxy::Context::OnMessageReceivedNoFilter(const Message& message) {
   listener_task_runner_->PostTask(
       FROM_HERE, base::Bind(&Context::OnDispatchMessage, this, message));
   return true;
 }
 
 // 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));
 }
 
 // Called on the IPC::Channel thread
 void ChannelProxy::Context::OnChannelError() {
   for (size_t i = 0; i < filters_.size(); ++i)
     filters_[i]->OnChannelError();
 
@@ skipping 16 matching lines @@
   }
 
   for (size_t i = 0; i < filters_.size(); ++i)
     filters_[i]->OnFilterAdded(channel_.get());
 }
 
 // Called on the IPC::Channel thread
 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) {
     filters_[i]->OnChannelClosing();
     filters_[i]->OnFilterRemoved();
   }
 
   // We don't need the filters anymore.
   message_filter_router_->Clear();
   filters_.clear();
 
   channel_.reset();
 
   // Balance with the reference taken during startup.  This may result in
   // self-destruction.
   Release();
 }
 
 void ChannelProxy::Context::Clear() {
   listener_ = NULL;
 }
 
 // Called on the IPC::Channel thread
 void ChannelProxy::Context::OnSendMessage(scoped_ptr<Message> message) {
-  if (!channel_.get()) {
-    OnChannelClosed();
+  if (!channel_)
     return;

[dmichael (off chromium), 2014/02/28 23:25:47]

There's no need to call OnChannelClosed; it's already closed, since that's the
only place that we reset channel_. (It also bails immediately if channel_ is
NULL). So I thought this was a little easier to understand.

-  }
+
   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_);
     new_filters.swap(pending_filters_);
   }
+  if (!channel_)
+    return;  // The channel has been closed, so don't really add the filters.

[jam, 2014/03/03 16:27:03]

can this condition really get hit since we're only getting here through
OnChannelConnected now?

[dmichael (off chromium), 2014/03/03 16:37:02]

Good point, looks like it can't happen now.

I'll remove that.

 
   for (size_t i = 0; i < new_filters.size(); ++i) {
     filters_.push_back(new_filters[i]);
 
     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);
 
   for (size_t i = 0; i < filters_.size(); ++i) {
     if (filters_[i].get() == filter) {
       filter->OnFilterRemoved();
       filters_.erase(filters_.begin() + i);
       return;
     }
@@ skipping 197 matching lines @@
   Channel* channel = context_.get()->channel_.get();
   // Channel must have been created first.
   DCHECK(channel) << context_.get()->channel_id_;
   return channel->GetPeerEuid(peer_euid);
 }
 #endif
 
 //-----------------------------------------------------------------------------
 
 }  // namespace IPC