Don't trigger sanitizer initialization from `sysctlbyname` interceptor.

[delcypher]

Summary

This fixes the ThreadSanitizer-x86_64-iossim testsuite which broke
when r348770 (https://reviews.llvm.org/D55473) landed.

The root cause of the problem is that early-on during the iOS simulator
init process a call to sysctlbyname is issued. If the TSan initializer
is triggered at this point it will eventually trigger a call to
__cxa_at_exit(...). This call then aborts because the library
implementing this function is not yet had its initialization function
called.

rdar://problem/46696934

[vitalybuka]

If we are going to support this use-case we need a test.

I think you can clone/update sysctl.cc, and define all possible __*san_default_options as following:

__tsan_default_options() {
  test_sysctl();
  test_sysctlbyname();
  test_sysctlnametomib();
}

This is probably early enough.
Please check that the new test fail without your patch.

lib/sanitizer_common/sanitizer_common_interceptors.inc

7542 ↗

(On Diff #178245)

Could you please update all 3

7564 ↗

(On Diff #178245)

Please remove UNLIKELY for consistency with the rest

7571 ↗

(On Diff #178245)

I guess this will work only on Darwin
Ptr to REAL() is not yet initialized
It must be internal_sysctlbyname

[kubamracek]

If we are going to support this use-case we need a test.

We have iOS simulator tests are they're all currently failing because of this: http://green.lab.llvm.org/green/job/clang-stage1-cmake-RA-expensive/13113/testReport/

[krytarowski]

In D55711#1331469, @vitalybuka wrote:

If we are going to support this use-case we need a test.

I think you can clone/update sysctl.cc, and define all possible __*san_default_options as following:

__tsan_default_options() {
  test_sysctl();
  test_sysctlbyname();
  test_sysctlnametomib();
}

This is probably early enough.
Please check that the new test fail without your patch.

It's a good idea for a test, once it will be shared I will cross-check this for NetBSD.

[delcypher]

In D55711#1331469, @vitalybuka wrote:

If we are going to support this use-case we need a test.

I think you can clone/update sysctl.cc, and define all possible __*san_default_options as following:

__tsan_default_options() {
  test_sysctl();
  test_sysctlbyname();
  test_sysctlnametomib();
}

This is probably early enough.
Please check that the new test fail without your patch.

@vitalybuka

As @kubamracek pointed we already have a failing test. However I tried getting your suggested testing approach and unfortunately it doesn't work.

1. Defining __tsan_default_options() as you suggest does trigger a crash, however this was due to __tsan_default_options() and the functions it calls being instrumented by TSan. In particular the instrumentation adds calls to __tsan_func_entry() which triggers a crash if we're already doing TSan init. Changing the implementation of the sysctl interceptors doesn't fix this because we crash even before we get to those.
2. I rewrote the test to build __tsan_default_options() (and the functions it calls) without any instrumentation. However now the test succeeds, i.e. we can call the current implementation of sysctl from inside __tsan_default_options and there's no crash.

Basically triggering a call to sysctl inside __tsan_default_options is the wrong thing to do. This is because if __tsan_default_options is being called it means TSan's init has already been triggered and it's already too late. To reproduce this bug (not in a iOS simulator test) we'd have to trigger the call to sysctl much earlier and I don't know how to do this. I think we're better of just relying on the iOS simulator test and just making sure they are run on Darwin by default if the simulator is available.

[delcypher]

• Also intercept sysctl
• Don't use UNLIKELY(...)
• Use internal_sysctl* rather than REAL(sysctl*)

[delcypher]

lib/sanitizer_common/sanitizer_common_interceptors.inc

7542 ↗

(On Diff #178245)

@vitalybuka How can we do sysctlnametomib? There's no internal_sysctlnametomib for me to use.

[krytarowski]

lib/sanitizer_common/sanitizer_common_interceptors.inc

7542 ↗

(On Diff #178245)

For simplicity I would skip it now unless there will be a real problem detected.

If we need it, it can be likely reimplemented with internal_sysctl()... but the reimplementation would likely be complex.

[krytarowski]

lib/sanitizer_common/sanitizer_common_interceptors.inc

7542 ↗

(On Diff #178403)

Drop brackets.

7567 ↗

(On Diff #178403)

Drop brackets.

[delcypher]

lib/sanitizer_common/sanitizer_common_interceptors.inc

7542 ↗

(On Diff #178403)

Do you mean the curly braces ({ ...})? Dropping the brackets around COMMON_INTERCEPTOR_NOTHING_IS_INITIALIZED doesn't make much sense.

[krytarowski]

lib/sanitizer_common/sanitizer_common_interceptors.inc

7542 ↗

(On Diff #178403)

Right {} curly ones.

[delcypher]

Drop curly braces for single line true branch statement.

[delcypher]

@vitalybuka @krytarowski Can we please land this today? Our bots that the iOS simulator tests are broken because of https://reviews.llvm.org/D55473 . We're going to have to revert that patch if this doesn't land soon.

[delcypher]

@vitalybuka @krytarowski Thanks for approving. I'm landing this now.

compiler-rt/trunk/lib/sanitizer_common/sanitizer_common_interceptors.inc

 #else
 #define INIT_FTS
 #endif
 
 #if SANITIZER_INTERCEPT_SYSCTL
 INTERCEPTOR(int, sysctl, int *name, unsigned int namelen, void *oldp,
             SIZE_T *oldlenp, void *newp, SIZE_T newlen) {
   void *ctx;
+  if (COMMON_INTERCEPTOR_NOTHING_IS_INITIALIZED)
+    return internal_sysctl(name, namelen, oldp, oldlenp, newp, newlen);
   COMMON_INTERCEPTOR_ENTER(ctx, sysctl, name, namelen, oldp, oldlenp, newp,
                            newlen);
   if (name)
     COMMON_INTERCEPTOR_READ_RANGE(ctx, name, namelen * sizeof(*name));
   if (oldlenp)
     COMMON_INTERCEPTOR_READ_RANGE(ctx, oldlenp, sizeof(*oldlenp));
   if (newp && newlen)
     COMMON_INTERCEPTOR_READ_RANGE(ctx, newp, newlen);
   int res = REAL(sysctl)(name, namelen, oldp, oldlenp, newp, newlen);
   if (!res) {
     if (oldlenp) {
       COMMON_INTERCEPTOR_WRITE_RANGE(ctx, oldlenp, sizeof(*oldlenp));
       if (oldp)
         COMMON_INTERCEPTOR_WRITE_RANGE(ctx, oldp, *oldlenp);
     }
   }
   return res;
 }
 
 INTERCEPTOR(int, sysctlbyname, char *sname, void *oldp, SIZE_T *oldlenp,
             void *newp, SIZE_T newlen) {
   void *ctx;
+  if (COMMON_INTERCEPTOR_NOTHING_IS_INITIALIZED)
+    return internal_sysctlbyname(sname, oldp, oldlenp, newp, newlen);
   COMMON_INTERCEPTOR_ENTER(ctx, sysctlbyname, sname, oldp, oldlenp, newp,
                            newlen);
   if (sname)
     COMMON_INTERCEPTOR_READ_RANGE(ctx, sname, REAL(strlen)(sname) + 1);
   if (oldlenp)
     COMMON_INTERCEPTOR_READ_RANGE(ctx, oldlenp, sizeof(*oldlenp));
   if (newp && newlen)
     COMMON_INTERCEPTOR_READ_RANGE(ctx, newp, newlen);