tsan: strerror_r interceptor incorrectly handles result pointers to immutable storage

[vitalybuka]

Summary

This fixes sanitizer issue #696: when the GNU version of strerror_r returns a result pointer that doesn't match the input buffer, that result pointer is in fact a pointer to some immutable storage that is part of the library (namely sys_errlist). TSAN is currently recording a write to this location, which is incorrect.

[toddlipcon]

As a drive-by contributor, I didn't really know how to write a test for this. But I verified in my application that this fixes a false positive.

[eugenis]

For a test, look at test/tsan/race_on_puts.cc
The same template with strerror_r called in two threads should do the trick.

lib/sanitizer_common/sanitizer_common_interceptors.inc

3230

else COMMON_INTERCEPTOR_INITIALIZE_RANGE ( same args )
This would count as an initializing write for MSan, but not for TSan.

[eugenis]

test comment (the previous one went to the spam folder in gmail)

[toddlipcon]

I have some other high priority work for the next couple weeks, so may be a while before I can get back to this patch and add a test. If someone else is interested in carrying it over the finish line in the meantime, please feel free, otherwise I'm likely to pick it up in mid December or January.

[vitalybuka]

Add tests.

[vitalybuka]

Clang-format

[eugenis]

test/msan/Linux/strerror_r.c

14 ↗

(On Diff #101655)

I'd remove these pointer checks, they rely on libc implementation details.

test/tsan/strerror_r.cc

19

Is the barrier really needed?
Again, I'd remove the assert at line 20.

[vitalybuka]

Remove barrier and pointers checks

[eugenis]

LGTM

[vitalybuka]

Remove "UNSUPPORTED: android" and _GNU_SOURCE

[vitalybuka]

Somehow I missed review URL in my patch description, so closing manually.
https://llvm.org/svn/llvm-project/compiler-rt/trunk@304858

lib/sanitizer_common/sanitizer_common_interceptors.inc

 
 #define INIT_WCSNRTOMBS COMMON_INTERCEPT_FUNCTION(wcsnrtombs);
 #else
 #define INIT_WCSNRTOMBS
 #endif
 
 
 #if SANITIZER_INTERCEPT_WCRTOMB
 INTERCEPTOR(SIZE_T, wcrtomb, char *dest, wchar_t src, void *ps) {
   void *ctx;
   COMMON_INTERCEPTOR_ENTER(ctx, wcrtomb, dest, src, ps);
   if (ps) COMMON_INTERCEPTOR_READ_RANGE(ctx, ps, mbstate_t_sz);
   // FIXME: under ASan the call below may write to freed memory and corrupt
   // its metadata. See
   // https://github.com/google/sanitizers/issues/321.
   SIZE_T res = REAL(wcrtomb)(dest, src, ps);
   if (res != ((SIZE_T)-1) && dest) {

 // GNU version.
 INTERCEPTOR(char *, strerror_r, int errnum, char *buf, SIZE_T buflen) {
   void *ctx;
   COMMON_INTERCEPTOR_ENTER(ctx, strerror_r, errnum, buf, buflen);
   // FIXME: under ASan the call below may write to freed memory and corrupt
   // its metadata. See
   // https://github.com/google/sanitizers/issues/321.
   char *res = REAL(strerror_r)(errnum, buf, buflen);
+  if (res == buf)
     COMMON_INTERCEPTOR_WRITE_RANGE(ctx, res, REAL(strlen)(res) + 1);
+  else
+    COMMON_INTERCEPTOR_INITIALIZE_RANGE(res, REAL(strlen)(res) + 1);
   return res;
 }
 #endif //(_POSIX_C_SOURCE >= 200112L || _XOPEN_SOURCE >= 600) && !_GNU_SOURCE ||
        //SANITIZER_MAC
 #define INIT_STRERROR_R COMMON_INTERCEPT_FUNCTION(strerror_r);
 #else
 #define INIT_STRERROR_R
 #endif

lib/tsan/rtl/tsan_rtl_thread.cc

Printf("Bad shadow addr %p (%zx)\n",
                shadow_mem + size * kShadowCnt / 8 - 1, addr + size - 1);
     DCHECK(IsShadowMem((uptr)(shadow_mem + size * kShadowCnt / 8 - 1)));
   }
 #endif
 
   StatInc(thr, StatMopRange);
 
   if (*shadow_mem == kShadowRodata) {
+    DCHECK(!is_write);
     // Access to .rodata section, no races here.
     // Measurements show that it can be 10-20% of all memory accesses.
     StatInc(thr, StatMopRangeRodata);
     return;
   }
 
   FastState fast_state = thr->fast_state;
   if (fast_state.GetIgnoreBit())

test/msan/Linux/strerror_r.cc

+// RUN: %clang_msan -O0 -g %s -o %t && %run %t
+
+#include <assert.h>
+#include <errno.h>
+#include <string.h>
+
+int main() {
+  char buf[1000];
+  char *res = strerror_r(EINVAL, buf, sizeof(buf));
+  assert(res);
+  volatile int z = strlen(res);
+
+  res = strerror_r(-1, buf, sizeof(buf));
+  assert(res);
+  z = strlen(res);
+
+  return 0;
+}

test/tsan/strerror_r.cc

+// RUN: %clangxx_tsan -O1 -DTEST_ERROR=ERANGE %s -o %t && %run %t 2>&1 | FileCheck --check-prefixes=CHECK,CHECK-SYS %s
+// RUN: %clangxx_tsan -O1 -DTEST_ERROR=-1 %s -o %t && not %run %t 2>&1 | FileCheck --check-prefixes=CHECK,CHECK-USER %s
+
+#include <assert.h>
+#include <errno.h>
+#include <pthread.h>
+#include <stdio.h>
+#include <string.h>
+
+char buffer[1000];
+
+void *Thread(void *p) {
+  return strerror_r(TEST_ERROR, buffer, sizeof(buffer));
+}
+
+int main() {
+  pthread_t th[2];
+  pthread_create(&th[0], 0, Thread, 0);
+  pthread_create(&th[1], 0, Thread, 0);
+  pthread_join(th[0], 0);
+  pthread_join(th[1], 0);
+  fprintf(stderr, "DONE\n");
+}
+
+// CHECK-USER: WARNING: ThreadSanitizer: data race
+// CHECK-SYS-NOT: WARNING: ThreadSanitizer: data race
+
+// CHECK: DONE