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));
     }
   }
 };