Basic Mojo message header validation for JavaScript.

mojo/public/js/bindings/validator.js

Index: mojo/public/js/bindings/validator.js
diff --git a/mojo/public/js/bindings/validator.js b/mojo/public/js/bindings/validator.js
new file mode 100644
index 0000000000000000000000000000000000000000..ff5b8ee3e587fcab9a62dc45ccb24c91c99ad32c
--- /dev/null
+++ b/mojo/public/js/bindings/validator.js
@@ -0,0 +1,93 @@
+// 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.
+
+define("mojo/public/js/bindings/validator", [
+  "mojo/public/js/bindings/codec",
+  ], function(codec) {
+
+  var ValidationError = {

[Tom Sepez, 2014/07/29 16:32:54]

nit: In theory, this probably shouldn't be capitalized; its not the name of an
object you can construct with new.  But other chrome code violates this
convention.

[hansmuller, 2014/07/29 19:06:23]

OK, will use validationError.

+    NONE: 'VALIDATION_ERROR_NONE',

[Tom Sepez, 2014/07/29 16:32:54]

nit: if you use '' as the value for none (or some other fasly value), you get to
write things later on like:  if (err) { ... } instead of if (err !=
validationError.none) { ... }

[hansmuller, 2014/07/29 19:06:23]

A good suggestion and I tried it but then reverted. I liked the reminder about
the return value that's apparent in:

    if (v.validateMessage() != validator.validationError.NONE)
      this.close();

+    MISALIGNED_OBJECT: 'VALIDATION_ERROR_MISALIGNED_OBJECT',
+    ILLEGAL_MEMORY_RANGE: 'VALIDATION_ERROR_ILLEGAL_MEMORY_RANGE',
+    UNEXPECTED_STRUCT_HEADER: 'VALIDATION_ERROR_UNEXPECTED_STRUCT_HEADER',
+    UNEXPECTED_ARRAY_HEADER: 'VALIDATION_ERROR_UNEXPECTED_ARRAY_HEADER',
+    ILLEGAL_HANDLE: 'VALIDATION_ERROR_ILLEGAL_HANDLE',
+    ILLEGAL_POINTER: 'VALIDATION_ERROR_ILLEGAL_POINTER',
+    MESSAGE_HEADER_INVALID_FLAG_COMBINATION:
+        'VALIDATION_ERROR_MESSAGE_HEADER_INVALID_FLAG_COMBINATION',
+    MESSAGE_HEADER_MISSING_REQUEST_ID:
+        'VALIDATION_ERROR_MESSAGE_HEADER_MISSING_REQUEST_ID'
+  };
+
+  function Validator(message, offset) {
+    this.message = message;
+    this.offset = offset;
+  }
+
+  Object.defineProperty(Validator.prototype, "offsetLimit", {
+    get: function() { return this.message.buffer.byteLength; }
+  });
+
+  Validator.prototype.claimMemory = function(numBytes) {

[yzshen1, 2014/07/29 05:59:53]

The objects are not guaranteed to be stored in the message continuously.
(Alignment requirement; unrecognized region from newer versions, etc.)

[hansmuller, 2014/07/29 19:06:23]

Sorry, about that, I've written this incorrectly. Two parameters are needed: the
offset from which the allocation is to start, and numBytes.

+    if (numBytes < 0 || this.offset + numBytes > this.offsetLimit)

[yzshen1, 2014/07/29 05:59:53]

Is it possible for the addition to overflow?
If not, is it useful to add a comment explaining that this operation is safe?

[Tom Sepez, 2014/07/29 16:32:54]

Addition in JS is safe. No need to comment.

[hansmuller, 2014/07/29 19:06:23]

Good point. Only positive JS integers that are less than 2^53
(Number.MAX_SAFE_INTEGER) are represented exactly, so overflow from integer to
floating point is a problem.

I've added checks to verify that the arithmetic doesn't overflow and a comment
explaining as much.

[yzshen1, 2014/07/30 17:14:56]

Does it make sense to add your comment to the code so that it is more obvious
what the code does?
"""Only positive JS integers that are less than 2^53
(Number.MAX_SAFE_INTEGER) are represented exactly, so overflow from integer to
floating point is a problem."""

[hansmuller, 2014/07/30 18:32:30]

Yes, I'll do that.

+      return false;
+    this.offset += numBytes;
+    return true;
+  }
+
+  Validator.prototype.validateStructHeader =
+      function(offset, minNumBytes, minNumFields) {
+    if (!codec.isAligned(offset))
+      return ValidationError.MISALIGNED_OBJECT;
+
+    if (offset + codec.kStructHeaderSize > this.offsetLimit)

[yzshen1, 2014/07/29 05:59:53]

This is inadequate: the offset may overlap a previous object, or violate the
depth-first layout order.
Please see the Mojo Archive Format and the Validate Algorithm.

It seems better to make this a separate method, just like claimMemory(). (Or
even group them as a separate class.)

[hansmuller, 2014/07/29 19:06:23]

I've made isValidRange() a separate method and corrected the validity test.

+      return ValidationError.ILLEGAL_MEMORY_RANGE;
+
+    var numBytes = this.message.buffer.getUint32(offset);
+    var numFields = this.message.buffer.getUint32(offset + 4);
+
+    if (numBytes < minNumBytes || numFields < minNumFields)
+      return ValidationError.UNEXPECTED_STRUCT_HEADER;
+
+    if (!this.claimMemory(numBytes))

[yzshen1, 2014/07/29 05:59:53]

It should claim the specified size starting from |offset| .

[hansmuller, 2014/07/29 19:06:23]

Yes. I've added a start parameter to claimMemory().

+      return ValidationError.ILLEGAL_MEMORY_RANGE;
+
+    return ValidationError.NONE;
+  }
+
+  Validator.prototype.validateMessageHeader = function() {
+    var numBytes = this.message.getHeaderNumBytes();
+    var numFields = this.message.getHeaderNumFields();
+
+    var validNumFieldsAndNumBytes =
+        (numFields == 2 && numBytes == codec.kMessageHeaderSize) ||
+        (numFields == 3 && numBytes == codec.kMessageWithRequestIDHeaderSize);
+    if (!validNumFieldsAndNumBytes)

[yzshen1, 2014/07/29 05:59:53]

This is incorrect. In the future, the message header may evolve to have more
fields than 3. An older implementation should ignore fields that it doesn't
understand.

(Please see the validation algorithm.)

[hansmuller, 2014/07/29 19:06:23]

I've added a numFields > 3 case. However:

From "Message Header Validation" (and per our conversation)

...
  else if (header.num_fields > 3) {
    if (header.num_bytes < sizeof(MessageHeaderWithRequestID)) {
      Report(VALIDATION_ERROR_UNEXPECTED_STRUCT_HEADER);
      return false;
    }
  }

If there are more than 3 message header fields, the header's length would have
to be > sizeof(MessageHeaderWithRequestID). In other words, this test should
probably be |header.num_bytes <= sizeof(MessageHeaderWithRequestID)|.

For now I've made the JS version consistent with existing test.

[yzshen1, 2014/07/30 17:14:56]

Yes, you are right. In this case, we could tighten the check if we want.

The reason why I am okay keeping it the current way: adding more fields to a
struct could remain the same size, if there is padding and the new fields could
fit into the padding according to our packing algorithm. So here the check is
consistent with the generic header check that we will do for structs in general.
(We haven't done that part because we haven't support backward compatibility.)

+      return ValidationError.UNEXPECTED_STRUCT_HEADER;
+
+    var expectsResponse = this.message.expectsResponse();
+    var isResponse = this.message.isResponse();
+
+    if (numFields == 2 && (expectsResponse || isResponse))
+      return ValidationError.MESSAGE_HEADER_MISSING_REQUEST_ID;
+
+    if (isResponse && expectsResponse)
+      return ValidationError.MESSAGE_HEADER_INVALID_FLAG_COMBINATION;
+
+    return ValidationError.NONE;
+  }
+
+  Validator.prototype.validateMessage = function() {
+    var err = this.validateStructHeader(0, codec.kStructHeaderSize, 2);
+    if (err != ValidationError.NONE)
+      return err;
+
+    return this.validateMessageHeader();
+  }
+
+  var exports = {};
+  exports.ValidationError = ValidationError;
+  exports.Validator = Validator;
+  return exports;
+});