[libcxx-commits] [libcxx] [libc++][hardening] Check bounds on arithmetic in __bounded_iter (PR #78876)

[David Benjamin via libcxx-commits]

================
@@ -31,13 +31,10 @@ _LIBCPP_BEGIN_NAMESPACE_STD
 // Iterator wrapper that carries the valid range it is allowed to access.
 //
 // This is a simple iterator wrapper for contiguous iterators that points
-// within a [begin, end) range and carries these bounds with it. The iterator
-// ensures that it is pointing within that [begin, end) range when it is
-// dereferenced.
-//
-// Arithmetic operations are allowed and the bounds of the resulting iterator
-// are not checked. Hence, it is possible to create an iterator pointing outside
-// its range, but it is not possible to dereference it.
+// within a [begin, end] range and carries these bounds with it. The iterator
+// ensures that it is pointing within [begin, end) range when it is
+// dereferenced. It also ensures that it is never iterated outside of
+// [begin, end].
 template <class _Iterator, class = __enable_if_t< __libcpp_is_contiguous_iterator<_Iterator>::value > >
----------------
davidben wrote:

I think it would _work_ with every random access iterator, in the sense of producing coherent behavior, but there are a couple of subtleties:

1. A general random access iterator (I'm thinking `std::map`) may have slightly different performance properties. In particular, I don't think we'd want to write `begin + n` in `<algorithm>` with `std::map` because that'll end up walking the iterator a second time, and I'm not positive the first walk can be easily deleted. And since we don't unwrap the map iterators in `std::copy`, it's not necessary to check ahead of time. We can just lean on the wrapped `operator++` check.

2. Replacing `iter` with `iter2 = bounded(begin, iter, end)` means that `iter2` becomes invalidated whenever `end` gets invalidated, not just `iter`. That's fine for contiguous ranges because the end iterator is just another pointer into the allocation. But the more complex containers, irritatingly, have different invalidation rules for `end` and general iterators! See https://github.com/llvm/llvm-project/issues/80212#issuecomment-1930063010 where I considered and then sadly had to reject basically this strategy for `std::map`.

3. We may not need to store both `begin` and `end` for a general random access iterator. For example, if we ignored the iterator invalidation problem above, `std::map` only needs `(iter, end)`. `begin` is only used to stop an errant `operator--`, and we can already detect that from the tree structure itself.

So my inclination would be to wait to see how we decide to solve the problem for non-contiguous random access and then make a decision about whether to generalize `__bounded_iter` then. My guess would be that we wouldn't end up wanting to reuse it anyway.

https://github.com/llvm/llvm-project/pull/78876