[Sanitizer][ASAN][MSAN] Fix infinite recursion on FreeBSD

[arichardson]

Summary

MSAN was broken on FreeBSD by https://reviews.llvm.org/D55703: after this
change accesses to the key variable call tls_get_addr, which is
intercepted. The interceptor then calls GetCurrentThread which calls
MsanTSDGet which again calls tls_get_addr, etc...
Using the default implementation in the SANITIZER_FREEBSD case fixes MSAN
for me.

I then applied the same change to ASAN (introduced in https://reviews.llvm.org/D55596)
but that did not work yet. In the ASAN case, we get infinite recursion
again during initialization, this time because calling pthread_key_create() early on
results in infinite recursion. pthread_key_create() calls sysctlbyname()
which is intercepted but COMMON_INTERCEPTOR_NOTHING_IS_INITIALIZED returns
true, so the interceptor calls internal_sysctlbyname() which then ends up
calling the interceptor again. I fixed this issue by using dlsym() to get
the libc version of sysctlbyname() instead.

This fixes https://llvm.org/PR40761

[vitalybuka]

Is any test was broken?

[dim]

In D65221#1600309, @vitalybuka wrote:

Is any test was broken?

As described in https://bugs.llvm.org/show_bug.cgi?id=40761, almost all sanitizer tests crash with DEADLYSIGNAL due to the infinite recursion.

[arichardson]

Remove freebsd XFAIL from two tests that pass with this change

[MaskRay]

compiler-rt/lib/sanitizer_common/sanitizer_linux.cc

780 ↗

(On Diff #211894)

Change it to using and place it inside the function.

788 ↗

(On Diff #211894)

When is dlfunc(RTLD_DEFAULT, "sysctlbyname") used?

[devnexen]

compiler-rt/lib/sanitizer_common/sanitizer_linux.cc

788 ↗

(On Diff #211894)

I wonder the same do you have a situation when it occurs ?

[arichardson]

compiler-rt/lib/sanitizer_common/sanitizer_linux.cc

788 ↗

(On Diff #211894)

Indeed that does seem unlikely (I guess it could theoretically happen statically linking the runtime into libc?).
I copied this code from the sanitizer_netbsd.cc file and assumed there might be a case where the RTLD_DEFAULT case is needed.
But it does seem like more things would break if this is ever needed so I'll remove it.

[arichardson]

• Address review feedback
• Remove XFAIL: freebsd from another test that now passes

[devnexen]

Fine by me but prefers someone else giving the greenlight.

[MaskRay]

compiler-rt/lib/sanitizer_common/sanitizer_linux.cc

780 ↗

(On Diff #211894)

If the signature of internal_sysctlbyname is the same as sysctlbyname, I believe the below will be simpler:

static void *real;
if (!real)
  real = dlsym(RTLD_NEXT, "sysctlbyname");
return (decltype(internal_sysctlbyname)*(real))(sname, oldp, oldlenp, newp, newlen);

I wonder if FreeBSD specific dlfunc is still relevant nowadays...

FreeBSD dlsym(3): "in the C standard, conversions between data and function pointer types are undefined"

Yet, C11 J.5.7 Function pointer casts (Annex J. I guess all working C compilers support this..):

A pointer to an object or to void may be cast to a pointer to a function, allowing data to be invoked as a function (6.5.4).

C++11 [expr.reinterpret.cast]

Converting a function pointer to an object pointer type or vice versa is conditionally-supported.

[arichardson]

compiler-rt/lib/sanitizer_common/sanitizer_linux.cc

780 ↗

(On Diff #211894)

Theoretically, function pointers could be a different size from data pointers (e.g. a pair of pointers for code+got, or even something smaller if you have separate code and data address spaces). But since so much C code assumes that you can store them in a void* I'm not sure any relevant architecture actually uses different sizes.

I quite like dlfunc() because using it explicitly states that you are looking up a function and not a data symbol. FreeBSD rtld doesn't actually differntiate it right now, but I was considering changing it to return NULL if the lookup yields a non-function symbol.

However, using dlfunc() is only fine because this is in a FreeBSD specific block so if other operating systems want to use this function it would need to change. I don't mind changing it to use dlsym() if you think that may be the case in the future.

[arichardson]

compiler-rt/lib/sanitizer_common/sanitizer_linux.cc

780 ↗

(On Diff #211894)

Unfortunately uptr* is not the same as size_t*. I'm not sure why the signature is different but changing the signature would require updating the other implementations, too.

[arichardson]

Use the more portable dlsym() instead of dlfunc()

[dim]

compiler-rt/lib/sanitizer_common/sanitizer_linux.cc

788 ↗

(On Diff #211989)

Note that I got an 80 column -Werror warning about this line. Please move the (size_t) newlen part to the next line.

[arichardson]

git-clang-format

[arichardson]

Remove mention of FreeBSD from comment

[krytarowski]

compiler-rt/lib/sanitizer_common/sanitizer_linux.cc

780 ↗

(On Diff #211894)

size_t is popularly replaced with uptr that in practice is the same underlying type.

[arichardson]

compiler-rt/lib/sanitizer_common/sanitizer_linux.cc

780 ↗

(On Diff #211894)

I believe I got an error without the cast. Maybe for i386 size_t is int and uptr long? Unrelated to this review, the system that I work on (CHERI on FreeBSD) has 64-bit size_t and 128-bit uinptr_t so they are definitely not always compatible.

Also these casts were already required before my patch.

[krytarowski]

compiler-rt/lib/sanitizer_common/sanitizer_linux.cc

780 ↗

(On Diff #211894)

128-bit uinptr_t? 128bit pointers?

If there are 128 bit pointers then there are probably bigger problems than this cast here.

[arichardson]

compiler-rt/lib/sanitizer_common/sanitizer_linux.cc

780 ↗

(On Diff #211894)

Yes exactly (see http://www.chericpu.com for more details). But as I said this is unrelated to this review.

This change is just about making ASAN work again on FreeBSD (x86).
It's been broken for all of 8.0 and it would be very nice to have it working again before 9.0 is shipped.

[MaskRay]

compiler-rt/lib/sanitizer_common/sanitizer_linux.cc

780 ↗

(On Diff #211894)

I think you still miss the example I gave above.

using syctlbyname_ptr should not be needed and should not be placed outside the function (no other functions will use it).

[arichardson]

Used decltype now. Should be a bit shorter than the using declaration.
I still need the casts, otherwise the build fails when building the i368 RTSanitizerCommon.i386.dir/sanitizer_linux.cc.o:

/exports/users/alr48/sources/upstream-llvm-project/compiler-rt/lib/sanitizer_common/sanitizer_linux.cc:786:57: error: cannot initialize a parameter of type 'size_t *' (aka 'unsigned int *') with an lvalue of type '__sanitizer::uptr *' (aka 'unsigned long *')

[arichardson]

Move the cast to dlsym() return value rather than the function pointer call

compiler-rt/trunk/lib/asan/asan_posix.cc

 void AsanOnDeadlySignal(int signo, void *siginfo, void *context) {
   StartReportDeadlySignal();
   SignalContext sig(siginfo, context);
   ReportDeadlySignal(sig);
 }
 
 // ---------------------- TSD ---------------- {{{1
 
-#if SANITIZER_NETBSD || SANITIZER_FREEBSD
-// Thread Static Data cannot be used in early init on NetBSD and FreeBSD.
+#if SANITIZER_NETBSD
+// Thread Static Data cannot be used in early init on NetBSD.
 // Reuse the Asan TSD API for compatibility with existing code
 // with an alternative implementation.
 
 static void (*tsd_destructor)(void *tsd) = nullptr;
 
 struct tsd_key {
   tsd_key() : key(nullptr) {}
   ~tsd_key() {

compiler-rt/trunk/lib/msan/msan_linux.cc

 }
 
 void InstallAtExitHandler() {
   atexit(MsanAtExit);
 }
 
 // ---------------------- TSD ---------------- {{{1
 
-#if SANITIZER_NETBSD || SANITIZER_FREEBSD
-// Thread Static Data cannot be used in early init on NetBSD and FreeBSD.
+#if SANITIZER_NETBSD
+// Thread Static Data cannot be used in early init on NetBSD.
 // Reuse the MSan TSD API for compatibility with existing code
 // with an alternative implementation.
 
 static void (*tsd_destructor)(void *tsd) = nullptr;
 
 struct tsd_key {
   tsd_key() : key(nullptr) {}
   ~tsd_key() {

compiler-rt/trunk/lib/sanitizer_common/sanitizer_linux.cc

#else
   return internal_syscall(SYSCALL(__sysctl), name, namelen, oldp,
                           (size_t *)oldlenp, newp, (size_t)newlen);
 #endif
 }
 
 #if SANITIZER_FREEBSD
 int internal_sysctlbyname(const char *sname, void *oldp, uptr *oldlenp,
                           const void *newp, uptr newlen) {
-  return sysctlbyname(sname, oldp, (size_t *)oldlenp, newp, (size_t)newlen);
+  static decltype(sysctlbyname) *real = nullptr;
+  if (!real)
+    real = (decltype(sysctlbyname) *)dlsym(RTLD_NEXT, "sysctlbyname");
+  CHECK(real);
+  return real(sname, oldp, (size_t *)oldlenp, newp, (size_t)newlen);
 }
 #endif
 #endif
 
 #if SANITIZER_LINUX
 #define SA_RESTORER 0x04000000
 // Doesn't set sa_restorer if the caller did not set it, so use with caution
 //(see below).

compiler-rt/trunk/test/asan/TestCases/Posix/fread_fwrite.cc

 // RUN: %clangxx_asan -g %s -o %t
 // RUN: not %run %t 2>&1 | FileCheck %s --check-prefix=CHECK-FWRITE
 // RUN: not %run %t 1 2>&1 | FileCheck %s --check-prefix=CHECK-FREAD
-//
-// On FreeBSD stack overflow error instead
-// XFAIL: freebsd
 
 #include <stdio.h>
 #include <stdlib.h>
 
 int test_fread() {
   FILE *f = fopen("/dev/zero", "r");
   char buf[2];
   fread(buf, sizeof(buf), 2, f); // BOOM

compiler-rt/trunk/test/asan/TestCases/Posix/tsd_dtor_leak.cc

 // Regression test for a leak in tsd:
 // https://code.google.com/p/address-sanitizer/issues/detail?id=233
 // RUN: %clangxx_asan -O1 %s -pthread -o %t
 // RUN: %env_asan_opts=quarantine_size_mb=0 %run %t
 // XFAIL: x86_64-netbsd
-// Assertion fails
-// XFAIL: x86_64-freebsd
 #include <pthread.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <assert.h>
 #include <sanitizer/allocator_interface.h>
 
 static pthread_key_t tsd_key;
 

compiler-rt/trunk/test/msan/tzset.cc

 // RUN: %clangxx_msan -O0 %s -o %t && %run %t
-// XFAIL: freebsd
 
 #include <stdlib.h>
 #include <string.h>
 #include <time.h>
 
 extern char *tzname[2];
 
 int main(void) {
   if (!strlen(tzname[0]) || !strlen(tzname[1]))
     exit(1);
   tzset();
   if (!strlen(tzname[0]) || !strlen(tzname[1]))
     exit(1);
   return 0;
 }