Mojo C++ bindings: fix inlined union validation.

Previously the code couldn't correctly validate inlined nullable unions.

BUG=None.

Review-Url: https://codereview.chromium.org/2286513002
Cr-Commit-Position: refs/heads/master@{#414662}

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

@@ -46,20 +46,17 @@ bool ValidateStructHeaderAndClaimMemory(const void* data,
   return true;
 }
 
-bool ValidateUnionHeaderAndClaimMemory(const void* data,
-                                       bool inlined,
-                                       ValidationContext* validation_context) {
+bool ValidateNonInlinedUnionHeaderAndClaimMemory(
+    const void* data,
+    ValidationContext* validation_context) {
   if (!IsAligned(data)) {
     ReportValidationError(validation_context,
                           VALIDATION_ERROR_MISALIGNED_OBJECT);
     return false;
   }
 
-  // If the union is inlined in another structure its memory was already
-  // claimed.
-  // This ONLY applies to the union itself, NOT anything which the union points
-  // to.
-  if (!inlined && !validation_context->ClaimMemory(data, kUnionDataSize)) {
+  if (!validation_context->ClaimMemory(data, kUnionDataSize) ||
+      *static_cast<const uint32_t*>(data) != kUnionDataSize) {
     ReportValidationError(validation_context,
                           VALIDATION_ERROR_ILLEGAL_MEMORY_RANGE);
     return false;

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

@@ -50,12 +50,12 @@ bool ValidateStructHeaderAndClaimMemory(const void* data,
                                         ValidationContext* validation_context);
 
 // Validates that |data| contains a valid union header, in terms of alignment
-// and size. If not inlined, it checks that the memory range
-// [data, data + num_bytes) is not marked as occupied by other objects in
-// |validation_context|. On success, the memory range is marked as occupied.
-bool ValidateUnionHeaderAndClaimMemory(const void* data,
-                                       bool inlined,
-                                       ValidationContext* validation_context);
+// and size. It checks that the memory range [data, data + kUnionDataSize) is
+// not marked as occupied by other objects in |validation_context|. On success,
+// the memory range is marked as occupied.
+bool ValidateNonInlinedUnionHeaderAndClaimMemory(
+    const void* data,
+    ValidationContext* validation_context);
 
 // Validates that the message is a request which doesn't expect a response.
 bool ValidateMessageIsRequestWithoutResponse(

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

+[dist4]message_header  // num_bytes
+[u4]0                  // version
+[u4]0                  // interface ID
+[u4]18                 // name
+[u4]0                  // flags
+[u4]0                  // padding
+[anchr]message_header
+
+[dist4]method18_params  // num_bytes
+[u4]0                   // version
+[u4]0                   // param0: Size 0 indicating the inlined union is null.
+[u4]0                   // param0: Tag field ignored.
+[u8]0                   // param0: Payload field ignored.
+[anchr]method18_params

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

+PASS

mojo/public/interfaces/bindings/tests/validation_test_interfaces.mojom

@@ -56,6 +56,11 @@ enum EnumB {
   ENUM_B_2
 };
 
+union UnionA {
+  StructA struct_a;
+  bool b;
+};
+
 // This interface is used for testing bounds-checking in the mojom
 // binding code. If you add a method please update the files
 // ./data/validation/boundscheck_*. If you add a response please update
@@ -84,6 +89,7 @@ interface ConformanceTestInterface {
   Method15(array<EnumA>? param0, array<EnumB>? param1);
   Method16(map<EnumA, EnumA>? param0);
   Method17(array<InterfaceA> param0);
+  Method18(UnionA? param0);
 };
 
 struct BasicStruct {

mojo/public/tools/bindings/generators/cpp_templates/union_definition.tmpl

@@ -7,15 +7,27 @@ bool {{class_name}}::Validate(
     const void* data,
     mojo::internal::ValidationContext* validation_context,
     bool inlined) {
-  if (!data)
+  if (!data) {
+    DCHECK(!inlined);
     return true;
+  }
+
+  // If it is inlined, the alignment is already enforced by its enclosing
+  // object. We don't have to validate that.
+  DCHECK(!inlined || mojo::internal::IsAligned(data));
 
-  if (!ValidateUnionHeaderAndClaimMemory(data, inlined, validation_context))
+  if (!inlined &&
+      !mojo::internal::ValidateNonInlinedUnionHeaderAndClaimMemory(
+          data, validation_context)) {
     return false;
+  }
 
   const {{class_name}}* object = static_cast<const {{class_name}}*>(data);
   ALLOW_UNUSED_LOCAL(object);
 
+  if (inlined && object->is_null())
+    return true;
+
   switch (object->tag) {
 {%  for field in union.fields %}
     case {{enum_name}}::{{field.name|upper}}: {