Mojo C++ bindings: reject messages version 2 with null payload pointer.

BUG=712416

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

mojo/edk/js/tests/OWNERS

+per-file *.mojom=file://mojo/OWNERS
+
+# These mojom test files don't really require security review, but we need to
+# add these to please PRESUBMIT.
+per-file *.mojom=set noparent
+per-file *.mojom=file://ipc/SECURITY_OWNERS

mojo/edk/js/tests/js_to_cpp.mojom

@@ -28,6 +28,8 @@ struct EchoArgsList {
   EchoArgs? item;
 };
 
+interface ForTesting {};
+
 // Note: For messages which control test flow, pick numbers that are unlikely
 // to be hit as a result of our deliberate corruption of response messages.
 interface CppSide {
@@ -40,7 +42,11 @@ interface CppSide {
   // Responses from specific tests.
   PingResponse();
   EchoResponse(EchoArgsList list);
-  BitFlipResponse(EchoArgsList arg);
+
+  // Having an associated interface pointer in the message makes sure the
+  // message header version 2 is tested.
+  BitFlipResponse(EchoArgsList arg, associated ForTesting? not_used);
+
   BackPointerResponse(EchoArgsList arg);
 };
 

mojo/edk/js/tests/js_to_cpp_tests.cc

@@ -239,7 +239,9 @@ class CppSideConnection : public js_to_cpp::CppSide {
     mishandled_messages_ += 1;
   }
 
-  void BitFlipResponse(js_to_cpp::EchoArgsListPtr list) override {
+  void BitFlipResponse(
+      js_to_cpp::EchoArgsListPtr list,
+      js_to_cpp::ForTestingAssociatedPtrInfo not_used) override {
     mishandled_messages_ += 1;
   }
 
@@ -332,7 +334,9 @@ class BitFlipCppSideConnection : public CppSideConnection {
   // js_to_cpp::CppSide:
   void StartTest() override { js_side_->BitFlip(BuildSampleEchoArgs()); }
 
-  void BitFlipResponse(js_to_cpp::EchoArgsListPtr list) override {
+  void BitFlipResponse(
+      js_to_cpp::EchoArgsListPtr list,
+      js_to_cpp::ForTestingAssociatedPtrInfo not_used) override {
     CheckCorruptedEchoArgsList(list);
   }
 

mojo/edk/js/tests/js_to_cpp_tests.js

@@ -114,7 +114,7 @@ define('mojo/edk/js/tests/js_to_cpp_tests', [
       writeMessagePipe(messagePipe, sampleMessage);
       arg.message_handle = messagePipe.handle1;
 
-      this.cppSide_.bitFlipResponse(createEchoArgsList(arg));
+      this.cppSide_.bitFlipResponse(createEchoArgsList(arg), null);
 
       core.close(messagePipe.handle0);
       iteration += 1;

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

@@ -80,6 +80,7 @@ const uint8_t* Message::payload() const {
   if (version() < 2)
     return data() + header()->num_bytes;
 
+  DCHECK(!header_v2()->payload.is_null());
   return static_cast<const uint8_t*>(header_v2()->payload.Get());
 }
 
@@ -89,17 +90,14 @@ uint32_t Message::payload_num_bytes() const {
   if (version() < 2) {
     num_bytes = data_num_bytes() - header()->num_bytes;
   } else {
-    auto payload = reinterpret_cast<uintptr_t>(header_v2()->payload.Get());
-    if (!payload) {
-      num_bytes = 0;
-    } else {
-      auto payload_end =
-          reinterpret_cast<uintptr_t>(header_v2()->payload_interface_ids.Get());
-      if (!payload_end)
-        payload_end = reinterpret_cast<uintptr_t>(data() + data_num_bytes());
-      DCHECK_GE(payload_end, payload);
-      num_bytes = payload_end - payload;
-    }
+    auto payload_begin =
+        reinterpret_cast<uintptr_t>(header_v2()->payload.Get());
+    auto payload_end =
+        reinterpret_cast<uintptr_t>(header_v2()->payload_interface_ids.Get());
+    if (!payload_end)
+      payload_end = reinterpret_cast<uintptr_t>(data() + data_num_bytes());
+    DCHECK_GE(payload_end, payload_begin);
+    num_bytes = payload_end - payload_begin;
   }
   DCHECK_LE(num_bytes, std::numeric_limits<uint32_t>::max());
   return static_cast<uint32_t>(num_bytes);

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

@@ -73,9 +73,11 @@ bool IsValidMessageHeader(const internal::MessageHeader* header,
   //   payload size).
   // - Validation of the payload contents will be done separately based on the
   //   payload type.
-  if (!header_v2->payload.is_null() &&
-      (!internal::ValidatePointer(header_v2->payload, validation_context) ||
-       !validation_context->ClaimMemory(header_v2->payload.Get(), 1))) {
+  if (!internal::ValidatePointerNonNullable(header_v2->payload,
+                                            "null payload in message header",
+                                            validation_context) ||
+      !internal::ValidatePointer(header_v2->payload, validation_context) ||
+      !validation_context->ClaimMemory(header_v2->payload.Get(), 1)) {
     return false;
   }