Change NaClIPCAdapter to save translated descriptors to the rewritten message.

chrome/nacl/nacl_ipc_adapter.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/nacl/nacl_ipc_adapter.h"
 
 #include <limits.h>
 #include <string.h>
 
 #include "base/basictypes.h"
@@ skipping 123 matching lines @@
 class NaClIPCAdapter::RewrittenMessage
     : public base::RefCounted<RewrittenMessage> {
  public:
   RewrittenMessage();
 
   bool is_consumed() const { return data_read_cursor_ == data_len_; }
 
   void SetData(const NaClIPCAdapter::NaClMessageHeader& header,
                const void* payload, size_t payload_length);
 
-  int Read(char* dest_buffer, size_t dest_buffer_size);
+  int TransferData(NaClImcTypedMsgHdr* msg);
+
+  void AddDescriptor(nacl::DescWrapper* desc) { descs_.push_back(desc); }
+
+  int desc_count() const { return static_cast<int>(descs_.size()); }

[dmichael (off chromium), 2012/07/25 16:00:40]

why not just use size_t for the return type?

[bbudge, 2012/07/25 16:54:14]

Done.

 
  private:
   friend class base::RefCounted<RewrittenMessage>;
   ~RewrittenMessage() {}
 
   scoped_array<char> data_;
   size_t data_len_;
 
   // Offset into data where the next read will happen. This will be equal to
   // data_len_ when all data has been consumed.
   size_t data_read_cursor_;
+
+  // Wrapped descriptors for transfer to untrusted code.
+  ScopedVector<nacl::DescWrapper> descs_;
 };
 
 NaClIPCAdapter::RewrittenMessage::RewrittenMessage()
     : data_len_(0),
       data_read_cursor_(0) {
 }
 
 void NaClIPCAdapter::RewrittenMessage::SetData(
     const NaClIPCAdapter::NaClMessageHeader& header,
     const void* payload,
     size_t payload_length) {
   DCHECK(!data_.get() && data_len_ == 0);
   size_t header_len = sizeof(NaClIPCAdapter::NaClMessageHeader);
   data_len_ = header_len + payload_length;
   data_.reset(new char[data_len_]);
 
   memcpy(data_.get(), &header, sizeof(NaClIPCAdapter::NaClMessageHeader));
   memcpy(&data_[header_len], payload, payload_length);
 }
 
-int NaClIPCAdapter::RewrittenMessage::Read(char* dest_buffer,
-                                           size_t dest_buffer_size) {
+int NaClIPCAdapter::RewrittenMessage::TransferData(NaClImcTypedMsgHdr* msg) {

[dmichael (off chromium), 2012/07/25 16:00:40]

I think I liked |Read| better for the name. It's not really transferring
anything at this point.

[bbudge, 2012/07/25 16:54:14]

I was finding 'Read' to be confusing. But 2 against 1. Done.

   CHECK(data_len_ >= data_read_cursor_);
+  char* dest_buffer = static_cast<char*>(msg->iov[0].base);
+  size_t dest_buffer_size = msg->iov[0].length;
   size_t bytes_to_write = std::min(dest_buffer_size,
                                    data_len_ - data_read_cursor_);
   if (bytes_to_write == 0)
     return 0;
 
   memcpy(dest_buffer, &data_[data_read_cursor_], bytes_to_write);
   data_read_cursor_ += bytes_to_write;
+
+  // Once all data has been consumed, transfer any file descriptors.
+  if (is_consumed()) {
+    nacl_abi_size_t desc_count = static_cast<nacl_abi_size_t>(descs_.size());
+    CHECK(desc_count <= msg->ndesc_length);
+    msg->ndesc_length = desc_count;
+    for (nacl_abi_size_t i = 0; i < desc_count; i++) {
+      // Copy the NaClDesc to the buffer and add a ref so it won't be freed
+      // when we clear our ScopedVector.
+      msg->ndescv[i] = descs_[i]->desc();
+      NaClDescRef(descs_[i]->desc());
+    }
+    descs_.clear();
+  }
   return static_cast<int>(bytes_to_write);
 }
 
 NaClIPCAdapter::LockedData::LockedData()
     : channel_closed_(false) {
 }
 
 NaClIPCAdapter::LockedData::~LockedData() {
 }
 
@@ skipping 91 matching lines @@
       // When the plugin gives us too much data, it's an error.
       ClearToBeSent();
       return -1;
   }
 }
 
 int NaClIPCAdapter::BlockingReceive(NaClImcTypedMsgHdr* msg) {
   if (msg->iov_length != 1)
     return -1;
 
-  char* output_buffer = static_cast<char*>(msg->iov[0].base);
-  size_t output_buffer_size = msg->iov[0].length;
   int retval = 0;
   {
     base::AutoLock lock(lock_);
     while (locked_data_.to_be_received_.empty() &&
            !locked_data_.channel_closed_)
       cond_var_.Wait();
     if (locked_data_.channel_closed_) {
       retval = -1;
     } else {
-      retval = LockedReceive(output_buffer, output_buffer_size);
+      retval = LockedReceive(msg);
       DCHECK(retval > 0);
     }
-    int desc_count = static_cast<int>(locked_data_.nacl_descs_.size());
-    CHECK(desc_count <= NACL_ABI_IMC_DESC_MAX);
-    msg->ndesc_length = desc_count;
-    for (int i = 0; i < desc_count; i++)
-      msg->ndescv[i] = locked_data_.nacl_descs_[i]->desc();
   }
   cond_var_.Signal();
   return retval;
 }
 
 void NaClIPCAdapter::CloseChannel() {
   {
     base::AutoLock lock(lock_);
     locked_data_.channel_closed_ = true;
   }
   cond_var_.Signal();
 
   task_runner_->PostTask(FROM_HERE,
       base::Bind(&NaClIPCAdapter::CloseChannelOnIOThread, this));
 }
 
 NaClDesc* NaClIPCAdapter::MakeNaClDesc() {
   return MakeNaClDescCustom(this);
 }
 
 #if defined(OS_POSIX)
 int NaClIPCAdapter::TakeClientFileDescriptor() {
   return io_thread_data_.channel_->TakeClientFileDescriptor();
 }
 #endif
 
-bool NaClIPCAdapter::OnMessageReceived(const IPC::Message& message) {
+bool NaClIPCAdapter::OnMessageReceived(const IPC::Message& msg) {
   {
     base::AutoLock lock(lock_);
 
-    // Clear any descriptors left from the prior message.
-    locked_data_.nacl_descs_.clear();
+    scoped_refptr<RewrittenMessage> rewritten_msg(new RewrittenMessage);
+    locked_data_.to_be_received_.push(rewritten_msg);

[dmichael (off chromium), 2012/07/25 16:00:40]

Should this part maybe be at the end of SaveMessage? If we end up with any kind
of error conditions below, it seems like it would be better to drop the message
than send it incomplete.

[bbudge, 2012/07/25 16:54:14]

Good idea. Done.

 
-    PickleIterator it(message);
-    switch (message.type()) {
+    PickleIterator it(msg);
+    switch (msg.type()) {
       case PpapiMsg_PPBAudio_NotifyAudioStreamCreated::ID: {
         int instance_id;
         int resource_id;
         int result_code;
         NaClHandle sock_handle;
         NaClHandle shm_handle;
         uint32_t shm_length;
         if (ReadHostResource(&it, &instance_id, &resource_id) &&
             it.ReadInt(&result_code) &&
-            ReadFileDescriptor(message, &it, &sock_handle) &&
-            ReadFileDescriptor(message, &it, &shm_handle) &&
+            ReadFileDescriptor(msg, &it, &sock_handle) &&
+            ReadFileDescriptor(msg, &it, &shm_handle) &&
             it.ReadUInt32(&shm_length)) {
-          // Our caller, OnMessageReceived, holds the lock for locked_data_.
-          // Import the sync socket. Use DescWrappers to simplify clean up.
+          // Import the sync socket.
           nacl::DescWrapperFactory factory;
           scoped_ptr<nacl::DescWrapper> socket_wrapper(
               factory.ImportSyncSocketHandle(sock_handle));
           // Import the shared memory handle and increase its size by 4 bytes to
-          // accommodate the length data we write to signal the host.
+          // accommodate the length data we write at the end to signal the host.
           scoped_ptr<nacl::DescWrapper> shm_wrapper(
               factory.ImportShmHandle(shm_handle, shm_length + sizeof(uint32)));
           if (shm_wrapper.get() && socket_wrapper.get()) {
-            locked_data_.nacl_descs_.push_back(socket_wrapper.release());
-            locked_data_.nacl_descs_.push_back(shm_wrapper.release());
+            rewritten_msg->AddDescriptor(socket_wrapper.release());
+            rewritten_msg->AddDescriptor(shm_wrapper.release());
           }
 #if defined(OS_POSIX)
-          SaveMessage(message);
-#else  // defined(OS_POSIX)
-          // On Windows we must rewrite the message to the POSIX representation.
-          IPC::Message new_msg(message.routing_id(),
+          SaveMessage(msg, rewritten_msg.get());
+#else
+          // On Windows we must rewrite the message to match the POSIX form.
+          IPC::Message new_msg(msg.routing_id(),
                                PpapiMsg_PPBAudio_NotifyAudioStreamCreated::ID,
-                               message.priority());
+                               msg.priority());
           WriteHostResource(&new_msg, instance_id, resource_id);
           new_msg.WriteInt(result_code);
           WriteFileDescriptor(&new_msg, 0);  // socket handle, index = 0
           WriteFileDescriptor(&new_msg, 1);  // shm handle, index = 1
           new_msg.WriteUInt32(shm_length);
-          SaveMessage(new_msg);
+          SaveMessage(new_msg, rewritten_msg.get());
 #endif
         }
         break;
       }
       default: {
-        SaveMessage(message);
+        SaveMessage(msg, rewritten_msg.get());
       }
     }
   }
   cond_var_.Signal();
   return true;
 }
 
 void NaClIPCAdapter::OnChannelConnected(int32 peer_pid) {
 }
 
 void NaClIPCAdapter::OnChannelError() {
   CloseChannel();
 }
 
 NaClIPCAdapter::~NaClIPCAdapter() {
   // Make sure the channel is deleted on the IO thread.
   task_runner_->PostTask(FROM_HERE,
       base::Bind(&DeleteChannel, io_thread_data_.channel_.release()));
 }
 
-int NaClIPCAdapter::LockedReceive(char* output_buffer,
-                                  size_t output_buffer_size) {
+int NaClIPCAdapter::LockedReceive(NaClImcTypedMsgHdr* msg) {
   lock_.AssertAcquired();
 
   if (locked_data_.to_be_received_.empty())
     return 0;
   scoped_refptr<RewrittenMessage> current =
       locked_data_.to_be_received_.front();
 
-  int retval = current->Read(output_buffer, output_buffer_size);
+  int retval = current->TransferData(msg);
 
   // When a message is entirely consumed, remove if from the waiting queue.
   if (current->is_consumed())
     locked_data_.to_be_received_.pop();
+
   return retval;
 }
 
 bool NaClIPCAdapter::SendCompleteMessage(const char* buffer,
                                          size_t buffer_len) {
   // The message will have already been validated, so we know it's large enough
   // for our header.
   const NaClMessageHeader* header =
       reinterpret_cast<const NaClMessageHeader*>(buffer);
 
@@ skipping 49 matching lines @@
 }
 
 void NaClIPCAdapter::CloseChannelOnIOThread() {
   io_thread_data_.channel_->Close();
 }
 
 void NaClIPCAdapter::SendMessageOnIOThread(scoped_ptr<IPC::Message> message) {
   io_thread_data_.channel_->Send(message.release());
 }
 
-void NaClIPCAdapter::SaveMessage(const IPC::Message& message) {
+void NaClIPCAdapter::SaveMessage(const IPC::Message& message,
+                                 RewrittenMessage* rewritten_message) {
+  lock_.AssertAcquired();
   // There is some padding in this structure (the "padding" member is 16
   // bits but this then gets padded to 32 bits). We want to be sure not to
   // leak data to the untrusted plugin, so zero everything out first.
   NaClMessageHeader header;
   memset(&header, 0, sizeof(NaClMessageHeader));
 
   header.payload_size = static_cast<uint32>(message.payload_size());
   header.routing = message.routing_id();
   header.type = message.type();
   header.flags = message.flags();
-  header.num_fds = static_cast<int>(locked_data_.nacl_descs_.size());
+  header.num_fds = rewritten_message->desc_count();
 
-  scoped_refptr<RewrittenMessage> dest(new RewrittenMessage);
-  dest->SetData(header, message.payload(), message.payload_size());
-  locked_data_.to_be_received_.push(dest);
+  rewritten_message->SetData(header, message.payload(), message.payload_size());
 }