[PATCH] D71740: [msan] Intercept qsort, qsort_r.

[Evgenii Stepanov via Phabricator via llvm-commits]

eugenis marked 4 inline comments as done.
eugenis added inline comments.


================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_common_interceptors.inc:9659
+  COMMON_INTERCEPTOR_UNPOISON_PARAM(2);
+  COMMON_INTERCEPTOR_INITIALIZE_RANGE(a, qsort_size);
+  COMMON_INTERCEPTOR_INITIALIZE_RANGE(b, qsort_size);
----------------
vitalybuka wrote:
> do we need  COMMON_INTERCEPTOR_READ_RANGE(ctx, src, size);  for a and b?
> or we going to assume that compar is instrumented?
compar is typically instrumented.
We don't want to check the shadow here. We want to clear it.



================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_common_interceptors.inc:9661
+  COMMON_INTERCEPTOR_INITIALIZE_RANGE(b, qsort_size);
+  return qsort_compar(a, b);
+}
----------------
vitalybuka wrote:
> 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?
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.


================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_common_interceptors.inc:9686
+static THREADLOCAL SIZE_T qsort_r_size;
+int wrapped_qsort_r_compar(const void *a, const void *b, void *arg) {
+  COMMON_INTERCEPTOR_UNPOISON_PARAM(3);
----------------
vitalybuka wrote:
> eugenis wrote:
> > vitalybuka wrote:
> > > you can wrap <qsort_r_size, qsort_r_compar, arg> and pass as arg
> > 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.
> this way you can't call qsort from qsort_compar recursively :)
I can. See old_compar and old_size.


================
Comment at: compiler-rt/test/msan/qsort.cpp:3
+
+#include <assert.h>
+#include <glob.h>
----------------
vitalybuka wrote:
> looks like we need .clang-tidy
yeah... not in this change


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71740