Avoid undefined behavior when checking for pointer wraparound

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()
 
@@ skipping 17 matching lines @@
   if (sizeof(Type) > sizeof(uint32))
     memcpy(result, read_from, sizeof(*result));
   else
     *result = *reinterpret_cast<const Type*>(read_from);
   return true;
 }
 
 template<typename Type>
 inline const char* PickleIterator::GetReadPointerAndAdvance() {
   const char* current_read_ptr = read_ptr_;
-  if (read_ptr_ + sizeof(Type) > read_end_ptr_)
+  size_t s;

[jar (doing other things), 2013/03/04 19:25:51]

nit: style guide suggests avoiding single character names.

Suggest:

size_t type_size;

+  if (sizeof(Type) < sizeof(uint32))
+    s = AlignInt(sizeof(Type), sizeof(uint32));

[jar (doing other things), 2013/03/04 19:25:51]

Am I correct in reading AlignInt(smaller, larger) as being equal to larger?

So this is just:
s = sizeof(uint32);

IF so, we may as well write line 38:
size_t = std::max(sizeof(Type), sizeof(uint32));

and toss lines 39-42.  It would be much clearer.

Am I missing anything?

+  else
+    s = sizeof(Type);
+  if (s > static_cast<size_t>(read_end_ptr_ - read_ptr_))
     return NULL;
-  if (sizeof(Type) < sizeof(uint32))
-    read_ptr_ += AlignInt(sizeof(Type), sizeof(uint32));
-  else
-    read_ptr_ += sizeof(Type);
+  read_ptr_ += s;
   return current_read_ptr;
 }
 
 const char* PickleIterator::GetReadPointerAndAdvance(int num_bytes) {
   if (num_bytes < 0 || read_end_ptr_ - read_ptr_ < num_bytes)
     return NULL;
   const char* current_read_ptr = read_ptr_;
   read_ptr_ += AlignInt(num_bytes, sizeof(uint32));
   return current_read_ptr;
 }
@@ skipping 292 matching lines @@
                              const char* start,
                              const char* end) {
   DCHECK_EQ(header_size, AlignInt(header_size, sizeof(uint32)));
   DCHECK_LE(header_size, static_cast<size_t>(kPayloadUnit));
 
   if (static_cast<size_t>(end - start) < sizeof(Header))
     return NULL;
 
   const Header* hdr = reinterpret_cast<const Header*>(start);
   const char* payload_base = start + header_size;
-  const char* payload_end = payload_base + hdr->payload_size;
-  if (payload_end < payload_base)
+  if (hdr->payload_size > static_cast<size_t>(end - payload_base))
     return NULL;
 
-  return (payload_end > end) ? NULL : payload_end;
+  return payload_base + hdr->payload_size;
 }