This is an archive of the discontinued LLVM Phabricator instance.

[libc] Temporarily suppress -fsanitize=function for qsort comparator
ClosedPublic

Authored by leonardchan on Jun 6 2023, 5:06 PM.

Details

Summary

Recent upstream changes to -fsanitize=function find more instances of function type mismatches. One case is with the comparator passed to this class. Libraries like boringssl will tend to pass comparators that take pointers to varying types while this comparator expects to accept const 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.

Diff Detail

Event Timeline

leonardchan created this revision.Jun 6 2023, 5:06 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 6 2023, 5:06 PM
leonardchan requested review of this revision.Jun 6 2023, 5:06 PM
abrachet accepted this revision.Jun 6 2023, 5:19 PM
This revision is now accepted and ready to land.Jun 6 2023, 5:19 PM
michaelrj accepted this revision.Jun 7 2023, 11:05 AM
michaelrj added a subscriber: michaelrj.

LGTM

davidben added inline comments.
libc/src/stdlib/qsort.cpp
50

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 inconsistently defined.

bsearch has the same problem, and we eventually just gave up on the libc and wrote our own. We had not yet given up on the libc for sorting but, with this issue, we 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.

davidben added inline comments.Jun 7 2023, 12:09 PM
libc/src/stdlib/qsort.cpp
50

(Also no need to cite BoringSSL in the comment anymore, as we've since stopped calling qsort here. :-) )