From 5e25b795ba30601fbd543fb311e28c946de52560 Mon Sep 17 00:00:00 2001
From: Jun Cai <juncai@chromium.org>
Date: Thu, 28 Feb 2019 03:27:43 +0000
Subject: [PATCH] Mojo: Update ChannelPosix::WriteNoLock() to send limited
 number of handles at a time

This CL changes ChannelPosix::WriteNoLock() to send limited number
of handles at a time so that it won't overflow the |cmsg_buf| buffer
that is used at mojo::SendmsgWithHandles().

Bug: 935357
Change-Id: I9df239763d5976022564881b8da81acfa6d61f1c
Reviewed-on: https://chromium-review.googlesource.com/c/1489104
Commit-Queue: Jun Cai <juncai@chromium.org>
Reviewed-by: Ken Rockot <rockot@google.com>
Cr-Commit-Position: refs/heads/master@{#636287}
---
 mojo/core/channel_posix.cc                    | 29 ++++++++++++++-----
 .../public/cpp/platform/socket_utils_posix.cc |  4 +--
 mojo/public/cpp/platform/socket_utils_posix.h |  5 ++++
 3 files changed, 28 insertions(+), 10 deletions(-)

diff --git a/mojo/core/channel_posix.cc b/mojo/core/channel_posix.cc
index 430dbb740541c7..2ef2a5fcfe3e61 100644
--- a/mojo/core/channel_posix.cc
+++ b/mojo/core/channel_posix.cc
@@ -85,10 +85,17 @@ class MessageView {
     handles_ = std::move(handles);
   }
 
+  size_t num_handles_sent() { return num_handles_sent_; }
+
+  void set_num_handles_sent(size_t num_handles_sent) {
+    num_handles_sent_ = num_handles_sent;
+  }
+
  private:
   Channel::MessagePtr message_;
   size_t offset_;
   std::vector<PlatformHandleInTransit> handles_;
+  size_t num_handles_sent_ = 0;
 
   DISALLOW_COPY_AND_ASSIGN(MessageView);
 };
@@ -514,17 +521,21 @@ class ChannelPosix : public Channel,
       return true;
     }
     size_t bytes_written = 0;
+    std::vector<PlatformHandleInTransit> handles = message_view.TakeHandles();
+    size_t num_handles = handles.size();
+    size_t handles_written = message_view.num_handles_sent();
     do {
       message_view.advance_data_offset(bytes_written);
 
       ssize_t result;
-      std::vector<PlatformHandleInTransit> handles = message_view.TakeHandles();
-      if (!handles.empty()) {
+      if (handles_written < num_handles) {
         iovec iov = {const_cast<void*>(message_view.data()),
                      message_view.data_num_bytes()};
-        std::vector<base::ScopedFD> fds(handles.size());
-        for (size_t i = 0; i < handles.size(); ++i)
-          fds[i] = handles[i].TakeHandle().TakeFD();
+        size_t num_handles_to_send =
+            std::min(num_handles - handles_written, kMaxSendmsgHandles);
+        std::vector<base::ScopedFD> fds(num_handles_to_send);
+        for (size_t i = 0; i < num_handles_to_send; ++i)
+          fds[i] = handles[i + handles_written].TakeHandle().TakeFD();
         // TODO: Handle lots of handles.
         result = SendmsgWithHandles(socket_.get(), &iov, 1, fds);
         if (result >= 0) {
@@ -549,11 +560,14 @@ class ChannelPosix : public Channel,
               fds_to_close_.emplace_back(std::move(fd));
           }
 #endif  // defined(OS_MACOSX)
+          handles_written += num_handles_to_send;
+          DCHECK_LE(handles_written, num_handles);
+          message_view.set_num_handles_sent(handles_written);
         } else {
           // Message transmission failed, so pull the FDs back into |handles|
           // so they can be held by the Message again.
           for (size_t i = 0; i < fds.size(); ++i) {
-            handles[i] =
+            handles[i + handles_written] =
                 PlatformHandleInTransit(PlatformHandle(std::move(fds[i])));
           }
         }
@@ -591,7 +605,8 @@ class ChannelPosix : public Channel,
       }
 
       bytes_written = static_cast<size_t>(result);
-    } while (bytes_written < message_view.data_num_bytes());
+    } while (handles_written < num_handles ||
+             bytes_written < message_view.data_num_bytes());
 
     return FlushOutgoingMessagesNoLock();
   }
diff --git a/mojo/public/cpp/platform/socket_utils_posix.cc b/mojo/public/cpp/platform/socket_utils_posix.cc
index 4bbdcb754bb876..6199a36a69e9b6 100644
--- a/mojo/public/cpp/platform/socket_utils_posix.cc
+++ b/mojo/public/cpp/platform/socket_utils_posix.cc
@@ -73,8 +73,6 @@ constexpr int kSendmsgFlags = 0;
 constexpr int kSendmsgFlags = MSG_NOSIGNAL;
 #endif
 
-constexpr size_t kMaxSendmsgHandles = 128;
-
 }  // namespace
 
 ssize_t SocketWrite(base::PlatformFile socket,
@@ -107,7 +105,7 @@ ssize_t SendmsgWithHandles(base::PlatformFile socket,
   DCHECK(iov);
   DCHECK_GT(num_iov, 0u);
   DCHECK(!descriptors.empty());
-  DCHECK_LE(descriptors.size(), kMaxSendmsgHandles);
+  CHECK_LE(descriptors.size(), kMaxSendmsgHandles);
 
   char cmsg_buf[CMSG_SPACE(kMaxSendmsgHandles * sizeof(int))];
   struct msghdr msg = {};
diff --git a/mojo/public/cpp/platform/socket_utils_posix.h b/mojo/public/cpp/platform/socket_utils_posix.h
index e512f1bc8077ec..30082484d56652 100644
--- a/mojo/public/cpp/platform/socket_utils_posix.h
+++ b/mojo/public/cpp/platform/socket_utils_posix.h
@@ -18,6 +18,11 @@ struct iovec;  // Declared in <sys/uio.h>
 
 namespace mojo {
 
+// There is an upper bound of number of handles on what is supported across
+// various OS implementations of sendmsg(). This value was chosen because it
+// should be safe across all supported platforms.
+constexpr size_t kMaxSendmsgHandles = 128;
+
 // NOTE: Functions declared here really don't belong in Mojo, but they exist to
 // support code which used to rely on internal parts of the Mojo implementation
 // and there wasn't a much better home for them. Consider moving them to