Page MenuHomePhabricator

Fix qsort() interceptor for FreeBSD
ClosedPublic

Authored by arichardson on Jul 24 2020, 3:55 AM.

Details

Summary

When the FreeBSD qsort() implementation recurses, it does so using an
interposable function call, so we end up calling the interceptor again
and set the saved comparator to wrapped_qsort_compar. This results in an
infinite loop and a eventually a stack overflow since wrapped_qsort_compar
ends up calling itself. This means that ASAN is completely broken on
FreeBSD for programs that call qsort(). I found this while running
check-all on a FreeBSD system a ASAN-instrumented LLVM.

Fix this by checking whether we are recursing inside qsort before writing
to qsort_compar. The same bug exists in the qsort_r interceptor, so use the
same approach there. I did not test the latter since the qsort_r function
signature does not match and therefore it's not intercepted on FreeBSD/macOS.

Fixes https://llvm.org/PR46832

Diff Detail

Event Timeline

arichardson created this revision.Jul 24 2020, 3:55 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 24 2020, 3:55 AM
Herald added subscribers: Restricted Project, krytarowski, emaste. · View Herald Transcript
eugenis added inline comments.Jul 24 2020, 9:42 AM
compiler-rt/lib/sanitizer_common/sanitizer_common_interceptors.inc
9864

I think we'd still want to save/restore the size argument, in the likely case when the recursive call is passed a different value.

arichardson marked an inline comment as done.Jul 26 2020, 3:41 AM
arichardson added inline comments.
compiler-rt/lib/sanitizer_common/sanitizer_common_interceptors.inc
9864

The size argument is the size of the elements which should not change in recursive calls. I could add an assertion/warning?

vitalybuka added inline comments.Jul 27 2020, 6:28 PM
compiler-rt/lib/sanitizer_common/sanitizer_common_interceptors.inc
9864

CHECK_EQ would be nice

eugenis accepted this revision.Jul 28 2020, 4:52 PM
eugenis added inline comments.
compiler-rt/lib/sanitizer_common/sanitizer_common_interceptors.inc
9864

Yes, I misunderstood the code.
LGTM with the check.

This revision is now accepted and ready to land.Jul 28 2020, 4:52 PM
  • Add CHECK()
  • Add new test
Hang added a subscriber: Hang.Aug 4 2020, 11:03 PM
Hang added inline comments.
compiler-rt/lib/sanitizer_common/sanitizer_common_interceptors.inc
9866

I think compar != qsort_compar here as compar == wrapped_sort_compar in this situation.

  • Fix inverted condition
  • Make arrays in the test case large enough to actually trigger the recursive case
eugenis accepted this revision.Aug 5 2020, 10:32 AM

LGTM

This revision was landed with ongoing or failed builds.Aug 6 2020, 1:17 AM
This revision was automatically updated to reflect the committed changes.