Bug 1840974 - Part 3: Make sure otherpid is set if known on IPC::Channel, r=ipc-reviewers,jld

This improves the quality of the logging from IPC::Channel, and also
adds extra assertions to ensure that it's aligned with the values
sent/received in the HELLO message.

This patch also makes the other_pid type more consistent, using
base::ProcessId instead of int32_t in IPC::Channel.

Differential Revision: https://phabricator.services.mozilla.com/D183410

ipc/chromium/src/chrome/common/child_thread.cc

@@ -9,10 +9,11 @@
 #include "chrome/common/child_process.h"
 #include "mozilla/ipc/NodeController.h"
 
-ChildThread::ChildThread(Thread::Options options)
+ChildThread::ChildThread(Thread::Options options, base::ProcessId parent_pid)
     : Thread("IPC I/O Child"),
       owner_loop_(MessageLoop::current()),
-      options_(options) {
+      options_(options),
+      parent_pid_(parent_pid) {
   DCHECK(owner_loop_);
 }
 
@@ -32,16 +33,16 @@ void ChildThread::Init() {
   // to start the initial IPC connection to the parent process.
   IPC::Channel::ChannelHandle client_handle(
       IPC::Channel::GetClientChannelHandle());
-  auto channel = mozilla::MakeUnique<IPC::Channel>(std::move(client_handle),
-                                                   IPC::Channel::MODE_CLIENT);
+  auto channel = mozilla::MakeUnique<IPC::Channel>(
+      std::move(client_handle), IPC::Channel::MODE_CLIENT, parent_pid_);
 #if defined(XP_WIN)
   channel->StartAcceptingHandles(IPC::Channel::MODE_CLIENT);
 #elif defined(XP_DARWIN)
   channel->StartAcceptingMachPorts(IPC::Channel::MODE_CLIENT);
 #endif
 
-  initial_port_ =
-      mozilla::ipc::NodeController::InitChildProcess(std::move(channel));
+  initial_port_ = mozilla::ipc::NodeController::InitChildProcess(
+      std::move(channel), parent_pid_);
 }
 
 void ChildThread::CleanUp() { mozilla::ipc::NodeController::CleanUp(); }

ipc/chromium/src/chrome/common/child_thread.h

@@ -19,7 +19,7 @@ class ResourceDispatcher;
 class ChildThread : public base::Thread {
  public:
   // Creates the thread.
-  explicit ChildThread(Thread::Options options);
+  ChildThread(Thread::Options options, base::ProcessId parent_pid);
   virtual ~ChildThread();
 
   mozilla::ipc::ScopedPort TakeInitialPort() {
@@ -48,6 +48,8 @@ class ChildThread : public base::Thread {
 
   Thread::Options options_;
 
+  base::ProcessId parent_pid_;
+
   DISALLOW_EVIL_CONSTRUCTORS(ChildThread);
 };
 

ipc/chromium/src/chrome/common/ipc_channel.h

@@ -82,11 +82,14 @@ class Channel {
   // |mode| specifies whether this channel is operating in server mode or client
   // mode. One side of the connection should be the client, and the other should
   // be the server.
+  // |other_pid| specifies the pid of the other side of this channel. This will
+  // be used for logging, and for transferring HANDLEs from a privileged process
+  // on Windows (if enabled).
   //
   // The Channel must be created and destroyed on the IO thread, and all
   // methods, unless otherwise noted, are only safe to call on the I/O thread.
   //
-  Channel(ChannelHandle pipe, Mode mode);
+  Channel(ChannelHandle pipe, Mode mode, base::ProcessId other_pid);
 
   ~Channel();
 
@@ -112,9 +115,13 @@ class Channel {
   // immediately.
   bool Send(mozilla::UniquePtr<Message> message);
 
-  // The PID which this channel has been opened with. This will be
-  // `-1` until `OnChannelConnected` has been called.
-  int32_t OtherPid() const;
+  // Explicitly set the pid expected for the other side of this channel. This
+  // will be used for logging, and on Windows may be used for transferring
+  // handles between processes.
+  //
+  // If it is set this way, the "hello" message will be checked to ensure that
+  // the same pid is reported.
+  void SetOtherPid(base::ProcessId other_pid);
 
   // IsClosed() is safe to call from any thread, but the value returned may
   // be out of date.

ipc/chromium/src/chrome/common/ipc_channel_posix.cc

@@ -33,6 +33,7 @@
 #include "base/command_line.h"
 #include "base/eintr_wrapper.h"
 #include "base/logging.h"
+#include "base/process.h"
 #include "base/process_util.h"
 #include "base/string_util.h"
 #include "chrome/common/chrome_switches.h"
@@ -140,9 +141,11 @@ void Channel::SetClientChannelFd(int fd) { gClientChannelFd = fd; }
 
 int Channel::GetClientChannelHandle() { return gClientChannelFd; }
 
-Channel::ChannelImpl::ChannelImpl(ChannelHandle pipe, Mode mode)
+Channel::ChannelImpl::ChannelImpl(ChannelHandle pipe, Mode mode,
+                                  base::ProcessId other_pid)
     : chan_cap_("ChannelImpl::SendMutex",
-                MessageLoopForIO::current()->SerialEventTarget()) {
+                MessageLoopForIO::current()->SerialEventTarget()),
+      other_pid_(other_pid) {
   Init(mode);
   SetPipe(pipe.release());
 
@@ -245,6 +248,16 @@ bool Channel::ChannelImpl::ContinueConnect() {
   return ProcessOutgoingMessages();
 }
 
+void Channel::ChannelImpl::SetOtherPid(base::ProcessId other_pid) {
+  IOThread().AssertOnCurrentThread();
+  mozilla::MutexAutoLock lock(SendMutex());
+  chan_cap_.NoteExclusiveAccess();
+  MOZ_RELEASE_ASSERT(
+      other_pid_ == base::kInvalidProcessId || other_pid_ == other_pid,
+      "Multiple sources of SetOtherPid disagree!");
+  other_pid_ = other_pid;
+}
+
 bool Channel::ChannelImpl::ProcessIncomingMessages() {
   chan_cap_.NoteOnIOThread();
 
@@ -1163,8 +1176,8 @@ bool Channel::ChannelImpl::TransferMachPorts(Message& msg) {
 
 //------------------------------------------------------------------------------
 // Channel's methods simply call through to ChannelImpl.
-Channel::Channel(ChannelHandle pipe, Mode mode)
-    : channel_impl_(new ChannelImpl(std::move(pipe), mode)) {
+Channel::Channel(ChannelHandle pipe, Mode mode, base::ProcessId other_pid)
+    : channel_impl_(new ChannelImpl(std::move(pipe), mode, other_pid)) {
   MOZ_COUNT_CTOR(IPC::Channel);
 }
 
@@ -1180,7 +1193,9 @@ bool Channel::Send(mozilla::UniquePtr<Message> message) {
   return channel_impl_->Send(std::move(message));
 }
 
-int32_t Channel::OtherPid() const { return channel_impl_->OtherPid(); }
+void Channel::SetOtherPid(base::ProcessId other_pid) {
+  channel_impl_->SetOtherPid(other_pid);
+}
 
 bool Channel::IsClosed() const { return channel_impl_->IsClosed(); }
 

ipc/chromium/src/chrome/common/ipc_channel_posix.h

@@ -18,6 +18,7 @@
 #include <list>
 
 #include "base/message_loop.h"
+#include "base/process.h"
 #include "base/task.h"
 
 #include "mozilla/Maybe.h"
@@ -36,18 +37,14 @@ class Channel::ChannelImpl : public MessageLoopForIO::Watcher {
       ChannelImpl, IOThread().GetEventTarget());
 
   // Mirror methods of Channel, see ipc_channel.h for description.
-  ChannelImpl(ChannelHandle pipe, Mode mode);
+  ChannelImpl(ChannelHandle pipe, Mode mode, base::ProcessId other_pid);
   bool Connect(Listener* listener) MOZ_EXCLUDES(SendMutex());
   void Close() MOZ_EXCLUDES(SendMutex());
 
   // NOTE: `Send` may be called on threads other than the I/O thread.
   bool Send(mozilla::UniquePtr<Message> message) MOZ_EXCLUDES(SendMutex());
 
-  int32_t OtherPid() {
-    IOThread().AssertOnCurrentThread();
-    chan_cap_.NoteOnIOThread();
-    return other_pid_;
-  }
+  void SetOtherPid(base::ProcessId other_pid);
 
   // See the comment in ipc_channel.h for info on IsClosed()
   // NOTE: `IsClosed` may be called on threads other than the I/O thread.
@@ -68,12 +65,6 @@ class Channel::ChannelImpl : public MessageLoopForIO::Watcher {
 
   void Init(Mode mode) MOZ_REQUIRES(SendMutex(), IOThread());
   void SetPipe(int fd) MOZ_REQUIRES(SendMutex(), IOThread());
-  void SetOtherPid(int other_pid) MOZ_REQUIRES(IOThread())
-      MOZ_EXCLUDES(SendMutex()) {
-    mozilla::MutexAutoLock lock(SendMutex());
-    chan_cap_.NoteExclusiveAccess();
-    other_pid_ = other_pid;
-  }
   bool PipeBufHasSpaceAfter(size_t already_written)
       MOZ_REQUIRES_SHARED(chan_cap_);
   bool EnqueueHelloMessage() MOZ_REQUIRES(SendMutex(), IOThread());
@@ -180,7 +171,8 @@ class Channel::ChannelImpl : public MessageLoopForIO::Watcher {
 
   // We keep track of the PID of the other side of this channel so that we can
   // record this when generating logs of IPC messages.
-  int32_t other_pid_ MOZ_GUARDED_BY(chan_cap_) = -1;
+  base::ProcessId other_pid_ MOZ_GUARDED_BY(chan_cap_) =
+      base::kInvalidProcessId;
 
 #if defined(XP_DARWIN)
   struct PendingDescriptors {

ipc/chromium/src/chrome/common/ipc_channel_win.cc

@@ -14,6 +14,7 @@
 #include "base/command_line.h"
 #include "base/compiler_specific.h"
 #include "base/logging.h"
+#include "base/process.h"
 #include "base/process_util.h"
 #include "base/rand_util.h"
 #include "base/string_util.h"
@@ -44,11 +45,13 @@ Channel::ChannelImpl::State::~State() {
 
 //------------------------------------------------------------------------------
 
-Channel::ChannelImpl::ChannelImpl(ChannelHandle pipe, Mode mode)
+Channel::ChannelImpl::ChannelImpl(ChannelHandle pipe, Mode mode,
+                                  base::ProcessId other_pid)
     : chan_cap_("ChannelImpl::SendMutex",
                 MessageLoopForIO::current()->SerialEventTarget()),
       ALLOW_THIS_IN_INITIALIZER_LIST(input_state_(this)),
-      ALLOW_THIS_IN_INITIALIZER_LIST(output_state_(this)) {
+      ALLOW_THIS_IN_INITIALIZER_LIST(output_state_(this)),
+      other_pid_(other_pid) {
   Init(mode);
 
   if (!pipe) {
@@ -209,9 +212,13 @@ bool Channel::ChannelImpl::Connect(Listener* listener) {
   return true;
 }
 
-void Channel::ChannelImpl::SetOtherPid(int other_pid) {
+void Channel::ChannelImpl::SetOtherPid(base::ProcessId other_pid) {
+  IOThread().AssertOnCurrentThread();
   mozilla::MutexAutoLock lock(SendMutex());
   chan_cap_.NoteExclusiveAccess();
+  MOZ_RELEASE_ASSERT(
+      other_pid_ == base::kInvalidProcessId || other_pid_ == other_pid,
+      "Multiple sources of SetOtherPid disagree!");
   other_pid_ = other_pid;
 
   // Now that we know the remote pid, open a privileged handle to the
@@ -501,7 +508,7 @@ void Channel::ChannelImpl::StartAcceptingHandles(Mode mode) {
   accept_handles_ = true;
   privileged_ = mode == MODE_SERVER;
 
-  if (privileged_ && other_pid_ != -1 &&
+  if (privileged_ && other_pid_ != base::kInvalidProcessId &&
       other_process_ == INVALID_HANDLE_VALUE) {
     other_process_ = OpenProcess(PROCESS_DUP_HANDLE, false, other_pid_);
     if (!other_process_) {
@@ -721,8 +728,8 @@ bool Channel::ChannelImpl::TransferHandles(Message& msg) {
 
 //------------------------------------------------------------------------------
 // Channel's methods simply call through to ChannelImpl.
-Channel::Channel(ChannelHandle pipe, Mode mode)
-    : channel_impl_(new ChannelImpl(std::move(pipe), mode)) {
+Channel::Channel(ChannelHandle pipe, Mode mode, base::ProcessId other_pid)
+    : channel_impl_(new ChannelImpl(std::move(pipe), mode, other_pid)) {
   MOZ_COUNT_CTOR(IPC::Channel);
 }
 
@@ -742,7 +749,9 @@ bool Channel::Send(mozilla::UniquePtr<Message> message) {
   return channel_impl_->Send(std::move(message));
 }
 
-int32_t Channel::OtherPid() const { return channel_impl_->OtherPid(); }
+void Channel::SetOtherPid(base::ProcessId other_pid) {
+  channel_impl_->SetOtherPid(other_pid);
+}
 
 bool Channel::IsClosed() const { return channel_impl_->IsClosed(); }
 

ipc/chromium/src/chrome/common/ipc_channel_win.h

@@ -15,6 +15,7 @@
 #include <string>
 
 #include "base/message_loop.h"
+#include "base/process.h"
 #include "base/task.h"
 #include "nsISupportsImpl.h"
 
@@ -34,18 +35,14 @@ class Channel::ChannelImpl : public MessageLoopForIO::IOHandler {
   using ChannelHandle = Channel::ChannelHandle;
 
   // Mirror methods of Channel, see ipc_channel.h for description.
-  ChannelImpl(ChannelHandle pipe, Mode mode);
+  ChannelImpl(ChannelHandle pipe, Mode mode, base::ProcessId other_pid);
   bool Connect(Listener* listener) MOZ_EXCLUDES(SendMutex());
   void Close() MOZ_EXCLUDES(SendMutex());
   void StartAcceptingHandles(Mode mode) MOZ_EXCLUDES(SendMutex());
   // NOTE: `Send` may be called on threads other than the I/O thread.
   bool Send(mozilla::UniquePtr<Message> message) MOZ_EXCLUDES(SendMutex());
 
-  int32_t OtherPid() {
-    IOThread().AssertOnCurrentThread();
-    chan_cap_.NoteOnIOThread();
-    return other_pid_;
-  }
+  void SetOtherPid(base::ProcessId other_pid);
 
   // See the comment in ipc_channel.h for info on IsClosed()
   // NOTE: `IsClosed` may be called on threads other than the I/O thread.
@@ -70,8 +67,6 @@ class Channel::ChannelImpl : public MessageLoopForIO::IOHandler {
       MOZ_REQUIRES(SendMutex());
   void OutputQueuePop() MOZ_REQUIRES(SendMutex());
 
-  void SetOtherPid(int other_pid) MOZ_REQUIRES(IOThread())
-      MOZ_EXCLUDES(SendMutex());
   bool EnqueueHelloMessage() MOZ_REQUIRES(SendMutex(), IOThread());
   void CloseLocked() MOZ_REQUIRES(SendMutex(), IOThread());
 
@@ -151,7 +146,8 @@ class Channel::ChannelImpl : public MessageLoopForIO::IOHandler {
 
   // We keep track of the PID of the other side of this channel so that we can
   // record this when generating logs of IPC messages.
-  int32_t other_pid_ MOZ_GUARDED_BY(chan_cap_) = -1;
+  base::ProcessId other_pid_ MOZ_GUARDED_BY(chan_cap_) =
+      base::kInvalidProcessId;
 
   // Whether or not to accept handles from a remote process, and whether this
   // process is the privileged side of a IPC::Channel which can transfer

ipc/glue/GeckoChildProcessHost.cpp

@@ -7,6 +7,7 @@
 #include "GeckoChildProcessHost.h"
 
 #include "base/command_line.h"
+#include "base/process.h"
 #include "base/process_util.h"
 #include "base/string_util.h"
 #include "base/task.h"
@@ -755,10 +756,15 @@ bool GeckoChildProcessHost::AsyncLaunch(std::vector<std::string> aExtraOpts) {
 
 #ifdef XP_MACOSX
                     this->mChildTask = aResults.mChildTask;
-                    if (mNodeChannel) {
-                      mNodeChannel->SetMachTaskPort(this->mChildTask);
-                    }
 #endif
+
+                    if (mNodeChannel) {
+                      mNodeChannel->SetOtherPid(
+                          base::GetProcId(this->mChildProcessHandle));
+#ifdef XP_MACOSX
+                      mNodeChannel->SetMachTaskPort(this->mChildTask);
+#endif
+                    }
                   }
 #if defined(XP_WIN) && defined(MOZ_SANDBOX)
                   this->mSandboxBroker = std::move(aResults.mSandboxBroker);
@@ -875,7 +881,8 @@ void GeckoChildProcessHost::InitializeChannel(
   // Create the IPC channel which will be used for communication with this
   // process.
   mozilla::UniquePtr<IPC::Channel> channel = MakeUnique<IPC::Channel>(
-      std::move(aServerHandle), IPC::Channel::MODE_SERVER);
+      std::move(aServerHandle), IPC::Channel::MODE_SERVER,
+      base::kInvalidProcessId);
 #if defined(XP_WIN)
   channel->StartAcceptingHandles(IPC::Channel::MODE_SERVER);
 #elif defined(XP_DARWIN)

ipc/glue/IOThreadChild.h

@@ -20,10 +20,10 @@ namespace ipc {
 // IPC IO MessageLoop lives.
 class IOThreadChild : public ChildThread {
  public:
-  IOThreadChild()
+  explicit IOThreadChild(base::ProcessId aParentPid)
       : ChildThread(base::Thread::Options(MessageLoop::TYPE_IO,
-                                          0))  // stack size
-  {}
+                                          /* stack size */ 0),
+                    aParentPid) {}
 
   ~IOThreadChild() = default;
 

ipc/glue/NodeChannel.cpp

@@ -108,6 +108,8 @@ void NodeChannel::SetOtherPid(base::ProcessId aNewPid) {
     MOZ_RELEASE_ASSERT(previousPid == aNewPid,
                        "Different sources disagree on the correct pid?");
   }
+
+  mChannel->SetOtherPid(aNewPid);
 }
 
 #ifdef XP_MACOSX

ipc/glue/NodeChannel.h

@@ -114,6 +114,10 @@ class NodeChannel final : public IPC::Channel::Listener {
   const NodeName& GetName() { return mName; }
 #endif
 
+  // Update the known PID for the remote process. MUST BE CALLED FROM THE IO
+  // THREAD.
+  void SetOtherPid(base::ProcessId aNewPid);
+
 #ifdef XP_MACOSX
   // Called by the GeckoChildProcessHost to provide the task_t for the peer
   // process. MUST BE CALLED FROM THE IO THREAD.
@@ -126,9 +130,6 @@ class NodeChannel final : public IPC::Channel::Listener {
   void Destroy();
   void FinalDestroy();
 
-  // Update the known PID for the remote process. IO THREAD ONLY
-  void SetOtherPid(base::ProcessId aNewPid);
-
   void SendMessage(UniquePtr<IPC::Message> aMessage);
 
   // IPC::Channel::Listener implementation

ipc/glue/NodeController.cpp

@@ -610,8 +610,9 @@ void NodeController::OnIntroduce(const NodeName& aFromNode,
     return;
   }
 
-  auto channel = MakeUnique<IPC::Channel>(std::move(aIntroduction.mHandle),
-                                          aIntroduction.mMode);
+  auto channel =
+      MakeUnique<IPC::Channel>(std::move(aIntroduction.mHandle),
+                               aIntroduction.mMode, aIntroduction.mOtherPid);
   auto nodeChannel = MakeRefPtr<NodeChannel>(
       aIntroduction.mName, std::move(channel), this, aIntroduction.mOtherPid);
 
@@ -786,7 +787,7 @@ void NodeController::InitBrokerProcess() {
 }
 
 ScopedPort NodeController::InitChildProcess(UniquePtr<IPC::Channel> aChannel,
-                                            int32_t aParentPid) {
+                                            base::ProcessId aParentPid) {
   AssertIOThread();
   MOZ_ASSERT(!gNodeController);
 

ipc/glue/NodeController.h

@@ -100,7 +100,7 @@ class NodeController final : public mojo::core::ports::NodeDelegate,
 
   // Called as the IO thread is started in a child process.
   static ScopedPort InitChildProcess(UniquePtr<IPC::Channel> aChannel,
-                                     int32_t aParentPid = -1);
+                                     base::ProcessId aParentPid);
 
   // Called when the IO thread is torn down.
   static void CleanUp();

ipc/glue/ProcessChild.cpp

@@ -33,7 +33,7 @@ ProcessChild* ProcessChild::gProcessChild;
 static Atomic<bool> sExpectingShutdown(false);
 
 ProcessChild::ProcessChild(ProcessId aParentPid, const nsID& aMessageChannelId)
-    : ChildProcess(new IOThreadChild()),
+    : ChildProcess(new IOThreadChild(aParentPid)),
       mUILoop(MessageLoop::current()),
       mParentPid(aParentPid),
       mMessageChannelId(aMessageChannelId) {