Page MenuHomePhabricator

[msan] Intercept qsort, qsort_r.
ClosedPublic

Authored by eugenis on Dec 19 2019, 5:48 PM.

Details

Summary

This fixes qsort-related false positives with glibc-2.27.
I'm not entirely sure why they did not show up with the earlier
versions; the code seems similar enough.

Diff Detail

Event Timeline

eugenis created this revision.Dec 19 2019, 5:48 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptDec 19 2019, 5:48 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript
vitalybuka added inline comments.Dec 19 2019, 6:07 PM
compiler-rt/lib/sanitizer_common/sanitizer_common_interceptors.inc
9686

you can wrap <qsort_r_size, qsort_r_compar, arg> and pass as arg

Unit tests: pass. 61035 tests passed, 0 failed and 728 were skipped.

clang-tidy: fail. Please fix clang-tidy findings.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

eugenis marked an inline comment as done.Dec 20 2019, 10:42 AM
eugenis added inline comments.
compiler-rt/lib/sanitizer_common/sanitizer_common_interceptors.inc
9686

Yes, but I can not do the same in qsort(). I kind of prefer the two interceptors to do the same thing to avoid introducing new, unique bugs in qsort_r, but I don't have a strong feeling about it.

I could not use qsort_r to implement the interceptor for qsort, because it does not exist on the same set of platforms.

vitalybuka added inline comments.Dec 20 2019, 11:03 AM
compiler-rt/lib/sanitizer_common/sanitizer_common_interceptors.inc
9659

do we need COMMON_INTERCEPTOR_READ_RANGE(ctx, src, size); for a and b?
or we going to assume that compar is instrumented?

9661

Problem here that if array has uninitialized fields they are going to be unpoisoned by compar
maybe before calling actual qsort we can call
compar(base[0], base[1])
compar(base[1], base[2])
compar(base[3], base[4])
...
just to trigger report of a bug is there?

9686

this way you can't call qsort from qsort_compar recursively :)

vitalybuka added inline comments.Dec 20 2019, 11:05 AM
compiler-rt/test/msan/qsort.cpp
4

looks like we need .clang-tidy

eugenis marked 4 inline comments as done.Dec 20 2019, 12:16 PM
eugenis added inline comments.
compiler-rt/lib/sanitizer_common/sanitizer_common_interceptors.inc
9659

compar is typically instrumented.
We don't want to check the shadow here. We want to clear it.

9661

This is a great idea. Ill run compar(x, x) for each node to trigger a check for equality which usually needs to exercise as much of the comparison logic as possible.

But I think I'll do it in a separate change for easier bisection and reverting in case something goes wrong somewhere.

9686

I can. See old_compar and old_size.

compiler-rt/test/msan/qsort.cpp
4

yeah... not in this change

eugenis updated this revision to Diff 234945.Dec 20 2019, 12:17 PM

addressed comments

vitalybuka accepted this revision.Dec 20 2019, 12:23 PM
This revision is now accepted and ready to land.Dec 20 2019, 12:23 PM

If possible, it would be nice to follow up with heapsort(3) and mergesort(3).

Unit tests: pass. 61055 tests passed, 0 failed and 728 were skipped.

clang-tidy: fail. Please fix clang-tidy findings.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

This revision was automatically updated to reflect the committed changes.

If possible, it would be nice to follow up with heapsort(3) and mergesort(3).

Would be nice, but on gnu/linux these functions are in libbsd.so; if we provide an interceptor then we must link the implementation as well to not confuse configure-style checks. This would make all sanitized binaries on linux link libbsd.so even if they don't use it... I'm not sure that's OK.

I've reverted this commit because our bots have been broken since this landed. Can you please keep an eye on these bots when you re-land this? Thanks!

Revert "[msan] Check qsort input." and "[msan] Intercept qsort, qsort_r."

Temporarily revert the qsort changes because they fail to build on bots
that build with modules:

error: thread-local storage is not supported for the current
target (iossim)

http://green.lab.llvm.org/green/job/clang-stage2-Rthinlto/1820/console
http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/4983/console

This reverts commit ddf897fc80499ece298bc33201db6b697d2af50e.
This reverts commit 07861e955d0095f25639d84c5726c73b528567cb.

Sorry about that! I have not received a mail notification about this - apparently, there was another build break at the same time.
I'll disable the new interceptors on ios and reland the changes.