Mojo cpp bindings: More clear checks for array num_bytes and num_elements.

This CL (1) handles the case that Array<>'s element number or total size exceeds what we can represent in Mojo messages; (2) makes the validation code of the num_bytes field doesn't rely on sizeof(size_t) > sizeof(uint32_t).

The newly-added test passes with/without this CL. But I think it is good to have such a test.

BUG=None
TEST=None

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

Cr-Commit-Position: refs/heads/master@{#290718}
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@290718 0039d316-1c4b-4281-b951-d872f2087c98

mojo/public/cpp/bindings/lib/array_internal.h

@@ -23,14 +23,22 @@ class String;
 
 namespace internal {
 
+// std::numeric_limits<uint32_t>::max() is not a compile-time constant (until
+// C++11).
+const uint32_t kMaxUint32 = 0xFFFFFFFF;
+
 template <typename T>
 struct ArrayDataTraits {
   typedef T StorageType;
   typedef T& Ref;
   typedef T const& ConstRef;
 
-  static size_t GetStorageSize(size_t num_elements) {
-    return sizeof(StorageType) * num_elements;
+  static const uint32_t kMaxNumElements =
+      (kMaxUint32 - sizeof(ArrayHeader)) / sizeof(StorageType);
+
+  static uint32_t GetStorageSize(uint32_t num_elements) {
+    MOJO_DCHECK(num_elements <= kMaxNumElements);
+    return sizeof(ArrayHeader) + sizeof(StorageType) * num_elements;
   }
   static Ref ToRef(StorageType* storage, size_t offset) {
     return storage[offset];
@@ -46,8 +54,12 @@ struct ArrayDataTraits<P*> {
   typedef P*& Ref;
   typedef P* const& ConstRef;
 
-  static size_t GetStorageSize(size_t num_elements) {
-    return sizeof(StorageType) * num_elements;
+  static const uint32_t kMaxNumElements =
+      (kMaxUint32 - sizeof(ArrayHeader)) / sizeof(StorageType);
+
+  static uint32_t GetStorageSize(uint32_t num_elements) {
+    MOJO_DCHECK(num_elements <= kMaxNumElements);
+    return sizeof(ArrayHeader) + sizeof(StorageType) * num_elements;
   }
   static Ref ToRef(StorageType* storage, size_t offset) {
     return storage[offset].ptr;
@@ -63,8 +75,12 @@ struct ArrayDataTraits<Array_Data<T>*> {
   typedef Array_Data<T>*& Ref;
   typedef Array_Data<T>* const& ConstRef;
 
-  static size_t GetStorageSize(size_t num_elements) {
-    return sizeof(StorageType) * num_elements;
+  static const uint32_t kMaxNumElements =
+      (kMaxUint32 - sizeof(ArrayHeader)) / sizeof(StorageType);
+
+  static uint32_t GetStorageSize(uint32_t num_elements) {
+    MOJO_DCHECK(num_elements <= kMaxNumElements);
+    return sizeof(ArrayHeader) + sizeof(StorageType) * num_elements;
   }
   static Ref ToRef(StorageType* storage, size_t offset) {
     return storage[offset].ptr;
@@ -97,12 +113,15 @@ struct ArrayDataTraits<bool> {
     uint8_t mask_;
   };
 
+  // Because each element consumes only 1/8 byte.
+  static const uint32_t kMaxNumElements = kMaxUint32;
+
   typedef uint8_t StorageType;
   typedef BitRef Ref;
   typedef bool ConstRef;
 
-  static size_t GetStorageSize(size_t num_elements) {
-    return ((num_elements + 7) / 8);
+  static uint32_t GetStorageSize(uint32_t num_elements) {
+    return sizeof(ArrayHeader) + ((num_elements + 7) / 8);
   }
   static BitRef ToRef(StorageType* storage, size_t offset) {
     return BitRef(&storage[offset / 8], 1 << (offset % 8));
@@ -298,11 +317,16 @@ class Array_Data {
   typedef typename Traits::ConstRef ConstRef;
   typedef ArraySerializationHelper<T, IsHandle<T>::value> Helper;
 
+  // Returns NULL if |num_elements| or the corresponding storage size cannot be
+  // stored in uint32_t.
   static Array_Data<T>* New(size_t num_elements, Buffer* buf) {
-    size_t num_bytes = sizeof(Array_Data<T>) +
-                       Traits::GetStorageSize(num_elements);
-    return new (buf->Allocate(num_bytes)) Array_Data<T>(num_bytes,
-                                                        num_elements);
+    if (num_elements > Traits::kMaxNumElements)
+      return NULL;
+
+    uint32_t num_bytes =
+        Traits::GetStorageSize(static_cast<uint32_t>(num_elements));
+    return new (buf->Allocate(num_bytes)) Array_Data<T>(
+        num_bytes, static_cast<uint32_t>(num_elements));
   }
 
   template <typename Params>
@@ -318,8 +342,8 @@ class Array_Data {
       return false;
     }
     const ArrayHeader* header = static_cast<const ArrayHeader*>(data);
-    if (header->num_bytes < (sizeof(Array_Data<T>) +
-                             Traits::GetStorageSize(header->num_elements))) {
+    if (header->num_elements > Traits::kMaxNumElements ||
+        header->num_bytes < Traits::GetStorageSize(header->num_elements)) {
       ReportValidationError(VALIDATION_ERROR_UNEXPECTED_ARRAY_HEADER);
       return false;
     }
@@ -370,9 +394,9 @@ class Array_Data {
   }
 
  private:
-  Array_Data(size_t num_bytes, size_t num_elements) {
-    header_.num_bytes = static_cast<uint32_t>(num_bytes);
-    header_.num_elements = static_cast<uint32_t>(num_elements);
+  Array_Data(uint32_t num_bytes, uint32_t num_elements) {
+    header_.num_bytes = num_bytes;
+    header_.num_elements = num_elements;
   }
   ~Array_Data() {}
 

mojo/public/cpp/bindings/lib/array_serialization.h

@@ -227,10 +227,12 @@ inline void SerializeArray_(Array<E> input, internal::Buffer* buf,
 
     internal::Array_Data<F>* result =
         internal::Array_Data<F>::New(input.size(), buf);
-    internal::ArraySerializer<E, F>::template SerializeElements<
-        ValidateParams::element_is_nullable,
-        typename ValidateParams::ElementValidateParams>(
-            internal::Forward(input), buf, result);
+    if (result) {
+      internal::ArraySerializer<E, F>::template SerializeElements<
+          ValidateParams::element_is_nullable,
+          typename ValidateParams::ElementValidateParams>(
+              internal::Forward(input), buf, result);
+    }
     *output = result;
   } else {
     *output = NULL;

mojo/public/cpp/bindings/lib/string_serialization.cc

@@ -19,7 +19,8 @@ void Serialize_(const String& input, internal::Buffer* buf,
   if (input) {
     internal::String_Data* result =
         internal::String_Data::New(input.size(), buf);
-    memcpy(result->storage(), input.data(), input.size());
+    if (result)
+      memcpy(result->storage(), input.data(), input.size());
     *output = result;
   } else {
     *output = NULL;

mojo/public/interfaces/bindings/tests/data/validation/conformance_mthd8_array_num_bytes_overflow.data

+[dist4]message_header  // num_bytes
+[u4]2                  // num_fields
+[u4]8                  // name
+[u4]0                  // flags
+[anchr]message_header
+
+[dist4]method8_params  // num_bytes
+[u4]1                  // num_fields
+[dist8]param0_ptr      // param0
+[anchr]method8_params
+
+[anchr]param0_ptr
+[dist4]array_param  // num_bytes
+[u4]0x20000001      // num_elements: The corresponding array size should be
+                    // 0x20000001 * 8 + 8 = 0x100000010 which is
+                    // 2^32 + 16 (base-10), while |num_bytes| is a 32-bit
+                    // unsigned integer and its value is 16.
+[u8]0
+[anchr]array_param

mojo/public/interfaces/bindings/tests/data/validation/conformance_mthd8_array_num_bytes_overflow.expected

+VALIDATION_ERROR_UNEXPECTED_ARRAY_HEADER