[libcxx-commits] [PATCH] D147089: [libc++] Add assertions for potential OOB reads in std::sort

[Louis Dionne via Phabricator via libcxx-commits]

ldionne created this revision.
Herald added a subscriber: mgrang.
Herald added a project: All.
ldionne requested review of this revision.
Herald added a project: libc++.
Herald added a subscriber: libcxx-commits.
Herald added a reviewer: libc++.

We introduced an optimization to std::sort in 4eddbf9f10a6 <https://reviews.llvm.org/rG4eddbf9f10a6d1881c93d84f4363d6d881daf848>. However,
that optimization led to issues where users that were passing invalid
comparators to std::sort could start seeing OOB reads. This led to
the revert of the std::sort optimization from the LLVM 16 release
(see https://llvm.org/D146421).

This patch introduces _LIBCPP_ASSERTs to the places in the algorithm
where we make an assumption that the comparator will be consistent and
hence avoid a bounds check based on that. If the comparator happens not
to be consistent with itself, these are the places where we would
incorrectly go out of bounds. This allows users that enable libc++
assertions to catch such misuse at the cost of basically a bounds
check. For users that do not enable libc++ assertions (which is 99.9%
of users since assertions are off by default), this is basically a
no-op, and in fact the assertion will turn into a __builtin_assume,
making it explicit to the compiler that it can rely on the fact that
we're not going out of bounds.

I think this patch strikes the right balance. Folks that want absolute
performance will get what they want, since it is a precondition for the
comparator to be consistent, so the bounds checks are technically not
mandatory. Folks who want more safety *already* need to be enabling
libc++ assertions to catch other types of bugs (like operator[] OOB),
so this solution should also work for them.

I do think we have a lot of work towards popularizing the use of libc++
assertions and integrating it better so that users don't have to know
about the obscure _LIBCPP_ENABLE_ASSERTIONS macro to enable them, but
that's a separate concern.

rdar://106897934


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D147089

Files:
  libcxx/include/__algorithm/sort.h
  libcxx/test/libcxx/algorithms/alg.sorting/assert.sort.invalid_comparator.pass.cpp
  libcxx/test/libcxx/algorithms/alg.sorting/bad_comparator_values.dat

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D147089.509142.patch
Type: text/x-patch
Size: 41041 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/libcxx-commits/attachments/20230328/d5f8ecb1/attachment-0001.bin>