[libcxx-commits] [libcxx] [libc++][hardening] Use bounded iterators in std::vector and std::string (PR #78929)

[Konstantin Varlamov via libcxx-commits]

https://github.com/var-const approved this pull request.

@davidben Thank you so much for working on this! (And my sincere apologies for the very slow review -- I wanted to get to this for a while but unfortunately had my plate very full recently) LGTM.

My biggest concern starting this review was whether accepting this lightweight approach would prevent us from adding heavier iterators in the future that would retain a connection with the container and thus could also catch temporal safety issues, such as invalidation. That would bring the current incarnation of hardening on par with the historical debug mode. However, since we decided to create separate ABI macros for these new iterators, this is no longer an issue. While it's a tradeoff (having several independent ABI configuration options creates many possible combinations, increasing complexity), I believe it's probably an inevitable outcome -- for one, I don't think it's reasonable to force anyone using the current `bounded-iterators` macro to start using bounded iterators in their vectors and strings, especially with no targeted opt-out possible, but also I think there might be legitimate contexts where someone might want bounded iterators in some, but not all, containers. The ability to choose (in the future, potentially) between more and less lightweight iterator hardening strategies follows quite nicely from this, kind of mirroring the distinction between the `fast` and `extensive` modes.

The idea to bound the vector iterator by capacity has grown on me. It is slightly unfortunate that there is a discrepancy between how checking is performed for strings versus vectors, but it seems like a necessary evil -- the only way to reconsolidate would be to make string checks laxer. Checking just the "physical" bounds prevents the worst vulnerabilities, so I believe we're still getting the most valuable part of the checks; I'd also note that IIUC, this is how Asan checks work for contiguous containers that are not specially annotated for the sanitizer.

I think that even without covering iterator invalidation (which, being a temporal safety issue, is much harder to solve in general anyway), this patch provides a noticeable improvement in the amount of checks we provide with little added code complexity, and the fact that the bound iterators are comparatively lightweight can be an advantage for some contexts.

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