Fix a Mach port dead-name leak in mojo::core::ChannelMac.

If a channel sends a message to what turns out to be a dead-name port,
before the channel processes the kernel's dead-name notification, the
channel would leak the dead-name right. This is because the dead-name
notification message contains a port right that must be destroyed, and
that right is not transferred as a normal message descriptor. Therefore,
the right is not automatically released when the message remains in the
kernel message queue and the associated receive right is destroyed.

To fix this, if a channel fails to send a message because it is to an
invalid destination (namely, a dead-name), do not immediately report the
error. Instead, wait for the channel to process the dead-name
notification so that the port right can be appropriately disposed.

Bug: 1041682
Change-Id: I99ffdea096670ef1e133f62ae269292b1d7e290a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2013524Reviewed-by: Ken Rockot <rockot@google.com>
Reviewed-by: Mark Mentovai <mark@chromium.org>
Commit-Queue: Robert Sesek <rsesek@chromium.org>
Cr-Commit-Position: refs/heads/master@{#734670}

mojo/core/channel_mac.cc

@@ -446,12 +446,19 @@ class ChannelMac : public Channel,
         io_task_runner_->PostTask(
             FROM_HERE, base::BindOnce(&ChannelMac::SendPendingMessages, this));
       } else {
-        // If the message failed to send for other reasons, destroy it and
-        // close the channel.
-        MACH_LOG_IF(ERROR, kr != MACH_SEND_INVALID_DEST, kr) << "mach_msg send";
+        // If the message failed to send for other reasons, destroy it.
         send_buffer_contains_message_ = false;
         mach_msg_destroy(header);
-        OnWriteErrorLocked(Error::kDisconnected);
+        if (kr != MACH_SEND_INVALID_DEST) {
+          // If the message failed to send because the receiver is a dead-name,
+          // wait for the Channel to process the dead-name notification.
+          // Otherwise, the notification message will never be received and the
+          // dead-name right contained within it will be leaked
+          // (https://crbug.com/1041682). If the message failed to send for any
+          // other reason, report an error and shut down.
+          MACH_LOG(ERROR, kr) << "mach_msg send";
+          OnWriteErrorLocked(Error::kDisconnected);
+        }
       }
       return false;
     }

mojo/core/channel_unittest.cc

@@ -14,6 +14,7 @@
 #include "base/process/process_metrics.h"
 #include "base/run_loop.h"
 #include "base/strings/stringprintf.h"
+#include "base/test/bind_test_util.h"
 #include "base/test/task_environment.h"
 #include "base/threading/thread.h"
 #include "base/threading/thread_task_runner_handle.h"
@@ -518,36 +519,34 @@ TEST(ChannelTest, PeerStressTest) {
   EXPECT_EQ(0u, delegate_b.error_count_);
 }
 
-class SingleMessageWaiterDelegate : public Channel::Delegate {
+class CallbackChannelDelegate : public Channel::Delegate {
  public:
-  SingleMessageWaiterDelegate() {}
+  CallbackChannelDelegate() = default;
 
   void OnChannelMessage(const void* payload,
                         size_t payload_size,
                         std::vector<PlatformHandle> handles) override {
-    message_received_ = true;
-    run_loop_->Quit();
+    if (on_message_)
+      std::move(on_message_).Run();
   }
 
   void OnChannelError(Channel::Error error) override {
-    channel_error_ = true;
-    run_loop_->Quit();
+    if (on_error_)
+      std::move(on_error_).Run();
   }
 
-  void Reset(base::RunLoop* loop) {
-    run_loop_ = loop;
-    message_received_ = false;
-    channel_error_ = false;
+  void set_on_message(base::OnceClosure on_message) {
+    on_message_ = std::move(on_message);
   }
 
-  bool message_received() { return message_received_; }
-  bool channel_error() { return channel_error_; }
+  void set_on_error(base::OnceClosure on_error) {
+    on_error_ = std::move(on_error);
+  }
 
  private:
-  bool message_received_ = false;
-  bool channel_error_ = false;
-  base::RunLoop* run_loop_;
-  DISALLOW_COPY_AND_ASSIGN(SingleMessageWaiterDelegate);
+  base::OnceClosure on_message_;
+  base::OnceClosure on_error_;
+  DISALLOW_COPY_AND_ASSIGN(CallbackChannelDelegate);
 };
 
 TEST(ChannelTest, MessageSizeTest) {
@@ -555,7 +554,7 @@ TEST(ChannelTest, MessageSizeTest) {
       base::test::TaskEnvironment::MainThreadType::IO);
   PlatformChannel platform_channel;
 
-  SingleMessageWaiterDelegate receiver_delegate;
+  CallbackChannelDelegate receiver_delegate;
   scoped_refptr<Channel> receiver =
       Channel::Create(&receiver_delegate,
                       ConnectionParams(platform_channel.TakeLocalEndpoint()),
@@ -577,15 +576,132 @@ TEST(ChannelTest, MessageSizeTest) {
     memset(message->mutable_payload(), 0xAB, i);
     sender->Write(std::move(message));
 
+    bool got_message = false, got_error = false;
+
     base::RunLoop loop;
-    receiver_delegate.Reset(&loop);
+    receiver_delegate.set_on_message(
+        base::BindLambdaForTesting([&got_message, &loop]() {
+          got_message = true;
+          loop.Quit();
+        }));
+    receiver_delegate.set_on_error(
+        base::BindLambdaForTesting([&got_error, &loop]() {
+          got_error = true;
+          loop.Quit();
+        }));
     loop.Run();
 
-    EXPECT_TRUE(receiver_delegate.message_received());
-    EXPECT_FALSE(receiver_delegate.channel_error());
+    EXPECT_TRUE(got_message);
+    EXPECT_FALSE(got_error);
   }
 }
 
+#if defined(OS_MACOSX) && !defined(OS_IOS)
+TEST(ChannelTest, SendToDeadMachPortName) {
+  base::test::SingleThreadTaskEnvironment task_environment(
+      base::test::TaskEnvironment::MainThreadType::IO);
+
+  // Create a second IO thread for the B channel. It needs to process tasks
+  // separately from channel A.
+  base::Thread::Options thread_options;
+  thread_options.message_pump_type = base::MessagePumpType::IO;
+  base::Thread peer_thread("channel_b_io");
+  peer_thread.StartWithOptions(thread_options);
+
+  // Create a PlatformChannel send/receive right pair.
+  PlatformChannel platform_channel;
+
+  mach_port_urefs_t send = 0, dead = 0;
+  mach_port_t send_name = platform_channel.local_endpoint()
+                              .platform_handle()
+                              .GetMachSendRight()
+                              .get();
+
+  auto get_send_name_refs = [&send, &dead, send_name]() {
+    kern_return_t kr = mach_port_get_refs(mach_task_self(), send_name,
+                                          MACH_PORT_RIGHT_SEND, &send);
+    ASSERT_EQ(kr, KERN_SUCCESS);
+    kr = mach_port_get_refs(mach_task_self(), send_name,
+                            MACH_PORT_RIGHT_DEAD_NAME, &dead);
+    ASSERT_EQ(kr, KERN_SUCCESS);
+  };
+
+  get_send_name_refs();
+  EXPECT_EQ(1u, send);
+  EXPECT_EQ(0u, dead);
+
+  // Add an extra send right.
+  ASSERT_EQ(KERN_SUCCESS, mach_port_mod_refs(mach_task_self(), send_name,
+                                             MACH_PORT_RIGHT_SEND, 1));
+  get_send_name_refs();
+  EXPECT_EQ(2u, send);
+  EXPECT_EQ(0u, dead);
+  base::mac::ScopedMachSendRight extra_send(send_name);
+
+  // Channel A gets created with the Mach send right from |platform_channel|.
+  CallbackChannelDelegate delegate_a;
+  scoped_refptr<Channel> channel_a = Channel::Create(
+      &delegate_a, ConnectionParams(platform_channel.TakeLocalEndpoint()),
+      Channel::HandlePolicy::kAcceptHandles,
+      base::ThreadTaskRunnerHandle::Get());
+  channel_a->Start();
+
+  // Channel B gets the receive right.
+  MockChannelDelegate delegate_b;
+  scoped_refptr<Channel> channel_b = Channel::Create(
+      &delegate_b, ConnectionParams(platform_channel.TakeRemoteEndpoint()),
+      Channel::HandlePolicy::kAcceptHandles, peer_thread.task_runner());
+  channel_b->Start();
+
+  // Ensure the channels have started and are talking.
+  channel_b->Write(std::make_unique<Channel::Message>(0, 0));
+
+  {
+    base::RunLoop loop;
+    delegate_a.set_on_message(loop.QuitClosure());
+    loop.Run();
+  }
+
+  // Queue two messages from B to A. Two are required so that channel A does
+  // not immediately process the dead-name notification when channel B shuts
+  // down.
+  channel_b->Write(std::make_unique<Channel::Message>(0, 0));
+  channel_b->Write(std::make_unique<Channel::Message>(0, 0));
+
+  // Turn Channel A's send right into a dead name.
+  channel_b->ShutDown();
+  channel_b = nullptr;
+
+  // ShutDown() posts a task on the channel's TaskRunner, so wait for that
+  // to run.
+  base::WaitableEvent event;
+  peer_thread.task_runner()->PostTask(
+      FROM_HERE,
+      base::BindOnce(&base::WaitableEvent::Signal, base::Unretained(&event)));
+  event.Wait();
+
+  // Force a send-to-dead-name on Channel A.
+  channel_a->Write(std::make_unique<Channel::Message>(0, 0));
+
+  {
+    base::RunLoop loop;
+    delegate_a.set_on_error(base::BindOnce(
+        [](scoped_refptr<Channel> channel, base::RunLoop* loop) {
+          channel->ShutDown();
+          channel = nullptr;
+          loop->QuitWhenIdle();
+        },
+        channel_a, base::Unretained(&loop)));
+    loop.Run();
+  }
+
+  // The only remaining ref should be the extra one that was added in the test.
+  get_send_name_refs();
+  EXPECT_EQ(0u, send);
+  EXPECT_EQ(1u, dead);
+}
+#endif  // defined(OS_MACOSX) && !defined(OS_IOS)
+
 }  // namespace
 }  // namespace core
 }  // namespace mojo