Mojo cpp bindings: validation logic for incoming messages

mojo/public/cpp/bindings/lib/bounds_checker.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_BOUNDS_CHECKER_H_
+#define MOJO_PUBLIC_CPP_BINDINGS_LIB_BOUNDS_CHECKER_H_
+
+#include <stdint.h>
+
+#include <vector>
+
+#include "mojo/public/cpp/system/macros.h"
+
+namespace mojo {
+
+class Handle;
+
+namespace internal {
+
+// BoundsChecker is used to validate object sizes, pointers and handle indexes

[darin (slow to review), 2014/05/26 19:17:47]

Can we restrict the serialization format in ways that might trivialize bounds
checking? What if we had defined rules for serialization that required 1) that
pointers can only be non-negative, 2) that pointed to objects are serialized in
ordinal order, 3) that serialization is depth-first. Then we could probably
implement bounds checking with a min-offset counter, right?

[yzshen1, 2014/05/26 21:30:56]

We are already enforcing this. (The offset is defined as an unsigned integer.
That ensures we never get into a loop when validating/decoding a message.)
With regard to (2) and (3), I also considered this approach. (In the mail thread
"a corrupt message".)
At that time, I thought:
- We might want to preserve the flexibility of serialization. For example, with
the more-strict-serialization-format approach we usually need to do a clone to
arrange object orders before sending out a message. Maybe in some cases we would
like to avoid that? (I don't have good examples in mind, maybe for in-process
message passing?)

- Instead of enforcing the traversal order in the message format spec, maybe we
could make it some "best effort" behavior, and suggest bindings to follow the
rules whenever possible. The current cpp binding implementation follows these
rules for both validating and decoding, so BoundsChecker should be (almost) as
efficient as a min-offset counter.

That being said, I agree that the more-strict-serialization-format approach
results in simpler code. If we don't mind being a little less flexible, I am
fine switching to that approach. The change should be trivial.

What do you think?  Thanks!

+// for payload of incoming messages.
+class BoundsChecker {
+ public:
+  // [data, data_num_bytes) specifies the valid memory range.
+  // [0, num_handles) specifies the valid range of handle indexes.
+  BoundsChecker(const void* data, uint32_t data_num_bytes,
+                size_t num_handles);
+
+  ~BoundsChecker();
+
+  // Claims the specified memory range.
+  // The method fails if the range overlaps claimed ranges, or it is not
+  // contained inside the valid memory range, or it is of zero length.
+  bool ClaimMemory(const void* position, uint32_t num_bytes);
+
+  // Claims the specified encoded handle (which is basically a handle index).
+  // The method fails if the handle has already been claimed or it is outside
+  // of the valid range. The method succeeds if |encoded_handle|'s value is
+  // |kEncodedInvalidHandleValue|.
+  bool ClaimHandle(const Handle& encoded_handle);
+
+  // Returns true if the specified range is contained inside the valid memory
+  // range, no matter whether the range overlaps claimed ranges or not.
+  // The method returns false if the range is of zero length.
+  bool IsWithinBounds(const void* position, uint32_t num_bytes) const;
+
+  const std::vector<uintptr_t>& GetClaimedRangesForTesting() const;
+
+ private:
+  void InsertOrMergeClaimedRange(std::vector<uintptr_t>::iterator pos,
+                                 uintptr_t begin, uintptr_t end);
+
+  bool InternalIsWithinBounds(uintptr_t begin, uintptr_t end) const;
+
+  // The valid memory range is [data_begin_, data_end_).
+  uintptr_t data_begin_;
+  uintptr_t data_end_;
+
+  // This vector is sorted in increasing order, with an even number of elements.
+  // The (2*i)-th and (2*i+1)-th element form a claimed range:
+  // [claimed_ranges_[2*i], claimed_ranges_[2*i+1]).
+  // (Please note that |claimed_ranges_[2*i+1]| itself is not included in the
+  // range.)
+  std::vector<uintptr_t> claimed_ranges_;
+
+  std::vector<bool> claimed_handles_;
+
+  MOJO_DISALLOW_COPY_AND_ASSIGN(BoundsChecker);
+};
+
+}  // namespace internal
+}  // namespace mojo
+
+#endif  // MOJO_PUBLIC_CPP_BINDINGS_LIB_BOUNDS_CHECKER_H_