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_(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) {
+  size_t aligned_size = AlignInt(size, sizeof(uint32_t));
+  if (end_index_ - read_index_ < aligned_size) {
+    read_index_ = end_index_;
+  } else {
+    read_index_ += aligned_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_) {
+    read_index_ = end_index_;
     return NULL;
-  if (sizeof(Type) < sizeof(uint32))
-    read_ptr_ += AlignInt(sizeof(Type), sizeof(uint32));
-  else

[cpu_(ooo_6.6-7.5), 2014/05/29 17:14:58]

the two versions do not seem equivalent. Maybe I am misreading it but the
sizeof(Type) is no longer compared to sizeof(uint32).

Looks like we are going to get different alignments, maybe not a problem per se.

[halyavin, 2014/05/29 17:20:26]

It is intentional. The previous code was wrong.

-    read_ptr_ += sizeof(Type);
+  }
+  const char* current_read_ptr = payload_ + read_index_;
+  Advance(sizeof(Type));
   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)) {
+    read_index_ = end_index_;
     return NULL;
-  const char* current_read_ptr = read_ptr_;
-  read_ptr_ += AlignInt(num_bytes, sizeof(uint32));
+  }
+  const char* current_read_ptr = payload_ + read_index_;
+  Advance(num_bytes);
   return current_read_ptr;
 }
 
-inline const char* PickleIterator::GetReadPointerAndAdvance(int num_elements,
-                                                          size_t size_element) {
+inline const char* PickleIterator::GetReadPointerAndAdvance(

[jschuh, 2014/05/30 13:50:31]

Nit: Not security related, but does the explicit inline really make sense here?

[halyavin, 2014/05/30 15:20:24]

I removed inline from *.h file. I leave it here so that it can be inlined in
internal methods.

+    int num_elements,
+    size_t size_element) {
   // 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);
   write_offset_ = new_size;
 }