IPC: Fix attachment brokering race condition.

A channel must be registered as a broker communication channel before it is
connected. When possible, invert the sequence of the call to connect a channel,
and the call to register the channel as a broker. In some cases, the channel
constructor and the channel initializer had to be separated, so that the
registration could happen in between.

This requirement is now enforced by a CHECK, which verifies that a channel
cannot be registered after it is connected.

BUG=598088

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

Cr-Commit-Position: refs/heads/master@{#389618}

components/nacl/loader/nacl_listener.cc

@@ -225,10 +225,10 @@ void NaClListener::Listen() {
                                       &shutdown_event_);
   filter_ = channel_->CreateSyncMessageFilter();
   channel_->AddFilter(new FileTokenMessageFilter());
-  channel_->Init(channel_name, IPC::Channel::MODE_CLIENT, true);
   IPC::AttachmentBroker* global = IPC::AttachmentBroker::GetGlobal();
   if (global && !global->IsPrivilegedBroker())
     global->RegisterBrokerCommunicationChannel(channel_.get());
+  channel_->Init(channel_name, IPC::Channel::MODE_CLIENT, true);
   main_loop_ = base::MessageLoop::current();
   main_loop_->Run();
 }

content/browser/renderer_host/render_process_host_impl.cc

@@ -723,11 +723,6 @@ bool RenderProcessHostImpl::Init() {
   const std::string channel_id =
       IPC::Channel::GenerateVerifiedChannelID(std::string());
   channel_ = CreateChannelProxy(channel_id);
-#if USE_ATTACHMENT_BROKER
-  IPC::AttachmentBroker::GetGlobal()->RegisterCommunicationChannel(
-      channel_.get(), content::BrowserThread::GetMessageLoopProxyForThread(
-      content::BrowserThread::IO));
-#endif
 
   // Call the embedder first so that their IPC filters have priority.
   GetContentClient()->browser()->RenderProcessWillLaunch(this);
@@ -823,9 +818,15 @@ std::unique_ptr<IPC::ChannelProxy> RenderProcessHostImpl::CreateChannelProxy(
     }
 #endif  // OS_ANDROID
 
-    return IPC::ChannelProxy::Create(
-        IPC::ChannelMojo::CreateServerFactory(std::move(handle)), this,
-        runner.get());
+  std::unique_ptr<IPC::ChannelProxy> channel(
+      new IPC::ChannelProxy(this, runner.get()));
+#if USE_ATTACHMENT_BROKER
+  IPC::AttachmentBroker::GetGlobal()->RegisterCommunicationChannel(
+      channel.get(), content::BrowserThread::GetMessageLoopProxyForThread(
+      content::BrowserThread::IO));
+#endif
+  channel->Init(IPC::ChannelMojo::CreateServerFactory(std::move(handle)), true);
+  return channel;
   }
 
     // Do NOT expand ifdef or run time condition checks here! See comment above.
@@ -836,8 +837,15 @@ std::unique_ptr<IPC::ChannelProxy> RenderProcessHostImpl::CreateChannelProxy(
   }
 #endif  // OS_ANDROID
 
-  return IPC::ChannelProxy::Create(channel_id, IPC::Channel::MODE_SERVER, this,
-                                   runner.get());
+  std::unique_ptr<IPC::ChannelProxy> channel(
+      new IPC::ChannelProxy(this, runner.get()));
+#if USE_ATTACHMENT_BROKER
+  IPC::AttachmentBroker::GetGlobal()->RegisterCommunicationChannel(
+      channel.get(), content::BrowserThread::GetMessageLoopProxyForThread(
+      content::BrowserThread::IO));
+#endif
+  channel->Init(channel_id, IPC::Channel::MODE_SERVER, true);
+  return channel;
 }
 
 void RenderProcessHostImpl::CreateMessageFilters() {

content/child/child_thread_impl.cc

@@ -472,10 +472,10 @@ void ChildThreadImpl::Init(const Options& options) {
     channel_->AddFilter(startup_filter);
   }
 
-  ConnectChannel(options.use_mojo_channel, options.in_process_ipc_token);
   IPC::AttachmentBroker* broker = IPC::AttachmentBroker::GetGlobal();
   if (broker && !broker->IsPrivilegedBroker())
     broker->RegisterBrokerCommunicationChannel(channel_.get());
+  ConnectChannel(options.use_mojo_channel, options.in_process_ipc_token);
 
   int connection_timeout = kConnectionTimeoutS;
   std::string connection_override =

content/common/child_process_host_impl.cc

@@ -133,12 +133,17 @@ void ChildProcessHostImpl::ForceShutdown() {
 std::string ChildProcessHostImpl::CreateChannel() {
   channel_id_ = IPC::Channel::GenerateVerifiedChannelID(std::string());
   channel_ = IPC::Channel::CreateServer(channel_id_, this);
-  if (!channel_->Connect())
-    return std::string();
 #if USE_ATTACHMENT_BROKER
   IPC::AttachmentBroker::GetGlobal()->RegisterCommunicationChannel(
       channel_.get(), base::MessageLoopForIO::current()->task_runner());
 #endif
+  if (!channel_->Connect()) {
+#if USE_ATTACHMENT_BROKER
+    IPC::AttachmentBroker::GetGlobal()->DeregisterCommunicationChannel(
+        channel_.get());
+#endif
+    return std::string();
+  }
 
   for (size_t i = 0; i < filters_.size(); ++i)
     filters_[i]->OnFilterAdded(channel_.get());

ipc/attachment_broker_mac_unittest.cc

@@ -423,6 +423,12 @@ class IPCAttachmentBrokerMacTest : public IPCTestBase {
 
   // Setup shared between tests.
   void CommonSetUp(const char* name) {
+    PreConnectSetUp(name);
+    PostConnectSetUp();
+  }
+
+  // All of setup before the channel is connected.
+  void PreConnectSetUp(const char* name) {
     Init(name);
     MachPreForkSetUp();
 
@@ -432,6 +438,10 @@ class IPCAttachmentBrokerMacTest : public IPCTestBase {
     broker_->AddObserver(&observer_, task_runner());
     CreateChannel(&proxy_listener_);
     broker_->RegisterBrokerCommunicationChannel(channel());
+  }
+
+  // All of setup including the connection and everything after.
+  void PostConnectSetUp() {
     ASSERT_TRUE(ConnectChannel());
     ASSERT_TRUE(StartClient());
 
@@ -871,11 +881,11 @@ MULTIPROCESS_IPC_TEST_CLIENT_MAIN(SendPosixFDAndMachPort) {
 // process sending an attachment to another unprivileged process.
 TEST_F(IPCAttachmentBrokerMacTest, SendSharedMemoryHandleToSelf) {
   SetBroker(new MockBroker);
-  CommonSetUp("SendSharedMemoryHandleToSelf");
-
+  PreConnectSetUp("SendSharedMemoryHandleToSelf");
   // Technically, the channel is an endpoint, but we need the proxy listener to
   // receive the messages so that it can quit the message loop.
   channel()->SetAttachmentBrokerEndpoint(false);
+  PostConnectSetUp();
   get_proxy_listener()->set_listener(get_broker());
 
   {
@@ -940,8 +950,12 @@ TEST_F(IPCAttachmentBrokerMacTest, SendSharedMemoryHandleChannelProxy) {
   options.message_loop_type = base::MessageLoop::TYPE_IO;
   thread->StartWithOptions(options);
 
-  CreateChannelProxy(get_proxy_listener(), thread->task_runner().get());
+  set_channel_proxy(std::unique_ptr<IPC::ChannelProxy>(new IPC::ChannelProxy(
+      get_proxy_listener(), thread->task_runner().get())));
   get_broker()->RegisterBrokerCommunicationChannel(channel_proxy());
+  channel_proxy()->Init(
+      CreateChannelFactory(GetTestChannelHandle(), thread->task_runner().get()),
+      true);
 
   ASSERT_TRUE(StartClient());
 
@@ -1060,11 +1074,11 @@ MULTIPROCESS_IPC_TEST_CLIENT_MAIN(ShareReadOnlyToProcess) {
 // not have the task port for the parent process.
 TEST_F(IPCAttachmentBrokerMacTest, SendSharedMemoryHandleToSelfDelayedPort) {
   SetBroker(new MockBroker);
-  CommonSetUp("SendSharedMemoryHandleToSelfDelayedPort");
-
+  PreConnectSetUp("SendSharedMemoryHandleToSelfDelayedPort");
   // Technically, the channel is an endpoint, but we need the proxy listener to
   // receive the messages so that it can quit the message loop.
   channel()->SetAttachmentBrokerEndpoint(false);
+  PostConnectSetUp();
   get_proxy_listener()->set_listener(get_broker());
 
   {

ipc/attachment_broker_privileged_win_unittest.cc

@@ -265,11 +265,21 @@ class IPCAttachmentBrokerPrivilegedWinTest : public IPCTestBase {
   }
 
   void CommonSetUp() {
+    PreConnectSetUp();
+    PostConnectSetUp();
+  }
+
+  // All of setup before the channel is connected.
+  void PreConnectSetUp() {
     if (!broker_.get())
       set_broker(new IPC::AttachmentBrokerUnprivilegedWin);
     broker_->AddObserver(&observer_, task_runner());
     CreateChannel(&proxy_listener_);
     broker_->RegisterBrokerCommunicationChannel(channel());
+  }
+
+  // All of setup including the connection and everything after.
+  void PostConnectSetUp() {
     ASSERT_TRUE(ConnectChannel());
     ASSERT_TRUE(StartClient());
 
@@ -390,10 +400,12 @@ TEST_F(IPCAttachmentBrokerPrivilegedWinTest, SendHandleToSelf) {
   Init("SendHandleToSelf");
 
   set_broker(new MockBroker);
-  CommonSetUp();
+
+  PreConnectSetUp();
   // Technically, the channel is an endpoint, but we need the proxy listener to
   // receive the messages so that it can quit the message loop.
   channel()->SetAttachmentBrokerEndpoint(false);
+  PostConnectSetUp();
   get_proxy_listener()->set_listener(get_broker());
 
   HANDLE h = CreateTempFile();

ipc/ipc_channel.h

@@ -166,6 +166,9 @@ class IPC_EXPORT Channel : public Endpoint {
   // connect to a pre-existing pipe.  Note, calling Connect()
   // will not block the calling thread and may complete
   // asynchronously.
+  //
+  // The subclass implementation must call WillConnect() at the beginning of its
+  // implementation.
   virtual bool Connect() WARN_UNUSED_RESULT = 0;
 
   // Close this Channel explicitly.  May be called multiple times.
@@ -256,6 +259,16 @@ class IPC_EXPORT Channel : public Endpoint {
     void* buffer_;
     size_t length_;
   };
+
+  // Endpoint overrides.
+  void OnSetAttachmentBrokerEndpoint() override;
+
+  // Subclasses must call this method at the beginning of their implementation
+  // of Connect().
+  void WillConnect();
+
+ private:
+  bool did_start_connect_ = false;
 };
 
 #if defined(OS_POSIX)

ipc/ipc_channel_common.cc

@@ -52,5 +52,13 @@ bool Channel::IsSendThreadSafe() const {
   return false;
 }
 
+void Channel::OnSetAttachmentBrokerEndpoint() {
+  CHECK(!did_start_connect_);
+}
+
+void Channel::WillConnect() {
+  did_start_connect_ = true;
+}
+
 }  // namespace IPC
 

ipc/ipc_channel_nacl.cc

@@ -155,6 +155,8 @@ base::ProcessId ChannelNacl::GetSelfPID() const {
 }
 
 bool ChannelNacl::Connect() {
+  WillConnect();
+
   if (pipe_ == -1) {
     DLOG(WARNING) << "Channel creation failed: " << pipe_name_;
     return false;

ipc/ipc_channel_posix.cc

@@ -341,6 +341,8 @@ bool ChannelPosix::CreatePipe(
 }
 
 bool ChannelPosix::Connect() {
+  WillConnect();
+
   if (!server_listen_pipe_.is_valid() && !pipe_.is_valid()) {
     DLOG(WARNING) << "Channel creation failed: " << pipe_name_;
     return false;

ipc/ipc_channel_proxy.cc

@@ -490,6 +490,7 @@ base::ProcessId ChannelProxy::GetPeerPID() const {
 }
 
 void ChannelProxy::OnSetAttachmentBrokerEndpoint() {
+  CHECK(!did_init_);
   context()->set_attachment_broker_endpoint(is_attachment_broker_endpoint());
 }
 

ipc/ipc_channel_proxy.h

@@ -97,6 +97,11 @@ class IPC_EXPORT ChannelProxy : public Endpoint, public base::NonThreadSafe {
       Listener* listener,
       const scoped_refptr<base::SingleThreadTaskRunner>& ipc_task_runner);
 
+  // Constructs a ChannelProxy without initializing it.
+  ChannelProxy(
+      Listener* listener,
+      const scoped_refptr<base::SingleThreadTaskRunner>& ipc_task_runner);
+
   ~ChannelProxy() override;
 
   // Initializes the channel proxy. Only call this once to initialize a channel
@@ -159,9 +164,6 @@ class IPC_EXPORT ChannelProxy : public Endpoint, public base::NonThreadSafe {
   // to the internal state.
   ChannelProxy(Context* context);
 
-  ChannelProxy(
-      Listener* listener,
-      const scoped_refptr<base::SingleThreadTaskRunner>& ipc_task_runner);
 
   // Used internally to hold state that is referenced on the IPC thread.
   class Context : public base::RefCountedThreadSafe<Context>,

ipc/ipc_channel_win.cc

@@ -392,6 +392,8 @@ bool ChannelWin::CreatePipe(const IPC::ChannelHandle &channel_handle,
 }
 
 bool ChannelWin::Connect() {
+  WillConnect();
+
   DLOG_IF(WARNING, thread_check_.get()) << "Connect called more than once";
 
   if (!thread_check_.get())

ipc/ipc_endpoint.h

@@ -32,7 +32,7 @@ class IPC_EXPORT Endpoint : public Sender {
 
   // A callback that indicates that is_attachment_broker_endpoint() has been
   // changed.
-  virtual void OnSetAttachmentBrokerEndpoint(){};
+  virtual void OnSetAttachmentBrokerEndpoint() = 0;
 
   // Whether this channel is used as an endpoint for sending and receiving
   // brokerable attachment messages to/from the broker process.

ipc/ipc_test_base.h

@@ -105,6 +105,10 @@ class IPCTestBase : public base::MultiProcessTest {
 
   IPC::Channel* channel() { return channel_.get(); }
   IPC::ChannelProxy* channel_proxy() { return channel_proxy_.get(); }
+  void set_channel_proxy(std::unique_ptr<IPC::ChannelProxy> proxy) {
+    DCHECK(!channel_proxy_);
+    channel_proxy_.swap(proxy);
+  }
 
   const base::Process& client_process() const { return client_process_; }
   scoped_refptr<base::SequencedTaskRunner> task_runner();

ipc/mojo/ipc_channel_mojo.cc

@@ -239,6 +239,7 @@ ChannelMojo::~ChannelMojo() {
 }
 
 bool ChannelMojo::Connect() {
+  WillConnect();
   base::AutoLock lock(lock_);
   DCHECK(!task_runner_);
   task_runner_ = base::ThreadTaskRunnerHandle::Get();

remoting/host/desktop_process.cc

@@ -140,14 +140,12 @@ bool DesktopProcess::Start(
   }
 
   // Connect to the daemon.
-  daemon_channel_ =
-      IPC::ChannelProxy::Create(daemon_channel_name_, IPC::Channel::MODE_CLIENT,
-                                this, io_task_runner.get());
-
+  daemon_channel_.reset(new IPC::ChannelProxy(this, io_task_runner.get()));
   IPC::AttachmentBrokerUnprivileged::CreateBrokerIfNeeded();
   IPC::AttachmentBroker* broker = IPC::AttachmentBroker::GetGlobal();
   if (broker && !broker->IsPrivilegedBroker())
     broker->RegisterBrokerCommunicationChannel(daemon_channel_.get());
+  daemon_channel_->Init(daemon_channel_name_, IPC::Channel::MODE_CLIENT, true);
 
   // Pass |desktop_pipe| to the daemon.
   daemon_channel_->Send(

remoting/host/desktop_session_agent.cc

@@ -13,6 +13,7 @@
 #include "base/memory/shared_memory.h"
 #include "base/process/process_handle.h"
 #include "build/build_config.h"
+#include "ipc/attachment_broker.h"
 #include "ipc/ipc_channel_proxy.h"
 #include "ipc/ipc_message.h"
 #include "ipc/ipc_message_macros.h"
@@ -215,6 +216,10 @@ void DesktopSessionAgent::OnChannelError() {
   DCHECK(caller_task_runner_->BelongsToCurrentThread());
 
   // Make sure the channel is closed.
+  if (IPC::AttachmentBroker::GetGlobal()) {
+    IPC::AttachmentBroker::GetGlobal()->DeregisterCommunicationChannel(
+        network_channel_.get());
+  }
   network_channel_.reset();
   desktop_pipe_.Close();
 
@@ -413,6 +418,10 @@ void DesktopSessionAgent::Stop() {
   delegate_.reset();
 
   // Make sure the channel is closed.
+  if (IPC::AttachmentBroker::GetGlobal()) {
+    IPC::AttachmentBroker::GetGlobal()->DeregisterCommunicationChannel(
+        network_channel_.get());
+  }
   network_channel_.reset();
 
   if (started_) {

remoting/host/ipc_util.h

@@ -34,6 +34,7 @@ namespace remoting {
 // on the caller's thread while using |io_task_runner| to send and receive
 // messages in the background. The client end is returned as a pipe handle
 // (inheritable on Windows).
+// The channel is registered with the global AttachmentBroker.
 bool CreateConnectedIpcChannel(
     scoped_refptr<base::SingleThreadTaskRunner> io_task_runner,
     IPC::Listener* listener,