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

long is 4 bytes in 32 bit builds and 8 bytes in 64 bit builds so we don't want to send it between 32 and 64 bit processes. We can't remove the long IPC traits completely since types like uint64_t also use the long traits on Windows and 64 bit POSIX. So keep the traits for these platforms but remove it for others. This ensures that longs aren't sent over IPC between 32 and 64 bit  configs since the 32 bit build would have a compile error.

Also remove the size_t methods from Pickle. We can't add compile time checks for that since it's a typedef. A clang plugin will catch those cases.

BUG=581409

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

Cr-Commit-Position: refs/heads/master@{#374707}

base/pickle.cc

@@ -11,6 +11,7 @@
 
 #include "base/bits.h"
 #include "base/macros.h"
+#include "base/numerics/safe_conversions.h"
 #include "build/build_config.h"
 
 namespace base {
@@ -89,7 +90,15 @@ bool PickleIterator::ReadInt(int* 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);
+  return true;
 }
 
 bool PickleIterator::ReadUInt16(uint16_t* result) {
@@ -108,16 +117,6 @@ 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);
-}
-
 bool PickleIterator::ReadFloat(float* result) {
   // crbug.com/315213
   // The source data may not be properly aligned, and unaligned float reads

base/pickle.h

@@ -45,7 +45,6 @@ class BASE_EXPORT PickleIterator {
   bool ReadUInt32(uint32_t* result) WARN_UNUSED_RESULT;
   bool ReadInt64(int64_t* result) WARN_UNUSED_RESULT;
   bool ReadUInt64(uint64_t* result) WARN_UNUSED_RESULT;
-  bool ReadSizeT(size_t* result) WARN_UNUSED_RESULT;
   bool ReadFloat(float* result) WARN_UNUSED_RESULT;
   bool ReadDouble(double* result) WARN_UNUSED_RESULT;
   bool ReadString(std::string* result) WARN_UNUSED_RESULT;
@@ -122,12 +121,11 @@ class BASE_EXPORT PickleSizer {
 
   void AddBool() { return AddInt(); }
   void AddInt() { AddPOD<int>(); }
-  void AddLongUsingDangerousNonPortableLessPersistableForm() { AddPOD<long>(); }
+  void AddLong() { AddPOD<uint64_t>(); }
   void AddUInt16() { return AddPOD<uint16_t>(); }
   void AddUInt32() { return AddPOD<uint32_t>(); }
   void AddInt64() { return AddPOD<int64_t>(); }
   void AddUInt64() { return AddPOD<uint64_t>(); }
-  void AddSizeT() { return AddPOD<uint64_t>(); }
   void AddFloat() { return AddPOD<float>(); }
   void AddDouble() { return AddPOD<double>(); }
   void AddString(const StringPiece& value);
@@ -229,23 +227,15 @@ class BASE_EXPORT Pickle {
   bool WriteInt(int value) {
     return WritePOD(value);
   }
-  // WARNING: DO NOT USE THIS METHOD IF PICKLES ARE PERSISTED IN ANY WAY.
-  // It will write whatever a "long" is on this architecture. On 32-bit
-  // platforms, it is 32 bits. On 64-bit platforms, it is 64 bits. If persisted
-  // pickles are still around after upgrading to 64-bit, or if they are copied
-  // between dissimilar systems, YOUR PICKLES WILL HAVE GONE BAD.
-  bool WriteLongUsingDangerousNonPortableLessPersistableForm(long value) {
-    return WritePOD(value);
+  bool WriteLong(long value) {
+    // Always write long as a 64-bit value to ensure compatibility between
+    // 32-bit and 64-bit processes.
+    return WritePOD(static_cast<int64_t>(value));
   }
   bool WriteUInt16(uint16_t value) { return WritePOD(value); }
   bool WriteUInt32(uint32_t value) { return WritePOD(value); }
   bool WriteInt64(int64_t value) { return WritePOD(value); }
   bool WriteUInt64(uint64_t value) { return WritePOD(value); }
-  bool WriteSizeT(size_t value) {
-    // Always write size_t as a 64-bit value to ensure compatibility between
-    // 32-bit and 64-bit processes.
-    return WritePOD(static_cast<uint64_t>(value));
-  }
   bool WriteFloat(float value) {
     return WritePOD(value);
   }

base/pickle_unittest.cc

@@ -27,7 +27,6 @@ const uint16_t testuint16 = 32123;
 const uint32_t testuint32 = 1593847192;
 const int64_t testint64 = -0x7E8CA9253104BDFCLL;
 const uint64_t testuint64 = 0xCE8CA9253104BDF7ULL;
-const size_t testsizet = 0xFEDC7654;
 const float testfloat = 3.1415926935f;
 const double testdouble = 2.71828182845904523;
 const std::string teststring("Hello world");  // note non-aligned string length
@@ -73,10 +72,6 @@ void VerifyResult(const Pickle& pickle) {
   EXPECT_TRUE(iter.ReadUInt64(&outuint64));
   EXPECT_EQ(testuint64, outuint64);
 
-  size_t outsizet;
-  EXPECT_TRUE(iter.ReadSizeT(&outsizet));
-  EXPECT_EQ(testsizet, outsizet);
-
   float outfloat;
   EXPECT_TRUE(iter.ReadFloat(&outfloat));
   EXPECT_EQ(testfloat, outfloat);
@@ -119,13 +114,11 @@ TEST(PickleTest, EncodeDecode) {
   EXPECT_TRUE(pickle.WriteBool(testbool1));
   EXPECT_TRUE(pickle.WriteBool(testbool2));
   EXPECT_TRUE(pickle.WriteInt(testint));
-  EXPECT_TRUE(
-      pickle.WriteLongUsingDangerousNonPortableLessPersistableForm(testlong));
+  EXPECT_TRUE(pickle.WriteLong(testlong));
   EXPECT_TRUE(pickle.WriteUInt16(testuint16));
   EXPECT_TRUE(pickle.WriteUInt32(testuint32));
   EXPECT_TRUE(pickle.WriteInt64(testint64));
   EXPECT_TRUE(pickle.WriteUInt64(testuint64));
-  EXPECT_TRUE(pickle.WriteSizeT(testsizet));
   EXPECT_TRUE(pickle.WriteFloat(testfloat));
   EXPECT_TRUE(pickle.WriteDouble(testdouble));
   EXPECT_TRUE(pickle.WriteString(teststring));
@@ -145,25 +138,26 @@ TEST(PickleTest, EncodeDecode) {
   VerifyResult(pickle3);
 }
 
-// Tests that reading/writing a size_t works correctly when the source process
+// Tests that reading/writing a long works correctly when the source process
 // is 64-bit.  We rely on having both 32- and 64-bit trybots to validate both
 // arms of the conditional in this test.
-TEST(PickleTest, SizeTFrom64Bit) {
+TEST(PickleTest, LongFrom64Bit) {
   Pickle pickle;
-  // Under the hood size_t is always written as a 64-bit value, so simulate a
-  // 64-bit size_t even on 32-bit architectures by explicitly writing a
-  // uint64_t.
-  EXPECT_TRUE(pickle.WriteUInt64(testuint64));
+  // Under the hood long is always written as a 64-bit value, so simulate a
+  // 64-bit long even on 32-bit architectures by explicitly writing an int64_t.
+  EXPECT_TRUE(pickle.WriteInt64(testint64));
 
   PickleIterator iter(pickle);
-  size_t outsizet;
-  if (sizeof(size_t) < sizeof(uint64_t)) {
-    // ReadSizeT() should return false when the original written value can't be
-    // represented as a size_t.
-    EXPECT_FALSE(iter.ReadSizeT(&outsizet));
+  long outlong;
+  if (sizeof(long) < sizeof(int64_t)) {
+    // ReadLong() should return false when the original written value can't be
+    // represented as a long.
+#if GTEST_HAS_DEATH_TEST
+    EXPECT_DEATH(ignore_result(iter.ReadLong(&outlong)), "");
+#endif
   } else {
-    EXPECT_TRUE(iter.ReadSizeT(&outsizet));
-    EXPECT_EQ(testuint64, outsizet);
+    EXPECT_TRUE(iter.ReadLong(&outlong));
+    EXPECT_EQ(testint64, outlong);
   }
 }
 
@@ -556,14 +550,14 @@ TEST(PickleTest, ClaimBytes) {
   std::string data("Hello, world!");
 
   TestingPickle pickle;
-  pickle.WriteSizeT(data.size());
+  pickle.WriteUInt32(data.size());
   void* bytes = pickle.ClaimBytes(data.size());
   pickle.WriteInt(42);
   memcpy(bytes, data.data(), data.size());
 
   PickleIterator iter(pickle);
-  size_t out_data_length;
-  EXPECT_TRUE(iter.ReadSizeT(&out_data_length));
+  uint32_t out_data_length;
+  EXPECT_TRUE(iter.ReadUInt32(&out_data_length));
   EXPECT_EQ(data.size(), out_data_length);
 
   const char* out_data = nullptr;
@@ -594,8 +588,8 @@ TEST(PickleTest, PickleSizer) {
   {
     TestingPickle pickle;
     base::PickleSizer sizer;
-    pickle.WriteLongUsingDangerousNonPortableLessPersistableForm(42);
-    sizer.AddLongUsingDangerousNonPortableLessPersistableForm();
+    pickle.WriteLong(42);
+    sizer.AddLong();
     EXPECT_EQ(sizer.payload_size(), pickle.payload_size());
   }
   {
@@ -626,13 +620,6 @@ TEST(PickleTest, PickleSizer) {
     sizer.AddUInt64();
     EXPECT_EQ(sizer.payload_size(), pickle.payload_size());
   }
-  {
-    TestingPickle pickle;
-    base::PickleSizer sizer;
-    pickle.WriteSizeT(42);
-    sizer.AddSizeT();
-    EXPECT_EQ(sizer.payload_size(), pickle.payload_size());
-  }
   {
     TestingPickle pickle;
     base::PickleSizer sizer;

ipc/ipc_message_utils.cc

@@ -414,6 +414,7 @@ void ParamTraits<unsigned int>::Log(const param_type& p, std::string* l) {
   l->append(base::UintToString(p));
 }
 
+#if defined(OS_WIN) || defined(ARCH_CPU_ARM64) || defined(OS_LINUX)
 void ParamTraits<long>::Log(const param_type& p, std::string* l) {
   l->append(base::Int64ToString(static_cast<int64_t>(p)));
 }
@@ -421,6 +422,7 @@ void ParamTraits<long>::Log(const param_type& p, std::string* l) {
 void ParamTraits<unsigned long>::Log(const param_type& p, std::string* l) {
   l->append(base::Uint64ToString(static_cast<uint64_t>(p)));
 }
+#endif
 
 void ParamTraits<long long>::Log(const param_type& p, std::string* l) {
   l->append(base::Int64ToString(static_cast<int64_t>(p)));
@@ -690,7 +692,7 @@ void ParamTraits<base::SharedMemoryHandle>::Write(base::Pickle* m,
       size_t size = 0;
       bool result = p.GetSize(&size);
       DCHECK(result);
-      ParamTraits<size_t>::Write(m, size);
+      ParamTraits<uint32_t>::Write(m, static_cast<uint32_t>(size));
 
       // If the caller intended to pass ownership to the IPC stack, release a
       // reference.
@@ -738,11 +740,12 @@ bool ParamTraits<base::SharedMemoryHandle>::Read(const base::Pickle* m,
       if (!ParamTraits<MachPortMac>::Read(m, iter, &mach_port_mac))
         return false;
 
-      size_t size;
-      if (!ParamTraits<size_t>::Read(m, iter, &size))
+      uint32_t size;
+      if (!ParamTraits<uint32_t>::Read(m, iter, &size))
         return false;
 
-      *r = base::SharedMemoryHandle(mach_port_mac.get_mach_port(), size,
+      *r = base::SharedMemoryHandle(mach_port_mac.get_mach_port(),
+                                    static_cast<size_t>(size),
                                     base::GetCurrentProcId());
       return true;
     }

ipc/ipc_message_utils.h

@@ -182,14 +182,26 @@ struct ParamTraits<unsigned int> {
   IPC_EXPORT static void Log(const param_type& p, std::string* l);
 };
 
+// long isn't safe to send over IPC because it's 4 bytes on 32 bit builds but
+// 8 bytes on 64 bit builds. So if a 32 bit and 64 bit process have a channel
+// that would cause problem.
+// We need to keep this on for a few configs:
+//   1) Windows because DWORD is typedef'd to it, which is fine because we have
+//      very few IPCs that cross this boundary.
+//   2) We also need to keep it for Linux for two reasons: int64_t is typedef'd
+//      to long, and gfx::PluginWindow is long and is used in one GPU IPC.
+//   3) Android 64 bit also has int64_t typedef'd to long.
+// Since we want to support Android 32<>64 bit IPC, as long as we don't have
+// these traits for 32 bit ARM then that'll catch any errors.
+#if defined(OS_WIN) || defined(OS_LINUX) || defined(ARCH_CPU_ARM64)
 template <>
 struct ParamTraits<long> {
   typedef long param_type;
   static void GetSize(base::PickleSizer* sizer, const param_type& p) {
-    sizer->AddLongUsingDangerousNonPortableLessPersistableForm();
+    sizer->AddLong();
   }
   static void Write(base::Pickle* m, const param_type& p) {
-    m->WriteLongUsingDangerousNonPortableLessPersistableForm(p);
+    m->WriteLong(p);
   }
   static bool Read(const base::Pickle* m,
                    base::PickleIterator* iter,
@@ -203,10 +215,10 @@ template <>
 struct ParamTraits<unsigned long> {
   typedef unsigned long param_type;
   static void GetSize(base::PickleSizer* sizer, const param_type& p) {
-    sizer->AddLongUsingDangerousNonPortableLessPersistableForm();
+    sizer->AddLong();
   }
   static void Write(base::Pickle* m, const param_type& p) {
-    m->WriteLongUsingDangerousNonPortableLessPersistableForm(p);
+    m->WriteLong(p);
   }
   static bool Read(const base::Pickle* m,
                    base::PickleIterator* iter,
@@ -215,6 +227,7 @@ struct ParamTraits<unsigned long> {
   }
   IPC_EXPORT static void Log(const param_type& p, std::string* l);
 };
+#endif
 
 template <>
 struct ParamTraits<long long> {