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
+// for payload of incoming messages.
+class BoundsChecker {
+ public:
+  // [|data|, |data_num_bytes|) specifies the valid memory range.

[Tom Sepez, 2014/05/22 19:39:21]

nit: not sure vertical bars help inside the [) range syntax, but I may be wrong.

[yzshen1, 2014/05/22 20:56:22]

Done, I removed them.

+  // [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).

[Tom Sepez, 2014/05/22 19:39:21]

nit: blank line here.

[yzshen1, 2014/05/22 20:56:22]

Done.

+  // The method fails if the handle has already been claimed or it is outside
+  // of the valid range. The method succeeds if |encoded_handle| is invalid
+  // handle.

[Tom Sepez, 2014/05/22 19:39:21]

nit: while absolutely true, this is a strange sentence, we may have to call out
kInvalidHandle specifically as the placeholder for a valid "invalid handle" vs.
invalid invalid handles.

[yzshen1, 2014/05/22 20:56:22]

Revised the comment.

+  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.

[Tom Sepez, 2014/05/22 19:39:21]

I think you'll want a vector of struct Range { uinptr_t begin; uinptr_t end } or
equivalent here otherwise the arithmetic in the .cc file will be more confusing.
 Having done so the data_begin_ and data_end_ above might be ranges, too.

[yzshen1, 2014/05/22 20:56:22]

I did consider this alternative, but think that the current approach is a little
cleaner: the binary search in BoundsChecker::ClaimMemory() doesn't have to care
about range begin and end. It simply treats all elements of |claimed_ranges_| in
the same way. I think it results in less comparisons as well.

What do you think?

+  // 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_