Mojo cpp bindings: report the reason of validation failure.

Validation errors are written to stderr and also used in validation tests.
This CL also changes MessageHeaderValidator to use the same struct header
validation code as other structs.

BUG=None
TEST=validation_unittest.cc
R=darin@chromium.org

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

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@274855 0039d316-1c4b-4281-b951-d872f2087c98

mojo/mojo_public.gypi

@@ -273,6 +273,8 @@
         'public/cpp/bindings/lib/string_serialization.h',
         'public/cpp/bindings/lib/string_serialization.cc',
         'public/cpp/bindings/lib/sync_dispatcher.cc',
+        'public/cpp/bindings/lib/validation_errors.cc',
+        'public/cpp/bindings/lib/validation_errors.h',
       ],
     },
     {

mojo/public/cpp/bindings/BUILD.gn

@@ -47,5 +47,7 @@ source_set("bindings") {
     "lib/string_serialization.h",
     "lib/sync_dispatcher.cc",
     "lib/template_util.h",
+    "lib/validation_errors.cc",
+    "lib/validation_errors.h",
   ]
 }

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

@@ -58,8 +58,10 @@ bool ArraySerializationHelper<Handle, true>::ValidateElements(
     const ElementType* elements,
     BoundsChecker* bounds_checker) {
   for (uint32_t i = 0; i < header->num_elements; ++i) {
-    if (!bounds_checker->ClaimHandle(elements[i]))
+    if (!bounds_checker->ClaimHandle(elements[i])) {
+      ReportValidationError(VALIDATION_ERROR_ILLEGAL_HANDLE);
       return false;
+    }
   }
   return true;
 }

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

@@ -12,6 +12,7 @@
 #include "mojo/public/cpp/bindings/lib/bindings_serialization.h"
 #include "mojo/public/cpp/bindings/lib/bounds_checker.h"
 #include "mojo/public/cpp/bindings/lib/buffer.h"
+#include "mojo/public/cpp/bindings/lib/validation_errors.h"
 
 namespace mojo {
 template <typename T> class Array;
@@ -183,10 +184,12 @@ struct ArraySerializationHelper<P*, false> {
                                const ElementType* elements,
                                BoundsChecker* bounds_checker) {
     for (uint32_t i = 0; i < header->num_elements; ++i) {
-      if (!ValidateEncodedPointer(&elements[i].offset) ||
-          !P::Validate(DecodePointerRaw(&elements[i].offset), bounds_checker)) {
+      if (!ValidateEncodedPointer(&elements[i].offset)) {
+        ReportValidationError(VALIDATION_ERROR_ILLEGAL_POINTER);
         return false;
       }
+      if (!P::Validate(DecodePointerRaw(&elements[i].offset), bounds_checker))
+        return false;
     }
     return true;
   }
@@ -211,17 +214,24 @@ class Array_Data {
   static bool Validate(const void* data, BoundsChecker* bounds_checker) {
     if (!data)
       return true;
-    if (!IsAligned(data))
+    if (!IsAligned(data)) {
+      ReportValidationError(VALIDATION_ERROR_MISALIGNED_OBJECT);
       return false;
-    if (!bounds_checker->IsValidRange(data, sizeof(ArrayHeader)))
+    }
+    if (!bounds_checker->IsValidRange(data, sizeof(ArrayHeader))) {
+      ReportValidationError(VALIDATION_ERROR_ILLEGAL_MEMORY_RANGE);
       return false;
+    }
     const ArrayHeader* header = static_cast<const ArrayHeader*>(data);
     if (header->num_bytes < (sizeof(Array_Data<T>) +
                              Traits::GetStorageSize(header->num_elements))) {
+      ReportValidationError(VALIDATION_ERROR_UNEXPECTED_ARRAY_HEADER);
       return false;
     }
-    if (!bounds_checker->ClaimMemory(data, header->num_bytes))
+    if (!bounds_checker->ClaimMemory(data, header->num_bytes)) {
+      ReportValidationError(VALIDATION_ERROR_ILLEGAL_MEMORY_RANGE);
       return false;
+    }
 
     const Array_Data<T>* object = static_cast<const Array_Data<T>*>(data);
     return Helper::ValidateElements(&object->header_, object->storage(),

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

@@ -8,6 +8,7 @@
 
 #include "mojo/public/cpp/bindings/lib/bindings_internal.h"
 #include "mojo/public/cpp/bindings/lib/bounds_checker.h"
+#include "mojo/public/cpp/bindings/lib/validation_errors.h"
 
 namespace mojo {
 namespace internal {
@@ -83,21 +84,30 @@ bool ValidateStructHeader(const void* data,
                           uint32_t min_num_bytes,
                           uint32_t min_num_fields,
                           BoundsChecker* bounds_checker) {
-  if (!IsAligned(data))
+  if (!IsAligned(data)) {
+    ReportValidationError(VALIDATION_ERROR_MISALIGNED_OBJECT);
     return false;
-  if (!bounds_checker->IsValidRange(data, sizeof(StructHeader)))
+  }
+  if (!bounds_checker->IsValidRange(data, sizeof(StructHeader))) {
+    ReportValidationError(VALIDATION_ERROR_ILLEGAL_MEMORY_RANGE);
     return false;
+  }
 
   const StructHeader* header = static_cast<const StructHeader*>(data);
 
   // TODO(yzshen): Currently our binding code cannot handle structs of smaller
   // size or with fewer fields than the version that it sees. That needs to be
   // changed in order to provide backward compatibility.
-  if (header->num_bytes < min_num_bytes || header->num_fields < min_num_fields)
+  if (header->num_bytes < min_num_bytes ||
+      header->num_fields < min_num_fields) {
+    ReportValidationError(VALIDATION_ERROR_UNEXPECTED_STRUCT_HEADER);
     return false;
+  }
 
-  if (!bounds_checker->ClaimMemory(data, header->num_bytes))
+  if (!bounds_checker->ClaimMemory(data, header->num_bytes)) {
+    ReportValidationError(VALIDATION_ERROR_ILLEGAL_MEMORY_RANGE);
     return false;
+  }
 
   return true;
 }

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

@@ -5,49 +5,52 @@
 #include "mojo/public/cpp/bindings/lib/message_header_validator.h"
 
 #include "mojo/public/cpp/bindings/lib/bindings_serialization.h"
+#include "mojo/public/cpp/bindings/lib/bounds_checker.h"
+#include "mojo/public/cpp/bindings/lib/validation_errors.h"
 
 namespace mojo {
 namespace internal {
 namespace {
 
-bool IsValidMessageHeader(const internal::MessageHeader* header) {
+bool IsValidMessageHeader(const MessageHeader* header) {
   // NOTE: Our goal is to preserve support for future extension of the message
   // header. If we encounter fields we do not understand, we must ignore them.
 
-  // Validate num_bytes:
-
-  if (header->num_bytes < sizeof(internal::MessageHeader))
-    return false;
-  if (internal::Align(header->num_bytes) != header->num_bytes)
-    return false;
-
-  // Validate num_fields:
-
-  if (header->num_fields < 2)
-    return false;
+  // Extra validation of the struct header:
   if (header->num_fields == 2) {
-    if (header->num_bytes != sizeof(internal::MessageHeader))
+    if (header->num_bytes != sizeof(MessageHeader)) {
+      ReportValidationError(VALIDATION_ERROR_UNEXPECTED_STRUCT_HEADER);
       return false;
+    }
   } else if (header->num_fields == 3) {
-    if (header->num_bytes != sizeof(internal::MessageHeaderWithRequestID))
+    if (header->num_bytes != sizeof(MessageHeaderWithRequestID)) {
+      ReportValidationError(VALIDATION_ERROR_UNEXPECTED_STRUCT_HEADER);
       return false;
+    }
   } else if (header->num_fields > 3) {
-    if (header->num_bytes < sizeof(internal::MessageHeaderWithRequestID))
+    if (header->num_bytes < sizeof(MessageHeaderWithRequestID)) {
+      ReportValidationError(VALIDATION_ERROR_UNEXPECTED_STRUCT_HEADER);
       return false;
+    }
   }
 
   // Validate flags (allow unknown bits):
 
   // These flags require a RequestID.
   if (header->num_fields < 3 &&
-        ((header->flags & internal::kMessageExpectsResponse) ||
-         (header->flags & internal::kMessageIsResponse)))
+        ((header->flags & kMessageExpectsResponse) ||
+         (header->flags & kMessageIsResponse))) {
+    ReportValidationError(VALIDATION_ERROR_MESSAGE_HEADER_MISSING_REQUEST_ID);
     return false;
+  }
 
   // These flags are mutually exclusive.
-  if ((header->flags & internal::kMessageExpectsResponse) &&
-      (header->flags & internal::kMessageIsResponse))
+  if ((header->flags & kMessageExpectsResponse) &&
+      (header->flags & kMessageIsResponse)) {
+    ReportValidationError(
+        VALIDATION_ERROR_MESSAGE_HEADER_INVALID_FLAG_COMBINAION);
     return false;
+  }
 
   return true;
 }
@@ -59,9 +62,12 @@ MessageHeaderValidator::MessageHeaderValidator(MessageReceiver* sink)
 }
 
 bool MessageHeaderValidator::Accept(Message* message) {
-  // Make sure the message header isn't truncated before we start to read it.
-  if (message->data_num_bytes() < sizeof(internal::MessageHeader) ||
-      message->data_num_bytes() < message->header()->num_bytes) {
+  // Pass 0 as number of handles because we don't expect any in the header, even
+  // if |message| contains handles.
+  BoundsChecker bounds_checker(message->data(), message->data_num_bytes(), 0);
+
+  if (!ValidateStructHeader(message->data(), sizeof(MessageHeader), 2,
+                            &bounds_checker)) {
     return false;
   }
 

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

+// Copyright 2014 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 "mojo/public/cpp/bindings/lib/validation_errors.h"
+
+#include <assert.h>
+#include <stdio.h>
+
+namespace mojo {
+namespace internal {
+namespace {
+
+ValidationErrorObserverForTesting* g_validation_error_observer = NULL;
+
+}  // namespace
+
+const char* ValidationErrorToString(ValidationError error) {
+  switch (error) {
+    case VALIDATION_ERROR_NONE:
+      return "VALIDATION_ERROR_NONE";
+    case VALIDATION_ERROR_MISALIGNED_OBJECT:
+      return "VALIDATION_ERROR_MISALIGNED_OBJECT";
+    case VALIDATION_ERROR_ILLEGAL_MEMORY_RANGE:
+      return "VALIDATION_ERROR_ILLEGAL_MEMORY_RANGE";
+    case VALIDATION_ERROR_UNEXPECTED_STRUCT_HEADER:
+      return "VALIDATION_ERROR_UNEXPECTED_STRUCT_HEADER";
+    case VALIDATION_ERROR_UNEXPECTED_ARRAY_HEADER:
+      return "VALIDATION_ERROR_UNEXPECTED_ARRAY_HEADER";
+    case VALIDATION_ERROR_ILLEGAL_HANDLE:
+      return "VALIDATION_ERROR_ILLEGAL_HANDLE";
+    case VALIDATION_ERROR_ILLEGAL_POINTER:
+      return "VALIDATION_ERROR_ILLEGAL_POINTER";
+    case VALIDATION_ERROR_MESSAGE_HEADER_INVALID_FLAG_COMBINAION:
+      return "VALIDATION_ERROR_MESSAGE_HEADER_INVALID_FLAG_COMBINAION";
+    case VALIDATION_ERROR_MESSAGE_HEADER_MISSING_REQUEST_ID:
+      return "VALIDATION_ERROR_MESSAGE_HEADER_MISSING_REQUEST_ID";
+  }
+
+  return "Unknown error";
+}
+
+void ReportValidationError(ValidationError error) {
+  // TODO(yzshen): Consider adding better logging support.
+  fprintf(stderr, "Invalid message: %s\n", ValidationErrorToString(error));
+  if (g_validation_error_observer)
+    g_validation_error_observer->set_last_error(error);
+}
+
+ValidationErrorObserverForTesting::ValidationErrorObserverForTesting()
+    : last_error_(VALIDATION_ERROR_NONE) {
+  assert(!g_validation_error_observer);
+  g_validation_error_observer = this;
+}
+
+ValidationErrorObserverForTesting::~ValidationErrorObserverForTesting() {
+  assert(g_validation_error_observer == this);
+  g_validation_error_observer = NULL;
+}
+
+}  // namespace internal
+}  // namespace mojo

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

+// Copyright 2014 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.
+
+#ifndef MOJO_PUBLIC_CPP_BINDINGS_LIB_VALIDATION_ERRORS_H_
+#define MOJO_PUBLIC_CPP_BINDINGS_LIB_VALIDATION_ERRORS_H_
+
+#include "mojo/public/cpp/system/macros.h"
+
+namespace mojo {
+namespace internal {
+
+enum ValidationError {
+  // There is no validation error.
+  VALIDATION_ERROR_NONE,
+  // An object (struct or array) is not 8-byte aligned.
+  VALIDATION_ERROR_MISALIGNED_OBJECT,
+  // An object is not contained inside the message data, or it overlaps other
+  // objects.
+  VALIDATION_ERROR_ILLEGAL_MEMORY_RANGE,
+  // A struct header doesn't make sense, for example:
+  // - |num_bytes| is smaller than the size of the oldest version that we
+  // support.
+  // - |num_fields| is smaller than the field number of the oldest version that
+  // we support.
+  // - |num_bytes| and |num_fields| don't match.
+  VALIDATION_ERROR_UNEXPECTED_STRUCT_HEADER,
+  // An array header doesn't make sense, for example:
+  // - |num_bytes| is smaller than the size of the header plus the size required
+  // to store |num_elements| elements.
+  VALIDATION_ERROR_UNEXPECTED_ARRAY_HEADER,
+  // An encoded handle is illegal.
+  VALIDATION_ERROR_ILLEGAL_HANDLE,
+  // An encoded pointer is illegal.
+  VALIDATION_ERROR_ILLEGAL_POINTER,
+  // |flags| in the message header is an invalid flag combination.
+  VALIDATION_ERROR_MESSAGE_HEADER_INVALID_FLAG_COMBINAION,
+  // |flags| in the message header indicates that a request ID is required but
+  // there isn't one.
+  VALIDATION_ERROR_MESSAGE_HEADER_MISSING_REQUEST_ID,
+};
+
+const char* ValidationErrorToString(ValidationError error);
+
+void ReportValidationError(ValidationError error);
+
+// Only used by validation tests and when there is only one thread doing message
+// validation.
+class ValidationErrorObserverForTesting {
+ public:
+  ValidationErrorObserverForTesting();
+  ~ValidationErrorObserverForTesting();
+
+  ValidationError last_error() const { return last_error_; }
+  void set_last_error(ValidationError error) { last_error_ = error; }
+
+ private:
+  ValidationError last_error_;
+
+  MOJO_DISALLOW_COPY_AND_ASSIGN(ValidationErrorObserverForTesting);
+};
+
+}  // namespace internal
+}  // namespace mojo
+
+#endif  // MOJO_PUBLIC_CPP_BINDINGS_LIB_VALIDATION_ERRORS_H_

mojo/public/cpp/bindings/tests/validation_unittest.cc

@@ -10,6 +10,7 @@
 #include <vector>
 
 #include "mojo/public/cpp/bindings/lib/message_header_validator.h"
+#include "mojo/public/cpp/bindings/lib/validation_errors.h"
 #include "mojo/public/cpp/test_support/test_support.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -17,6 +18,12 @@ namespace mojo {
 namespace test {
 namespace {
 
+std::string ValidationErrorToResultString(internal::ValidationError error) {
+  std::string result = internal::ValidationErrorToString(error);
+  result.push_back('\n');
+  return result;
+}
+
 std::vector<std::string> GetMatchingTests(const std::vector<std::string>& names,
                                           const std::string& prefix) {
   const std::string suffix = ".data";
@@ -99,11 +106,14 @@ class DummyMessageReceiver : public MessageReceiver {
 };
 
 std::string DumpMessageHeader(Message* message) {
+  internal::ValidationErrorObserverForTesting observer;
   DummyMessageReceiver not_reached_receiver;
   internal::MessageHeaderValidator validator(&not_reached_receiver);
   bool rv = validator.Accept(message);
-  if (!rv)
-    return "ERROR\n";
+  if (!rv) {
+    EXPECT_NE(internal::VALIDATION_ERROR_NONE, observer.last_error());
+    return ValidationErrorToResultString(observer.last_error());
+  }
 
   std::ostringstream os;
   os << "num_bytes: " << message->header()->num_bytes << "\n"

mojo/public/interfaces/bindings/tests/data/validate_header_bad_empty.expected

-ERROR
+VALIDATION_ERROR_ILLEGAL_MEMORY_RANGE

mojo/public/interfaces/bindings/tests/data/validate_header_bad_too_small.expected

-ERROR
+VALIDATION_ERROR_ILLEGAL_MEMORY_RANGE

mojo/public/interfaces/bindings/tests/data/validate_header_bad_zeros.expected

-ERROR
+VALIDATION_ERROR_UNEXPECTED_STRUCT_HEADER

mojo/public/tools/bindings/generators/cpp_templates/module.cc.tmpl

@@ -14,6 +14,7 @@
 #include "mojo/public/cpp/bindings/lib/bounds_checker.h"
 #include "mojo/public/cpp/bindings/lib/message_builder.h"
 #include "mojo/public/cpp/bindings/lib/string_serialization.h"
+#include "mojo/public/cpp/bindings/lib/validation_errors.h"
 
 {%- for namespace in namespaces_as_array %}
 namespace {{namespace}} {

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

@@ -15,15 +15,20 @@
 {%-     set name = packed_field.field.name %}
 {%-     if packed_field.field.kind|is_object_kind %}
 {%-       set wrapper_type = packed_field.field.kind|cpp_wrapper_type %}
-  if (!mojo::internal::ValidateEncodedPointer(&object->{{name}}.offset) ||
-      !{{wrapper_type}}::Data_::Validate(
+  if (!mojo::internal::ValidateEncodedPointer(&object->{{name}}.offset)) {
+    ReportValidationError(mojo::internal::VALIDATION_ERROR_ILLEGAL_POINTER);
+    return false;
+  }
+  if (!{{wrapper_type}}::Data_::Validate(
           mojo::internal::DecodePointerRaw(&object->{{name}}.offset),
           bounds_checker)) {
     return false;
   }
 {%-     elif packed_field.field.kind|is_handle_kind %}
-  if (!bounds_checker->ClaimHandle(object->{{name}}))
+  if (!bounds_checker->ClaimHandle(object->{{name}})) {
+    ReportValidationError(mojo::internal::VALIDATION_ERROR_ILLEGAL_HANDLE);
     return false;
+  }
 {%-     endif %}
 {%-   endfor %}