Mojo: Make PlatformChannelRecvmsg() append handles to a deque instead.

This is part 1 (of N) of my rearranging things to make passing FDs
actually work correctly.

R=yzshen@chromium.org

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

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@271849 0039d316-1c4b-4281-b951-d872f2087c98

mojo/embedder/platform_channel_pair_posix_unittest.cc

@@ -13,7 +13,7 @@
 #include <sys/uio.h>
 #include <unistd.h>
 
-#include <vector>
+#include <deque>
 
 #include "base/file_util.h"
 #include "base/files/file_path.h"
@@ -22,6 +22,8 @@
 #include "base/macros.h"
 #include "mojo/common/test/test_utils.h"
 #include "mojo/embedder/platform_channel_utils_posix.h"
+#include "mojo/embedder/platform_handle.h"
+#include "mojo/embedder/platform_handle_vector.h"
 #include "mojo/embedder/scoped_platform_handle.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -117,12 +119,12 @@ TEST_F(PlatformChannelPairPosixTest, SendReceiveData) {
     WaitReadable(client_handle.get());
 
     char buf[10000] = {};
-    ScopedPlatformHandleVectorPtr received_handles;
+    std::deque<PlatformHandle> received_handles;
     ssize_t result = PlatformChannelRecvmsg(client_handle.get(), buf,
                                             sizeof(buf), &received_handles);
     EXPECT_EQ(static_cast<ssize_t>(send_string.size()), result);
     EXPECT_EQ(send_string, std::string(buf, static_cast<size_t>(result)));
-    EXPECT_FALSE(received_handles);
+    EXPECT_TRUE(received_handles.empty());
   }
 }
 
@@ -152,17 +154,17 @@ TEST_F(PlatformChannelPairPosixTest, SendReceiveFDs) {
     WaitReadable(client_handle.get());
 
     char buf[100] = { 'a' };
-    ScopedPlatformHandleVectorPtr received_handles;
+    std::deque<PlatformHandle> received_handles;
     EXPECT_EQ(1, PlatformChannelRecvmsg(client_handle.get(), buf, sizeof(buf),
                                         &received_handles));
     EXPECT_EQ('\0', buf[0]);
-    ASSERT_TRUE(received_handles);
-    EXPECT_EQ(i, received_handles->size());
+    ASSERT_FALSE(received_handles.empty());
+    EXPECT_EQ(i, received_handles.size());
 
-    for (size_t j = 0; j < received_handles->size(); j++) {
+    for (size_t j = 0; !received_handles.empty(); j++) {
       base::ScopedFILE fp(test::FILEFromPlatformHandle(
-          ScopedPlatformHandle((*received_handles)[j]), "rb"));
-      (*received_handles)[j] = PlatformHandle();
+          ScopedPlatformHandle(received_handles.front()), "rb"));
+      received_handles.pop_front();
       ASSERT_TRUE(fp);
       rewind(fp.get());
       char read_buf[100];
@@ -198,23 +200,22 @@ TEST_F(PlatformChannelPairPosixTest, AppendReceivedFDs) {
 
   WaitReadable(client_handle.get());
 
-  // Start with an invalid handle in the vector.
-  ScopedPlatformHandleVectorPtr handles(new PlatformHandleVector());
-  handles->push_back(PlatformHandle());
+  // Start with an invalid handle in the deque.
+  std::deque<PlatformHandle> received_handles;
+  received_handles.push_back(PlatformHandle());
 
   char buf[100] = { 'a' };
   EXPECT_EQ(1, PlatformChannelRecvmsg(client_handle.get(), buf, sizeof(buf),
-                                      &handles));
+                                      &received_handles));
   EXPECT_EQ('\0', buf[0]);
-  ASSERT_TRUE(handles);
-  ASSERT_EQ(2u, handles->size());
-  EXPECT_FALSE((*handles)[0].is_valid());
-  EXPECT_TRUE((*handles)[1].is_valid());
+  ASSERT_EQ(2u, received_handles.size());
+  EXPECT_FALSE(received_handles[0].is_valid());
+  EXPECT_TRUE(received_handles[1].is_valid());
 
   {
     base::ScopedFILE fp(test::FILEFromPlatformHandle(
-        ScopedPlatformHandle((*handles)[1]), "rb"));
-    (*handles)[1] = PlatformHandle();
+        ScopedPlatformHandle(received_handles[1]), "rb"));
+    received_handles[1] = PlatformHandle();
     ASSERT_TRUE(fp);
     rewind(fp.get());
     char read_buf[100];

mojo/embedder/platform_channel_utils_posix.cc

@@ -105,10 +105,10 @@ bool PlatformChannelSendHandles(PlatformHandle h,
 ssize_t PlatformChannelRecvmsg(PlatformHandle h,
                                void* buf,
                                size_t num_bytes,
-                               ScopedPlatformHandleVectorPtr* handles) {
+                               std::deque<PlatformHandle>* platform_handles) {
   DCHECK(buf);
   DCHECK_GT(num_bytes, 0u);
-  DCHECK(handles);
+  DCHECK(platform_handles);
 
   struct iovec iov = { buf, num_bytes };
   char cmsg_buf[CMSG_SPACE(kPlatformChannelMaxNumHandles * sizeof(int))];
@@ -137,10 +137,8 @@ ssize_t PlatformChannelRecvmsg(PlatformHandle h,
       size_t num_fds = payload_length / sizeof(int);
       const int* fds = reinterpret_cast<int*>(CMSG_DATA(cmsg));
       for (size_t i = 0; i < num_fds; i++) {
-        if (!*handles)
-          (*handles).reset(new PlatformHandleVector());
-        (*handles)->push_back(PlatformHandle(fds[i]));
-        DCHECK((*handles)->back().is_valid());
+        platform_handles->push_back(PlatformHandle(fds[i]));
+        DCHECK(platform_handles->back().is_valid());
       }
     }
   }

mojo/embedder/platform_channel_utils_posix.h

@@ -8,11 +8,10 @@
 #include <stddef.h>
 #include <sys/types.h>  // For |ssize_t|.
 
-#include <vector>
+#include <deque>
 
 #include "base/memory/scoped_ptr.h"
 #include "mojo/embedder/platform_handle.h"
-#include "mojo/embedder/platform_handle_vector.h"
 #include "mojo/system/system_impl_export.h"
 
 struct iovec;  // Declared in <sys/uio.h>.
@@ -48,14 +47,13 @@ MOJO_SYSTEM_IMPL_EXPORT bool PlatformChannelSendHandles(PlatformHandle h,
                                                         size_t num_handles);
 
 // Wrapper around |recvmsg()|, which will extract any attached file descriptors
-// (in the control message) to |PlatformHandle|s. (This also handles |EINTR|.)
-// If |*handles| is null and handles are received, a vector will be allocated;
-// otherwise, any handles received will be appended to the existing vector.
+// (in the control message) to |PlatformHandle|s (and append them to
+// |platform_handles|). (This also handles |EINTR|.)
 MOJO_SYSTEM_IMPL_EXPORT ssize_t PlatformChannelRecvmsg(
     PlatformHandle h,
     void* buf,
     size_t num_bytes,
-    ScopedPlatformHandleVectorPtr* handles);
+    std::deque<PlatformHandle>* platform_handles);
 
 }  // namespace embedder
 }  // namespace mojo

mojo/embedder/platform_handle_vector.cc → mojo/embedder/platform_handle_utils.h

@@ -2,13 +2,20 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "mojo/embedder/platform_handle_vector.h"
+#ifndef MOJO_EMBEDDER_PLATFORM_HANDLE_UTILS_H_
+#define MOJO_EMBEDDER_PLATFORM_HANDLE_UTILS_H_
+
+#include "mojo/embedder/platform_handle.h"
+#include "mojo/system/system_impl_export.h"
 
 namespace mojo {
 namespace embedder {
 
-void CloseAllHandles(PlatformHandleVector* platform_handles) {
-  for (PlatformHandleVector::iterator it = platform_handles->begin();
+template <typename PlatformHandleContainer>
+MOJO_SYSTEM_IMPL_EXPORT inline void CloseAllPlatformHandles(
+    PlatformHandleContainer* platform_handles) {
+  for (typename PlatformHandleContainer::iterator it =
+           platform_handles->begin();
        it != platform_handles->end();
        ++it)
     it->CloseIfNecessary();
@@ -16,3 +23,5 @@ void CloseAllHandles(PlatformHandleVector* platform_handles) {
 
 }  // namespace embedder
 }  // namespace mojo
+
+#endif  // MOJO_EMBEDDER_PLATFORM_HANDLE_UTILS_H_

mojo/embedder/platform_handle_vector.h

@@ -9,6 +9,7 @@
 
 #include "base/memory/scoped_ptr.h"
 #include "mojo/embedder/platform_handle.h"
+#include "mojo/embedder/platform_handle_utils.h"
 #include "mojo/system/system_impl_export.h"
 
 namespace mojo {
@@ -16,14 +17,11 @@ namespace embedder {
 
 typedef std::vector<PlatformHandle> PlatformHandleVector;
 
-MOJO_SYSTEM_IMPL_EXPORT void CloseAllHandles(
-    PlatformHandleVector* platform_handles);
-
 // A deleter (for use with |scoped_ptr|) which closes all handles and then
 // |delete|s the |PlatformHandleVector|.
 struct MOJO_SYSTEM_IMPL_EXPORT PlatformHandleVectorDeleter {
   void operator()(PlatformHandleVector* platform_handles) const {
-    CloseAllHandles(platform_handles);
+    CloseAllPlatformHandles(platform_handles);
     delete platform_handles;
   }
 };

mojo/mojo.gyp

@@ -152,7 +152,7 @@
         'embedder/platform_channel_utils_posix.h',
         'embedder/platform_handle.cc',
         'embedder/platform_handle.h',
-        'embedder/platform_handle_vector.cc',
+        'embedder/platform_handle_utils.h',
         'embedder/platform_handle_vector.h',
         'embedder/scoped_platform_handle.h',
         'system/channel.cc',

mojo/system/raw_channel_posix.cc

@@ -9,6 +9,7 @@
 #include <unistd.h>
 
 #include <algorithm>
+#include <deque>
 
 #include "base/basictypes.h"
 #include "base/bind.h"
@@ -68,7 +69,7 @@ class RawChannelPosix : public RawChannel,
 
   bool pending_read_;
 
-  embedder::ScopedPlatformHandleVectorPtr read_platform_handles_;
+  std::deque<embedder::PlatformHandle> read_platform_handles_;
 
   // The following members are used on multiple threads and protected by
   // |write_lock()|:
@@ -102,6 +103,8 @@ RawChannelPosix::~RawChannelPosix() {
   // These must have been shut down/destroyed on the I/O thread.
   DCHECK(!read_watcher_.get());
   DCHECK(!write_watcher_.get());
+
+  embedder::CloseAllPlatformHandles(&read_platform_handles_);
 }
 
 size_t RawChannelPosix::GetSerializedPlatformHandleSize() const {
@@ -117,23 +120,31 @@ RawChannel::IOResult RawChannelPosix::Read(size_t* bytes_read) {
   size_t bytes_to_read = 0;
   read_buffer()->GetBuffer(&buffer, &bytes_to_read);
 
-  size_t old_num_platform_handles =
-      read_platform_handles_ ? read_platform_handles_->size() : 0;
+  size_t old_num_platform_handles = read_platform_handles_.size();
   ssize_t read_result =
       embedder::PlatformChannelRecvmsg(fd_.get(),
                                        buffer,
                                        bytes_to_read,
                                        &read_platform_handles_);
-  if (read_platform_handles_ &&
-      read_platform_handles_->size() > old_num_platform_handles) {
+  if (read_platform_handles_.size() > old_num_platform_handles) {
+    DCHECK_LE(read_platform_handles_.size() - old_num_platform_handles,
+              embedder::kPlatformChannelMaxNumHandles);
+
     if (read_result != 1) {
       LOG(WARNING) << "Invalid control message with platform handles";
       return IO_FAILED;
     }
 
-    if (read_platform_handles_->size() > TransportData::kMaxPlatformHandles) {
+    // We should never accumulate more than |TransportData::kMaxPlatformHandles
+    // + embedder::kPlatformChannelMaxNumHandles| handles. (The latter part is
+    // possible because we could have accumulated all the handles for a message,
+    // then received the message data plus the first set of handles for the next
+    // message in the subsequent |recvmsg()|.)
+    if (read_platform_handles_.size() > (TransportData::kMaxPlatformHandles +
+            embedder::kPlatformChannelMaxNumHandles)) {
       LOG(WARNING) << "Received too many platform handles";
-      read_platform_handles_.reset();
+      embedder::CloseAllPlatformHandles(&read_platform_handles_);
+      read_platform_handles_.clear();
       return IO_FAILED;
     }
 
@@ -173,13 +184,20 @@ embedder::ScopedPlatformHandleVectorPtr RawChannelPosix::GetReadPlatformHandles(
     const void* /*platform_handle_table*/) {
   DCHECK_GT(num_platform_handles, 0u);
 
-  if (!read_platform_handles_ ||
-      read_platform_handles_->size() != num_platform_handles) {
-    read_platform_handles_.reset();
+  if (read_platform_handles_.size() < num_platform_handles) {
+    embedder::CloseAllPlatformHandles(&read_platform_handles_);
+    read_platform_handles_.clear();
     return embedder::ScopedPlatformHandleVectorPtr();
   }
 
-  return read_platform_handles_.Pass();
+  embedder::ScopedPlatformHandleVectorPtr rv(
+      new embedder::PlatformHandleVector(num_platform_handles));
+  rv->assign(read_platform_handles_.begin(),
+             read_platform_handles_.begin() + num_platform_handles);
+  read_platform_handles_.erase(
+      read_platform_handles_.begin(),
+      read_platform_handles_.begin() + num_platform_handles);
+  return rv.Pass();
 }
 
 RawChannel::IOResult RawChannelPosix::WriteNoLock(