Add compile time checks against longs being used in IPC structs on 32 bit Android.

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()
 #include <limits>
 
 #include "base/bits.h"
 #include "base/macros.h"
+#include "base/numerics/safe_conversions.h"
 #include "build/build_config.h"
 
 namespace base {
 
 // static
 const int Pickle::kPayloadUnit = 64;
 
 static const size_t kCapacityReadOnly = static_cast<size_t>(-1);
 
 PickleIterator::PickleIterator(const Pickle& pickle)
@@ skipping 58 matching lines @@
 
 bool PickleIterator::ReadBool(bool* result) {
   return ReadBuiltinType(result);
 }
 
 bool PickleIterator::ReadInt(int* result) {
   return ReadBuiltinType(result);
 }
 
 bool PickleIterator::ReadLong(long* result) {
-  return ReadBuiltinType(result);
+  // Always read long as a 64-bit value to ensure compatibility between 32-bit
+  // and 64-bit processes.
+  int64_t result_int64 = 0;
+  if (!ReadBuiltinType(&result_int64))
+    return false;
+  // CHECK if the cast truncates the value so that we know to change this IPC
+  // parameter to use int64_t.
+  *result = base::checked_cast<long>(result_int64);

[Tom Sepez, 2016/01/27 18:23:20]

Maybe we return FALSE if the value is too big to fit rather than faulting.  We
could then handle this case along the lines of any other failure to deserialize.

[jam, 2016/01/27 18:42:55]

good question: i wanted to avoid this because it leads to silent failures. i.e.
the processes might not crash, but a feature now isn't working correctly. it's
not the fault of developers, since it's not clear that when they sent a long (or
other types that are typedef'd to it) that there's a 32<>64 bit case where
overflow might happen. In that case, getting a crash (hopefully on our bots, but
if not, then on canary/dev/beta and hopefully not stable!) will be a good signal
for us to fix the code to use a type that won't overflow between 32 and 64 bits.

+  return true;
 }
 
 bool PickleIterator::ReadUInt16(uint16_t* result) {
   return ReadBuiltinType(result);
 }
 
 bool PickleIterator::ReadUInt32(uint32_t* result) {
   return ReadBuiltinType(result);
 }
 
 bool PickleIterator::ReadInt64(int64_t* result) {
   return ReadBuiltinType(result);
 }
 
 bool PickleIterator::ReadUInt64(uint64_t* result) {
   return ReadBuiltinType(result);
 }
 
 bool PickleIterator::ReadSizeT(size_t* result) {
   // Always read size_t as a 64-bit value to ensure compatibility between 32-bit
   // and 64-bit processes.
   uint64_t result_uint64 = 0;
-  bool success = ReadBuiltinType(&result_uint64);
-  *result = static_cast<size_t>(result_uint64);
-  // Fail if the cast above truncates the value.
-  return success && (*result == result_uint64);
+  if (!ReadBuiltinType(&result_uint64))
+    return false;
+  // CHECK if the cast truncates the value so that we know to change this IPC
+  // parameter to use uint64_t.

[Tom Sepez, 2016/01/27 18:23:20]

Same here.

+  *result = base::checked_cast<size_t>(result_uint64);
+  return true;
 }
 
 bool PickleIterator::ReadFloat(float* result) {
   // crbug.com/315213
   // The source data may not be properly aligned, and unaligned float reads
   // cause SIGBUS on some ARM platforms, so force using memcpy to copy the data
   // into the result.
   const char* read_from = GetReadPointerAndAdvance<float>();
   if (!read_from)
     return false;
@@ skipping 290 matching lines @@
 
 inline void Pickle::WriteBytesCommon(const void* data, size_t length) {
   DCHECK_NE(kCapacityReadOnly, capacity_after_header_)
       << "oops: pickle is readonly";
   MSAN_CHECK_MEM_IS_INITIALIZED(data, length);
   void* write = ClaimUninitializedBytesInternal(length);
   memcpy(write, data, length);
 }
 
 }  // namespace base