Refactor PickleIterator.

base/pickle.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 "base/pickle.h"
 
 #include <stdlib.h>
 
 #include <algorithm>  // for max()
 
 //------------------------------------------------------------------------------
 
 using base::char16;
 using base::string16;
 
 // static
 const int Pickle::kPayloadUnit = 64;
 
 static const size_t kCapacityReadOnly = static_cast<size_t>(-1);
 
 PickleIterator::PickleIterator(const Pickle& pickle)
-    : read_ptr_(pickle.payload()),
-      read_end_ptr_(pickle.end_of_payload()) {
+    : payload_ptr_(pickle.payload()),
+      read_index_(0),
+      end_index_(pickle.payload_size()) {
 }
 
 template <typename Type>
 inline bool PickleIterator::ReadBuiltinType(Type* result) {
   const char* read_from = GetReadPointerAndAdvance<Type>();
   if (!read_from)
     return false;
   if (sizeof(Type) > sizeof(uint32))
     memcpy(result, read_from, sizeof(*result));
   else
     *result = *reinterpret_cast<const Type*>(read_from);
   return true;
 }
 
+inline void PickleIterator::Advance(size_t size) {

[jar (doing other things), 2014/05/23 21:22:04]

I *think* all (both?) calls to this method pass in an aligned size (doing the
alignment at the call site).  It might be clearer to define this method to do
the alignment for the caller, and hence it becomes very clear that the
read_index remains aligned, until it matches end_index_.

[halyavin, 2014/05/26 07:43:02]

Done.

+  if (end_index_ - read_index_ < size) {
+    read_index_ = end_index_;
+  } else {
+    read_index_ += size;
+  }
+}
+
 template<typename Type>
 inline const char* PickleIterator::GetReadPointerAndAdvance() {
-  const char* current_read_ptr = read_ptr_;
-  if (read_ptr_ + sizeof(Type) > read_end_ptr_)
+  if (sizeof(Type) > end_index_ - read_index_)
     return NULL;

[jar (doing other things), 2014/05/23 21:22:04]

I agree that you perfectly translate the existing code here. It is interesting
that the old code didn't do the "Advance" part of the method name.

Do you think it is correct to leave read_index_ intact, or should we do:
read_index_ = end_index_;  // Advancing anyway.

The advantage(?) of that change is that it makes it harder (impossible?) to make
further use of the iterator.

[halyavin, 2014/05/26 07:43:02]

Looks reasonable. Let us see what happens.

-  if (sizeof(Type) < sizeof(uint32))
-    read_ptr_ += AlignInt(sizeof(Type), sizeof(uint32));
-  else
-    read_ptr_ += sizeof(Type);
+  const char* current_read_ptr = payload_ptr_ + read_index_;
+  Advance(AlignInt(sizeof(Type), sizeof(uint32_t)));

[jar (doing other things), 2014/05/23 21:22:04]

I think the precise translation of the old code would be:
if (sizeof(Type) < sizeof(int32))
  Advance(AlignInt(sizeof(Type), sizeof(uint32_t)));
else
  Advance(sizeof(Type));

I think you skipped the test because you are intentionally always doing the
AlignInt now.... 

[halyavin, 2014/05/26 07:43:02]

It was a bug since sizeof(Type) may not be divisible by 4. sizeof(struct { short
a; short b; short c;}) == 6 on Windows.

   return current_read_ptr;
 }
 
 const char* PickleIterator::GetReadPointerAndAdvance(int num_bytes) {
-  if (num_bytes < 0 || read_end_ptr_ - read_ptr_ < num_bytes)
+  if (num_bytes < 0 ||
+      end_index_ - read_index_ < static_cast<size_t>(num_bytes))
     return NULL;

[jar (doing other things), 2014/05/23 21:22:04]

Here again I *wonder* if we should advance, making the iterator unusable?

[halyavin, 2014/05/26 07:43:02]

Done.

-  const char* current_read_ptr = read_ptr_;
-  read_ptr_ += AlignInt(num_bytes, sizeof(uint32));
+  const char* current_read_ptr = payload_ptr_ + read_index_;
+  Advance(AlignInt(num_bytes, sizeof(uint32_t)));
   return current_read_ptr;
 }
 
 inline const char* PickleIterator::GetReadPointerAndAdvance(int num_elements,
                                                           size_t size_element) {

[jar (doing other things), 2014/05/23 21:22:04]

nit: While you're here.... all arguments should be aligned... so you should put
the argument from line 65 onto a new line, indented by 4, and then put this arg
directly under it.

[halyavin, 2014/05/26 07:43:02]

Done.

   // Check for int32 overflow.
   int64 num_bytes = static_cast<int64>(num_elements) * size_element;
   int num_bytes32 = static_cast<int>(num_bytes);
   if (num_bytes != static_cast<int64>(num_bytes32))
     return NULL;
   return GetReadPointerAndAdvance(num_bytes32);
 }
 
 bool PickleIterator::ReadBool(bool* result) {
   return ReadBuiltinType(result);
@@ skipping 255 matching lines @@
 #endif
   DCHECK_LE(write_offset_, kuint32max - data_len);
   size_t new_size = write_offset_ + data_len;
   if (new_size > capacity_after_header_) {
     Resize(std::max(capacity_after_header_ * 2, new_size));
   }
 
   char* write = mutable_payload() + write_offset_;
   memcpy(write, data, length);
   memset(write + length, 0, data_len - length);
-  header_->payload_size = static_cast<uint32>(write_offset_ + length);
+  header_->payload_size = static_cast<uint32>(new_size);

[jar (doing other things), 2014/05/23 21:22:04]

Would this change alone, which keeps payload_size aligned, have resolved the
issue with the iterator (getting an unaligned payload_size, and then going "well
beyond" the payload_size when we did align, thereby creating an invalid
pointer)?

[halyavin, 2014/05/26 07:43:02]

No, because the other side of IPC connection might be controlled by an attacker.

[jar (doing other things), 2014/05/27 23:29:34]

Isn't this the side that is writing into a pickle, for transmission.  Why does
it matter if the recipient (other end of the IPC) is an attacker??

It seems that if we intentionally keep payload_size aligned, a recipient (of the
pickle) can validate that fact (during iterator construction)... and never lose
this property (during iterations).

I think a pile of this CL related to having a payload size that was not
aligned... and then having to do backflips to deal with it clearly to avoid
having a pointer beyond an allocated region.

[halyavin, 2014/05/28 07:45:12]

It doesn't. I meant that we still need to check alignment on the receiving side,
if transmitting side is an attacker.
It is all concentrated in Advance method. We can easily remove it in the future.

One more thing I am worrying about is Chrome components which update
independently of Chrome (like PNaCl translator). If they contain an IPC or
Pickle code, they will break on a non-backward-compatible change.

[jar (doing other things), 2014/05/29 01:51:16]

FWIW: I checked... PNaCL uses its own IPC, and handles such issues separately
(no pickle involved).  

We always ship both side of the IPC in any upgrade... and I thought we were
completely safe (with a binary change of pickle format),... but Darin pointed
out that we persist history in a pickle format, and we'd need to edit special
code *IF* we made a binary change.  It is probably better not to bother with
such.

   write_offset_ = new_size;
 }