Fix crash in NaClIPCAdapter where incomplete messages incorrectly

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 242 matching lines @@
   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;

[Mark Seaborn, 2012/09/19 23:25:14]

It's kind of odd that your new ndesc_length assignment is far away from this
one...

[bbudge, 2012/09/19 23:46:09]

Very odd.

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

[Mark Seaborn, 2012/09/19 23:25:14]

...is there a reason you don't do the following here:

else {
  msg->ndesc_length = 0;
}

(I haven't looked at this code closely enough to know the answer.)

[bbudge, 2012/09/19 23:46:09]

You're right, this is a much better place. Done.

   return static_cast<int>(bytes_to_write);
 }
 
 NaClIPCAdapter::LockedData::LockedData()
     : channel_closed_(false) {
 }
 
 NaClIPCAdapter::LockedData::~LockedData() {
 }
 
@@ skipping 291 matching lines @@
   if (locked_data_.to_be_received_.empty())
     return 0;
   scoped_refptr<RewrittenMessage> current =
       locked_data_.to_be_received_.front();
 
   int retval = current->Read(msg);
 
   // When a message is entirely consumed, remove if from the waiting queue.
   if (current->is_consumed())
     locked_data_.to_be_received_.pop();
+  else
+    msg->ndesc_length = 0;

[dmichael (off chromium), 2012/09/19 22:47:48]

Would it be good to either add a comment or move this to Read(), so that it's
obvious why we need to set to 0? Here, it's less obvious that we only send the
descriptors with the last fragment of the message.

[bbudge, 2012/09/19 23:46:09]

Done.

[Mark Seaborn, 2012/09/20 00:28:42]

Did you mean to re-upload?  I just see the original change.

 
   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 75 matching lines @@
   header.payload_size = static_cast<uint32>(msg.payload_size());
   header.routing = msg.routing_id();
   header.type = msg.type();
   header.flags = msg.flags();
   header.num_fds = static_cast<int>(rewritten_msg->desc_count());
 
   rewritten_msg->SetData(header, msg.payload(), msg.payload_size());
   locked_data_.to_be_received_.push(rewritten_msg);
 }