POSIX: Clean up DescriptorSet

In general, the IPC Message objects are const and the iterator state is a void*
kept by the code which is doing the deserialisation.

However, now that we have an array of file descriptors, the index of the next
file descriptor is part of the iteration state and is kept /inside/ the Message
(in the DescriptorSet). This means that it's a mutable member, which is never
too nice and also, since the logging functions want to parse a message multiple
times, we've had to turn off returning an error when one runs off the end of
the array.

Also, the Message copy constructor and assignment function alters the /source/
Message, by stealing all of its descriptors

This patch encodes the index of each file descriptor on the wire. So the state
is moved from inside the DescriptorSet into the serialised data.

Additionally, the DescriptorSet is made into a lazyily created scoped_refptr,
solving the problems with copying messages.

Review URL: http://codereview.chromium.org/20275


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

6 files changed, 143 insertions, 63 deletions

diff --git a/chrome/common/descriptor_set_posix.cc b/chrome/common/descriptor_set_posix.cc
index 82f2899..9b19ef2 100644
--- a/chrome/common/descriptor_set_posix.cc
+++ b/chrome/common/descriptor_set_posix.cc
@@ -7,11 +7,11 @@
 #include "base/logging.h"
 
 DescriptorSet::DescriptorSet()
-    : next_descriptor_(0) {
+    : consumed_descriptor_highwater_(0) {
 }
 
 DescriptorSet::~DescriptorSet() {
-  if (next_descriptor_ == descriptors_.size())
+  if (consumed_descriptor_highwater_ == descriptors_.size())
     return;
 
   LOG(WARNING) << "DescriptorSet destroyed with unconsumed descriptors";
@@ -20,10 +20,11 @@ DescriptorSet::~DescriptorSet() {
   // flags set mirrors the expected behaviour.
   //
   // If this message was received with more descriptors than expected
-  // (which could a DOS against the browser by a rouge renderer) then all
+  // (which could a DOS against the browser by a rogue renderer) then all
   // the descriptors have their close flag set and we free all the extra
   // kernel resources.
-  for (unsigned i = next_descriptor_; i < descriptors_.size(); ++i) {
+  for (unsigned i = consumed_descriptor_highwater_;
+       i < descriptors_.size(); ++i) {
     if (descriptors_[i].auto_close)
       close(descriptors_[i].fd);
   }
@@ -52,16 +53,40 @@ bool DescriptorSet::AddAndAutoClose(int fd) {
   return true;
 }
 
-int DescriptorSet::NextDescriptor() {
-  if (next_descriptor_ == descriptors_.size())
+int DescriptorSet::GetDescriptorAt(unsigned index) const {
+  if (index >= descriptors_.size())
     return -1;
 
-  return descriptors_[next_descriptor_++].fd;
+  // We should always walk the descriptors in order, so it's reasonable to
+  // enforce this. Consider the case where a compromised renderer sends us
+  // the following message:
+  //
+  //   ExampleMsg:
+  //     num_fds:2 msg:FD(index = 1) control:SCM_RIGHTS {n, m}
+  //
+  // Here the renderer sent us a message which should have a descriptor, but
+  // actually sent two in an attempt to fill our fd table and kill us. By
+  // setting the index of the descriptor in the message to 1 (it should be
+  // 0), we would record a highwater of 1 and then consider all the
+  // descriptors to have been used.
+  //
+  // So we can either track of the use of each descriptor in a bitset, or we
+  // can enforce that we walk the indexes strictly in order.
+  //
+  // There's one more wrinkle: When logging messages, we may reparse them. So
+  // we have an exception: When the consumed_descriptor_highwater_ is at the
+  // end of the array and index 0 is requested, we reset the highwater value.
+  if (index == 0 && consumed_descriptor_highwater_ == descriptors_.size())
+    consumed_descriptor_highwater_ = 0;
+
+  if (index != consumed_descriptor_highwater_)
+    return -1;
+
+  consumed_descriptor_highwater_ = index + 1;
+  return descriptors_[index].fd;
 }
 
 void DescriptorSet::GetDescriptors(int* buffer) const {
-  DCHECK_EQ(next_descriptor_, 0u);
-
   for (std::vector<base::FileDescriptor>::const_iterator
        i = descriptors_.begin(); i != descriptors_.end(); ++i) {
     *(buffer++) = i->fd;
@@ -75,12 +100,13 @@ void DescriptorSet::CommitAll() {
       close(i->fd);
   }
   descriptors_.clear();
-  next_descriptor_ = 0;
+  consumed_descriptor_highwater_ = 0;
 }
 
 void DescriptorSet::SetDescriptors(const int* buffer, unsigned count) {
-  DCHECK(count <= MAX_DESCRIPTORS_PER_MESSAGE);
-  DCHECK(descriptors_.size() == 0);
+  DCHECK_LE(count, MAX_DESCRIPTORS_PER_MESSAGE);
+  DCHECK_EQ(descriptors_.size(), 0u);
+  DCHECK_EQ(consumed_descriptor_highwater_, 0u);
 
   descriptors_.reserve(count);
   for (unsigned i = 0; i < count; ++i) {
@@ -90,11 +116,3 @@ void DescriptorSet::SetDescriptors(const int* buffer, unsigned count) {
     descriptors_.push_back(sd);
   }
 }
-
-void DescriptorSet::TakeFrom(DescriptorSet* other) {
-  DCHECK(descriptors_.size() == 0);
-
-  descriptors_.swap(other->descriptors_);
-  next_descriptor_ = other->next_descriptor_;
-  other->next_descriptor_ = 0;
-}
diff --git a/chrome/common/descriptor_set_posix.h b/chrome/common/descriptor_set_posix.h
index 36a0433..1757648 100644
--- a/chrome/common/descriptor_set_posix.h
+++ b/chrome/common/descriptor_set_posix.h
@@ -9,13 +9,14 @@
 
 #include "base/basictypes.h"
 #include "base/file_descriptor_posix.h"
+#include "base/ref_counted.h"
 
 // -----------------------------------------------------------------------------
 // A DescriptorSet is an ordered set of POSIX file descriptors. These are
 // associated with IPC messages so that descriptors can be transmitted over a
 // UNIX domain socket.
 // -----------------------------------------------------------------------------
-class DescriptorSet {
+class DescriptorSet : public base::RefCountedThreadSafe<DescriptorSet> {
  public:
   DescriptorSet();
   ~DescriptorSet();
@@ -47,14 +48,18 @@ class DescriptorSet {
   // ---------------------------------------------------------------------------
   // Interfaces for accessing during message deserialisation...
 
-  // Return the number of descriptors remaining
-  unsigned size() const { return descriptors_.size() - next_descriptor_; }
+  // Return the number of descriptors
+  unsigned size() const { return descriptors_.size(); }
   // Return true if no unconsumed descriptors remain
-  bool empty() const { return descriptors_.size() == next_descriptor_; }
-  // Fetch the next descriptor from the beginning of the set. This interface is
-  // designed for the deserialising code as it doesn't support close flags.
+  bool empty() const { return descriptors_.empty(); }
+  // Fetch the nth descriptor from the beginning of the set. Code using this
+  // /must/ access the descriptors in order, except that it may wrap from the
+  // end to index 0 again.
+  //
+  // This interface is designed for the deserialising code as it doesn't
+  // support close flags.
   //   returns: file descriptor, or -1 on error
-  int NextDescriptor();
+  int GetDescriptorAt(unsigned n) const;
 
   // ---------------------------------------------------------------------------
 
@@ -84,24 +89,18 @@ class DescriptorSet {
 
   // ---------------------------------------------------------------------------
 
-  // ---------------------------------------------------------------------------
-  // Interfaces for IPC::Message...
-
-  // Take all the FileDescriptors from another set. Just like a copy
-  // constructor, except that the source is emptied.
-  void TakeFrom(DescriptorSet* other);
-
-  // ---------------------------------------------------------------------------
-
  private:
   // A vector of descriptors and close flags. If this message is sent, then
   // these descriptors are sent as control data. After sending, any descriptors
   // with a true flag are closed. If this message has been received, then these
   // are the descriptors which were received and all close flags are true.
   std::vector<base::FileDescriptor> descriptors_;
-  // When deserialising the message, the descriptors will be extracted
-  // one-by-one. This contains the index of the next unused descriptor.
-  unsigned next_descriptor_;
+
+  // This contains the index of the next descriptor which should be consumed.
+  // It's used in a couple of ways. Firstly, at destruction we can check that
+  // all the descriptors have been read (with GetNthDescriptor). Secondly, we
+  // can check that they are read in order.
+  mutable unsigned consumed_descriptor_highwater_;
 
   DISALLOW_COPY_AND_ASSIGN(DescriptorSet);
 };
diff --git a/chrome/common/ipc_channel_posix.cc b/chrome/common/ipc_channel_posix.cc
index b46024c..72d1554 100644
--- a/chrome/common/ipc_channel_posix.cc
+++ b/chrome/common/ipc_channel_posix.cc
@@ -483,13 +483,24 @@ bool Channel::ChannelImpl::ProcessIncomingMessages() {
       const char* message_tail = Message::FindNext(p, end);
       if (message_tail) {
         int len = static_cast<int>(message_tail - p);
-        const Message m(p, len);
+        Message m(p, len);
         if (m.header()->num_fds) {
           // the message has file descriptors
+          const char* error = NULL;
           if (m.header()->num_fds > num_fds - fds_i) {
             // the message has been completely received, but we didn't get
             // enough file descriptors.
-            LOG(WARNING) << "Message needs unreceived descriptors"
+            error = "Message needs unreceived descriptors";
+          }
+
+          if (m.header()->num_fds >
+              DescriptorSet::MAX_DESCRIPTORS_PER_MESSAGE) {
+            // There are too many descriptors in this message
+            error = "Message requires an excessive number of descriptors";
+          }
+
+          if (error) {
+            LOG(WARNING) << error
                          << " channel:" << this
                          << " message-type:" << m.type()
                          << " header()->num_fds:" << m.header()->num_fds
@@ -499,6 +510,7 @@ bool Channel::ChannelImpl::ProcessIncomingMessages() {
             for (unsigned i = fds_i; i < num_fds; ++i)
               close(fds[i]);
             input_overflow_fds_.clear();
+            // abort the connection
             return false;
           }
 
diff --git a/chrome/common/ipc_message.cc b/chrome/common/ipc_message.cc
index 848f8bc..9a15f1c 100644
--- a/chrome/common/ipc_message.cc
+++ b/chrome/common/ipc_message.cc
@@ -7,6 +7,10 @@
 #include "base/logging.h"
 #include "build/build_config.h"
 
+#if defined(OS_POSIX)
+#include "chrome/common/descriptor_set_posix.h"
+#endif
+
 namespace IPC {
 
 //------------------------------------------------------------------------------
@@ -41,7 +45,7 @@ Message::Message(const char* data, int data_len) : Pickle(data, data_len) {
 Message::Message(const Message& other) : Pickle(other) {
   InitLoggingVariables();
 #if defined(OS_POSIX)
-  descriptor_set_.TakeFrom(&other.descriptor_set_);
+  descriptor_set_ = other.descriptor_set_;
 #endif
 }
 
@@ -56,7 +60,7 @@ void Message::InitLoggingVariables() {
 Message& Message::operator=(const Message& other) {
   *static_cast<Pickle*>(this) = other;
 #if defined(OS_POSIX)
-  descriptor_set_.TakeFrom(&other.descriptor_set_);
+  descriptor_set_ = other.descriptor_set_;
 #endif
   return *this;
 }
@@ -82,5 +86,40 @@ void Message::set_received_time(int64 time) const {
 }
 #endif
 
+#if defined(OS_POSIX)
+bool Message::WriteFileDescriptor(const base::FileDescriptor& descriptor) {
+  // We write the index of the descriptor so that we don't have to
+  // keep the current descriptor as extra decoding state when deserialising.
+  WriteInt(descriptor_set()->size());
+  if (descriptor.auto_close) {
+    return descriptor_set()->AddAndAutoClose(descriptor.fd);
+  } else {
+    return descriptor_set()->Add(descriptor.fd);
+  }
+}
+
+bool Message::ReadFileDescriptor(void** iter,
+                                base::FileDescriptor* descriptor) const {
+  int descriptor_index;
+  if (!ReadInt(iter, &descriptor_index))
+    return false;
+
+  DescriptorSet* descriptor_set = descriptor_set_.get();
+  if (!descriptor_set)
+    return false;
+
+  descriptor->fd = descriptor_set->GetDescriptorAt(descriptor_index);
+  descriptor->auto_close = false;
+
+  return descriptor->fd >= 0;
+}
+
+void Message::EnsureDescriptorSet() {
+  if (descriptor_set_.get() == NULL)
+    descriptor_set_ = new DescriptorSet;
+}
+
+#endif
+
 }  // namespace IPC
 
diff --git a/chrome/common/ipc_message.h b/chrome/common/ipc_message.h
index 0601ed6..b670bf1 100644
--- a/chrome/common/ipc_message.h
+++ b/chrome/common/ipc_message.h
@@ -16,9 +16,15 @@
 #endif
 
 #if defined(OS_POSIX)
-#include "chrome/common/descriptor_set_posix.h"
+#include "base/ref_counted.h"
 #endif
 
+namespace base {
+class FileDescriptor;
+}
+
+class DescriptorSet;
+
 namespace IPC {
 
 //------------------------------------------------------------------------------
@@ -161,7 +167,14 @@ class Message : public Pickle {
   }
 
 #if defined(OS_POSIX)
-  DescriptorSet* descriptor_set() const { return &descriptor_set_; }
+  // On POSIX, a message supports reading / writing FileDescriptor objects.
+  // This is used to pass a file descriptor to the peer of an IPC channel.
+
+  // Add a descriptor to the end of the set. Returns false iff the set is full.
+  bool WriteFileDescriptor(const base::FileDescriptor& descriptor);
+  // Get a file descriptor from the message. Returns false on error.
+  //   iter: a Pickle iterator to the current location in the message.
+  bool ReadFileDescriptor(void** iter, base::FileDescriptor* descriptor) const;
 #endif
 
 #ifdef IPC_MESSAGE_LOG_ENABLED
@@ -226,7 +239,18 @@ class Message : public Pickle {
 
 #if defined(OS_POSIX)
   // The set of file descriptors associated with this message.
-  mutable DescriptorSet descriptor_set_;
+  scoped_refptr<DescriptorSet> descriptor_set_;
+
+  // Ensure that a DescriptorSet is allocated
+  void EnsureDescriptorSet();
+
+  DescriptorSet* descriptor_set() {
+    EnsureDescriptorSet();
+    return descriptor_set_.get();
+  }
+  const DescriptorSet* descriptor_set() const {
+    return descriptor_set_.get();
+  }
 #endif
 
 #ifdef IPC_MESSAGE_LOG_ENABLED
diff --git a/chrome/common/ipc_message_utils.h b/chrome/common/ipc_message_utils.h
index a87a594..fa65288 100644
--- a/chrome/common/ipc_message_utils.h
+++ b/chrome/common/ipc_message_utils.h
@@ -671,29 +671,17 @@ template<>
 struct ParamTraits<base::FileDescriptor> {
   typedef base::FileDescriptor param_type;
   static void Write(Message* m, const param_type& p) {
-    if (p.auto_close) {
-      if (!m->descriptor_set()->AddAndAutoClose(p.fd))
-        NOTREACHED();
-    } else {
-      if (!m->descriptor_set()->Add(p.fd))
-        NOTREACHED();
-    }
+    if (!m->WriteFileDescriptor(p))
+      NOTREACHED();
   }
   static bool Read(const Message* m, void** iter, param_type* r) {
-    r->auto_close = false;
-    r->fd = m->descriptor_set()->NextDescriptor();
-
-    // We always return true here because some of the IPC message logging
-    // functions want to parse the message multiple times. On the second and
-    // later attempts, the descriptor_set will be empty and so will return -1,
-    // however, failing to parse at log time is a fatal error.
-    return true;
+    return m->ReadFileDescriptor(iter, r);
   }
   static void Log(const param_type& p, std::wstring* l) {
     if (p.auto_close) {
-      l->append(L"FD(auto-close)");
+      l->append(StringPrintf(L"FD(%d auto-close)", p.fd));
     } else {
-      l->append(L"FD");
+      l->append(StringPrintf(L"FD(%d)", p.fd));
     }
   }
 };