[sanitizer] No THREADLOCAL in qsort and bsearch

[vitalybuka]

Summary

qsort can reuse qsort_r if available.
bsearch always passes key as the first comparator argument, so we
can use it to wrap the original comparator.

[eugenis]

this looks good. Too good, even. Why did not we do it before?

compiler-rt/lib/sanitizer_common/sanitizer_platform_interceptors.h

573

I'd rather place this logic in the .inc file - i.e. define the thread-local qsort interceptor if SANITIZER_INTERCEPT_QSORT && !SANITIZER_INTERCEPT_QSORT_R

[vitalybuka]

move !SANITIZER_INTERCEPT_QSORT_R

[vitalybuka]

fix build

[vitalybuka]

clang-format

[eugenis]

compiler-rt/lib/sanitizer_common/sanitizer_common_interceptors.inc

9952

do you need a !qsort_r check here to avoid defining two interceptors for qsort?

9953

Wouldn't it be easier to call the qsort_r interceptor directly?

9955

Let's avoid nested ifdef-else-endif-else-endif?
#if SANITIZER_INTERCEPT_QSORT && SANITIZER_INTERCEPT_QSORT_R

[vitalybuka]

cleanup

compiler-rt/lib/sanitizer_common/sanitizer_common_interceptors.inc

9952

it was not needed with nested ifs

[vitalybuka]

restore new line

[vitalybuka]

fix build

[eugenis]

LGTM

[vitalybuka]

simplify ifs

compiler-rt/lib/sanitizer_common/sanitizer_common_interceptors.inc

enum {
     CIMT_FILE
   } type;
   union {
     FileMetadata file;
   };
 };
 
 #if SI_POSIX
 typedef AddrHashMap<CommonInterceptorMetadata, 31051> MetadataHashMap;
 
 static MetadataHashMap *interceptor_metadata_map;
 
 UNUSED static void SetInterceptorMetadata(__sanitizer_FILE *addr,
                                           const FileMetadata &file) {
   MetadataHashMap::Handle h(interceptor_metadata_map, (uptr)addr);
   CHECK(h.created());
   h->type = CommonInterceptorMetadata::CIMT_FILE;
   h->file = file;
 }
 
 UNUSED static const FileMetadata *GetInterceptorMetadata(
     __sanitizer_FILE *addr) {

 
 UNUSED static void DeleteInterceptorMetadata(void *addr) {
   MetadataHashMap::Handle h(interceptor_metadata_map, (uptr)addr, true);
   CHECK(h.exists());
 }
 #endif  // SI_POSIX
 
 #if SANITIZER_INTERCEPT_STRLEN
 INTERCEPTOR(SIZE_T, strlen, const char *s) {
   // Sometimes strlen is called prior to InitializeCommonInterceptors,
   // in which case the REAL(strlen) typically used in
   // COMMON_INTERCEPTOR_ENTER will fail.  We use internal_strlen here
   // to handle that.
   if (COMMON_INTERCEPTOR_NOTHING_IS_INITIALIZED)
     return internal_strlen(s);
   void *ctx;
   COMMON_INTERCEPTOR_ENTER(ctx, strlen, s);
   SIZE_T result = REAL(strlen)(s);
   if (common_flags()->intercept_strlen)
     COMMON_INTERCEPTOR_READ_RANGE(ctx, s, result + 1);
   return result;
 }
 #define INIT_STRLEN COMMON_INTERCEPT_FUNCTION(strlen)
 #else
 #define INIT_STRLEN
 #endif
 
 #if SANITIZER_INTERCEPT_STRNLEN
 INTERCEPTOR(SIZE_T, strnlen, const char *s, SIZE_T maxlen) {
   void *ctx;
   COMMON_INTERCEPTOR_ENTER(ctx, strnlen, s, maxlen);
   SIZE_T length = REAL(strnlen)(s, maxlen);
   if (common_flags()->intercept_strlen)
     COMMON_INTERCEPTOR_READ_RANGE(ctx, s, Min(length + 1, maxlen));
   return length;
 }
 #define INIT_STRNLEN COMMON_INTERCEPT_FUNCTION(strnlen)
 #else
 #define INIT_STRNLEN
 #endif
 
 #if SANITIZER_INTERCEPT_STRNDUP
 INTERCEPTOR(char*, strndup, const char *s, uptr size) {
   void *ctx;
   COMMON_INTERCEPTOR_STRNDUP_IMPL(ctx, s, size);
 }
 #define INIT_STRNDUP COMMON_INTERCEPT_FUNCTION(strndup)
 #else
 #define INIT_STRNDUP
 #endif // SANITIZER_INTERCEPT_STRNDUP
 
INTERCEPTOR(int, getentropy, void *buf, SIZE_T buflen) {
   }
   return r;
 }
 #define INIT_GETENTROPY COMMON_INTERCEPT_FUNCTION(getentropy)
 #else
 #define INIT_GETENTROPY
 #endif
 
 #if SANITIZER_INTERCEPT_QSORT
 // Glibc qsort uses a temporary buffer allocated either on stack or on heap.
 // Poisoned memory from there may get copied into the comparator arguments,
 // where it needs to be dealt with. But even that is not enough - the results of
 // the sort may be copied into the input/output array based on the results of
 // the comparator calls, but directly from the temp memory, bypassing the
 // unpoisoning done in wrapped_qsort_compar. We deal with this by, again,
 // unpoisoning the entire array after the sort is done.
 //
 // We can not check that the entire array is initialized at the beginning. IMHO,
 // it's fine for parts of the sorted objects to contain uninitialized memory,
 // ex. as padding in structs.

 }
 #define INIT_QSORT COMMON_INTERCEPT_FUNCTION(qsort)
 #else
 #define INIT_QSORT
 #endif
 
 #if SANITIZER_INTERCEPT_QSORT_R
 typedef int (*qsort_r_compar_f)(const void *, const void *, void *);
-static THREADLOCAL qsort_r_compar_f qsort_r_compar;
-static THREADLOCAL SIZE_T qsort_r_size;
+struct qsort_r_compar_params {
+  SIZE_T size;
+  qsort_r_compar_f compar;
+  void *arg;
+};
 static int wrapped_qsort_r_compar(const void *a, const void *b, void *arg) {
+  qsort_r_compar_params *params = (qsort_r_compar_params *)arg;
   COMMON_INTERCEPTOR_UNPOISON_PARAM(3);
-  COMMON_INTERCEPTOR_INITIALIZE_RANGE(a, qsort_r_size);
-  COMMON_INTERCEPTOR_INITIALIZE_RANGE(b, qsort_r_size);
-  return qsort_r_compar(a, b, arg);
+  COMMON_INTERCEPTOR_INITIALIZE_RANGE(a, params->size);
+  COMMON_INTERCEPTOR_INITIALIZE_RANGE(b, params->size);
+  return params->compar(a, b, params->arg);
 }
-
 INTERCEPTOR(void, qsort_r, void *base, SIZE_T nmemb, SIZE_T size,
             qsort_r_compar_f compar, void *arg) {
   void *ctx;
   COMMON_INTERCEPTOR_ENTER(ctx, qsort_r, base, nmemb, size, compar, arg);
   // Run the comparator over all array elements to detect any memory issues.
   if (nmemb > 1) {
     for (SIZE_T i = 0; i < nmemb - 1; ++i) {
       void *p = (void *)((char *)base + i * size);
       void *q = (void *)((char *)base + (i + 1) * size);
       COMMON_INTERCEPTOR_UNPOISON_PARAM(3);
       compar(p, q, arg);
     }
   }
-  qsort_r_compar_f old_compar = qsort_r_compar;
-  SIZE_T old_size = qsort_r_size;
-  // Handle qsort_r() implementations that recurse using an
-  // interposable function call:
-  bool already_wrapped = compar == wrapped_qsort_r_compar;
-  if (already_wrapped) {
-    // This case should only happen if the qsort() implementation calls itself
-    // using a preemptible function call (e.g. the FreeBSD libc version).
-    // Check that the size and comparator arguments are as expected.
-    CHECK_NE(compar, qsort_r_compar);
-    CHECK_EQ(qsort_r_size, size);
-  } else {
-    qsort_r_compar = compar;
-    qsort_r_size = size;
-  }
-  REAL(qsort_r)(base, nmemb, size, wrapped_qsort_r_compar, arg);
-  if (!already_wrapped) {
-    qsort_r_compar = old_compar;
-    qsort_r_size = old_size;
-  }
+  qsort_r_compar_params params = {size, compar, arg};
+  REAL(qsort_r)(base, nmemb, size, wrapped_qsort_r_compar, &params);
+  COMMON_INTERCEPTOR_WRITE_RANGE(ctx, base, nmemb * size);
+
+// Re-use qsort_r if it's intercepted.
+typedef int (*qsort_compar_f)(const void *, const void *);
+struct qsort_compar_params {
+  SIZE_T size;
+  qsort_compar_f compar;
+};
+static int wrapped_qsort_compar(const void *a, const void *b, void *arg) {
+  qsort_compar_params *params = (qsort_compar_params *)arg;
+  COMMON_INTERCEPTOR_UNPOISON_PARAM(2);
+  COMMON_INTERCEPTOR_INITIALIZE_RANGE(a, params->size);
+  COMMON_INTERCEPTOR_INITIALIZE_RANGE(b, params->size);
+  return params->compar(a, b);
+INTERCEPTOR(void, qsort, void *base, SIZE_T nmemb, SIZE_T size,
+            qsort_compar_f compar) {
+  void *ctx;
+  COMMON_INTERCEPTOR_ENTER(ctx, qsort, base, nmemb, size, compar);
+  // Run the comparator over all array elements to detect any memory issues.
+  if (nmemb > 1) {
+    for (SIZE_T i = 0; i < nmemb - 1; ++i) {
+      void *p = (void *)((char *)base + i * size);
+      void *q = (void *)((char *)base + (i + 1) * size);
+      COMMON_INTERCEPTOR_UNPOISON_PARAM(2);
+      compar(p, q);
+    }
+  }
+  qsort_compar_params params = {size, compar};
+  REAL(qsort_r)(base, nmemb, size, wrapped_qsort_compar, &params);
   COMMON_INTERCEPTOR_WRITE_RANGE(ctx, base, nmemb * size);
 }
-#define INIT_QSORT_R COMMON_INTERCEPT_FUNCTION(qsort_r)
+
+#  define INIT_QSORT_R                  \
+    COMMON_INTERCEPT_FUNCTION(qsort_r); \
+    COMMON_INTERCEPT_FUNCTION(qsort)
 #else
 #define INIT_QSORT_R
 #endif
 
 #if SANITIZER_INTERCEPT_BSEARCH
 typedef int (*bsearch_compar_f)(const void *, const void *);
-static THREADLOCAL bsearch_compar_f bsearch_compar;
-static int wrapped_bsearch_compar(const void *a, const void *b) {
+struct bsearch_compar_params {
+  const void *key;
+  bsearch_compar_f compar;
+};
+
+static int wrapped_bsearch_compar(const void *key, const void *b) {
+  const bsearch_compar_params *params = (const bsearch_compar_params *)key;
   COMMON_INTERCEPTOR_UNPOISON_PARAM(2);
-  return bsearch_compar(a, b);
+  return params->compar(params->key, b);
 }
 
 INTERCEPTOR(void *, bsearch, const void *key, const void *base, SIZE_T nmemb,
             SIZE_T size, bsearch_compar_f compar) {
   void *ctx;
   COMMON_INTERCEPTOR_ENTER(ctx, bsearch, key, base, nmemb, size, compar);
-  // Unlike qsort, don't expect recursive implementation of bsearch.
-  CHECK_NE(compar, wrapped_bsearch_compar);
-  Swap(bsearch_compar, compar);
-  void *r = REAL(bsearch)(key, base, nmemb, size, wrapped_bsearch_compar);
-  bsearch_compar = compar;
-  return r;
+  bsearch_compar_params params = {key, compar};
+  return REAL(bsearch)(&params, base, nmemb, size, wrapped_bsearch_compar);
 }
 #  define INIT_BSEARCH COMMON_INTERCEPT_FUNCTION(bsearch)
 #else
 #  define INIT_BSEARCH
 #endif
 
 #if SANITIZER_INTERCEPT_SIGALTSTACK
 INTERCEPTOR(int, sigaltstack, void *ss, void *oss) {

compiler-rt/lib/sanitizer_common/sanitizer_platform_interceptors.h

 #define SANITIZER_INTERCEPT_CRYPT_R (SI_LINUX && !SI_ANDROID)
 
 #define SANITIZER_INTERCEPT_GETRANDOM \
   ((SI_LINUX && __GLIBC_PREREQ(2, 25)) || SI_FREEBSD)
 #define SANITIZER_INTERCEPT___CXA_ATEXIT SI_NETBSD
 #define SANITIZER_INTERCEPT_ATEXIT SI_NETBSD
 #define SANITIZER_INTERCEPT_PTHREAD_ATFORK SI_NETBSD
 #define SANITIZER_INTERCEPT_GETENTROPY SI_FREEBSD
-#define SANITIZER_INTERCEPT_QSORT \
-  (SI_POSIX && !SI_IOSSIM && !SI_WATCHOS && !SI_TVOS && !SI_ANDROID)
+#define SANITIZER_INTERCEPT_QSORT                                      \
+  (SI_POSIX && !SI_IOSSIM && !SI_WATCHOS && !SI_TVOS && !SI_ANDROID && \
+   !SI_GLIBC)
 #define SANITIZER_INTERCEPT_QSORT_R SI_GLIBC
 #define SANITIZER_INTERCEPT_BSEARCH \
   (SI_POSIX && !SI_IOSSIM && !SI_WATCHOS && !SI_TVOS && !SI_ANDROID)
 // sigaltstack on i386 macOS cannot be intercepted due to setjmp()
 // calling it and assuming that it does not clobber registers.
 #define SANITIZER_INTERCEPT_SIGALTSTACK \
   (SI_POSIX && !(SANITIZER_MAC && SANITIZER_I386))
 #define SANITIZER_INTERCEPT_UNAME (SI_POSIX && !SI_FREEBSD)