[all-commits] [llvm/llvm-project] 36d8b4: [libc++] Add assertions for potential OOB reads in...

[Louis Dionne via All-commits]

  Branch: refs/heads/main
  Home:   https://github.com/llvm/llvm-project
  Commit: 36d8b449cfc9850513bb2ed6c07b5b8cc9f1ae3a
      https://github.com/llvm/llvm-project/commit/36d8b449cfc9850513bb2ed6c07b5b8cc9f1ae3a
  Author: Louis Dionne <ldionne.2 at gmail.com>
  Date:   2023-05-09 (Tue, 09 May 2023)

  Changed paths:
    M libcxx/docs/ReleaseNotes.rst
    M libcxx/include/__algorithm/sort.h
    A libcxx/test/libcxx/algorithms/alg.sorting/assert.sort.invalid_comparator.pass.cpp
    A libcxx/test/libcxx/algorithms/alg.sorting/bad_comparator_values.dat
    M libcxx/test/support/check_assertion.h

  Log Message:
  -----------
  [libc++] Add assertions for potential OOB reads in std::sort

We introduced an optimization to std::sort in 4eddbf9f10a6. 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

Differential Revision: https://reviews.llvm.org/D147089