Change NaClIPCAdapter to save translated descriptors to the rewritten message.

chrome/nacl/nacl_ipc_adapter.cc

Index: chrome/nacl/nacl_ipc_adapter.cc
===================================================================
--- chrome/nacl/nacl_ipc_adapter.cc	(revision 148266)
+++ chrome/nacl/nacl_ipc_adapter.cc	(working copy)
@@ -141,8 +141,12 @@
   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() {}
@@ -153,6 +157,9 @@
   // 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()
@@ -173,9 +180,10 @@
   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)
@@ -183,6 +191,20 @@
 
   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);
 }
 
@@ -294,8 +316,6 @@
   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_);
@@ -305,14 +325,9 @@
     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;
@@ -339,15 +354,15 @@
 }
 #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;
@@ -357,41 +372,40 @@
         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());
       }
     }
   }
@@ -412,8 +426,7 @@
       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())
@@ -421,11 +434,12 @@
   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;
 }
 
@@ -495,7 +509,9 @@
   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.
@@ -506,10 +522,8 @@
   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());
 }