[libc-commits] [PATCH] D152322: [libc] Temporarily suppress -fsanitize=function for qsort comparator

[David Benjamin via Phabricator via libc-commits]

davidben added inline comments.


================
Comment at: libc/src/stdlib/qsort.cpp:50
+  // void pointers. Ideally those tools would pass a function that strictly
+  // accepts const void*s to avoid UB, or we'd have something like qsort_r/s.
+  [[clang::no_sanitize("function")]]
----------------
davidben wrote:
> Since BoringSSL is cited, I'll go ahead and explain. BoringSSL's constraint is that our public API (and OpenSSL's) allows callers to pass pointers that with a particular type signature. Specifically, we have the equivalent of a `std::vector<T*>` (though stored as `void*`s because the underlying implementation is type-erased) and callers pass comparators that take `int(*)(const T *const *p, const T *const *p)`.
> 
> This is the exact same calling convention as `qsort` (pointers to the elements of the array), but without the type erasure. The problem is casting the function pointer types is UB. Since it's public API, we can't change that easily. But also typed interfaces are good, so I'm not especially inclined to make it `void*`-based.
> 
> Short of stashing the comparator in a thread-local (which would risk reentrancy problems if someone sorts in their comparator), it is not possible to use this function pointer with `qsort`. `qsort`'s comparator is missing the extra context parameter of `qsort_s` and `qsort_r`. Unfortunately, `qsort_s` and `qsort_r` are not reliably available (LLVM libc among the offenders). Where they are available, they're [[ https://stackoverflow.com/a/39561369 | inconsistently defined ]].
> 
> `bsearch` has the same problem, and we eventually just gave up on the libc and [[ https://boringssl-review.googlesource.com/c/boringssl/+/35304 | wrote our own ]]. We had not yet given up on the libc for sorting but, with this issue, we [[ https://boringssl-review.googlesource.com/c/boringssl/+/60507 | now have ]]. So this workaround is not necessary for the latest BoringSSL. At the same time, this is absurd. That we had to implement our own sorting function is a failure of the toolchain to provide usable tools, so have a vote from me that you all implement `qsort_r` or `qsort_s`.
> 
> In particular, "Ideally those tools would pass a function that strictly accepts const void*s to avoid UB" is not viable on its own. LLVM libc would need to provide a better API first. Instead, we had to take the "give up on the libc" route.
(Also no need to cite BoringSSL in the comment anymore, as we've since stopped calling `qsort` here. :-) )


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152322/new/

https://reviews.llvm.org/D152322