Refactor PickleIterator.

Remove undefined behaviors by using offsets instead of pointers
and by checking that offset never exceeds the size of the data.

Update Native Messaging test to use Native Messaging format
instead of Pickle format which accidentally coincided before.

BUG=none

Review URL: https://codereview.chromium.org/290173008

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@274367 0039d316-1c4b-4281-b951-d872f2087c98

4 files changed, 47 insertions, 29 deletions

diff --git a/base/pickle.cc b/base/pickle.cc
index 12a3237..a9e9a0f 100644
--- a/base/pickle.cc
+++ b/base/pickle.cc
@@ -19,8 +19,9 @@ 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>
@@ -35,28 +36,40 @@ inline bool PickleIterator::ReadBuiltinType(Type* result) {
   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
-    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(
+    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);
@@ -332,6 +345,6 @@ inline void Pickle::WriteBytesCommon(const void* data, size_t length) {
   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;
 }
diff --git a/base/pickle.h b/base/pickle.h
index dd34f54..70b8eef 100644
--- a/base/pickle.h
+++ b/base/pickle.h
@@ -20,13 +20,14 @@ class Pickle;
 // while the PickleIterator object is in use.
 class BASE_EXPORT PickleIterator {
  public:
-  PickleIterator() : read_ptr_(NULL), read_end_ptr_(NULL) {}
+  PickleIterator() : payload_(NULL), read_index_(0), end_index_(0) {}
   explicit PickleIterator(const Pickle& pickle);
 
   // Methods for reading the payload of the Pickle. To read from the start of
   // the Pickle, create a PickleIterator from a Pickle. If successful, these
   // methods return true. Otherwise, false is returned to indicate that the
-  // result could not be extracted.
+  // result could not be extracted. It is not possible to read from iterator
+  // after that.
   bool ReadBool(bool* result) WARN_UNUSED_RESULT;
   bool ReadInt(int* result) WARN_UNUSED_RESULT;
   bool ReadLong(long* result) WARN_UNUSED_RESULT;
@@ -61,11 +62,15 @@ class BASE_EXPORT PickleIterator {
 
   // Read Type from Pickle.
   template <typename Type>
-  inline bool ReadBuiltinType(Type* result);
+  bool ReadBuiltinType(Type* result);
+
+  // Advance read_index_ but do not allow it to exceed end_index_.
+  // Keeps read_index_ aligned.
+  void Advance(size_t size);
 
   // Get read pointer for Type and advance read pointer.
   template<typename Type>
-  inline const char* GetReadPointerAndAdvance();
+  const char* GetReadPointerAndAdvance();
 
   // Get read pointer for |num_bytes| and advance read pointer. This method
   // checks num_bytes for negativity and wrapping.
@@ -73,12 +78,12 @@ class BASE_EXPORT PickleIterator {
 
   // Get read pointer for (num_elements * size_element) bytes and advance read
   // pointer. This method checks for int overflow, negativity and wrapping.
-  inline const char* GetReadPointerAndAdvance(int num_elements,
-                                              size_t size_element);
+  const char* GetReadPointerAndAdvance(int num_elements,
+                                       size_t size_element);
 
-  // Pointers to the Pickle data.
-  const char* read_ptr_;
-  const char* read_end_ptr_;
+  const char* payload_;  // Start of our pickle's payload.
+  size_t read_index_;  // Offset of the next readable byte in payload.
+  size_t end_index_;  // Payload size.
 
   FRIEND_TEST_ALL_PREFIXES(PickleTest, GetReadPointerAndAdvance);
 };
@@ -277,7 +282,9 @@ class BASE_EXPORT Pickle {
   }
 
   // The payload is the pickle data immediately following the header.
-  size_t payload_size() const { return header_->payload_size; }
+  size_t payload_size() const {
+    return header_ ? header_->payload_size : 0;
+  }
 
   const char* payload() const {
     return reinterpret_cast<const char*>(header_) + header_size_;
diff --git a/base/pickle_unittest.cc b/base/pickle_unittest.cc
index b1c5925..bec25d2 100644
--- a/base/pickle_unittest.cc
+++ b/base/pickle_unittest.cc
@@ -262,7 +262,7 @@ TEST(PickleTest, Resize) {
 
   // one more byte should double the capacity
   pickle.WriteData(data_ptr, 1);
-  cur_payload += 5;
+  cur_payload += 8;
   EXPECT_EQ(unit * 4, pickle.capacity_after_header());
   EXPECT_EQ(cur_payload, pickle.payload_size());
 }
diff --git a/chrome/browser/extensions/api/messaging/native_message_process_host_unittest.cc b/chrome/browser/extensions/api/messaging/native_message_process_host_unittest.cc
index 1fa86db..81e811b 100644
--- a/chrome/browser/extensions/api/messaging/native_message_process_host_unittest.cc
+++ b/chrome/browser/extensions/api/messaging/native_message_process_host_unittest.cc
@@ -142,10 +142,8 @@ class NativeMessagingTest : public ::testing::Test,
 
  protected:
   std::string FormatMessage(const std::string& message) {
-    Pickle pickle;
-    pickle.WriteString(message);
-    return std::string(const_cast<const Pickle*>(&pickle)->payload(),
-                       pickle.payload_size());
+    uint32_t length = message.length();
+    return std::string(reinterpret_cast<char*>(&length), 4).append(message);
   }
 
   base::FilePath CreateTempFileWithMessage(const std::string& message) {