Page MenuHomePhabricator

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

Authored by delcypher on Dec 14 2018, 10:05 AM.

Details

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

Diff Detail

Repository
rL LLVM

Event Timeline

delcypher created this revision.Dec 14 2018, 10:05 AM
Herald added a subscriber: Restricted Project. · View Herald TranscriptDec 14 2018, 10:05 AM

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

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/

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.

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.

  • Also intercept sysctl
  • Don't use UNLIKELY(...)
  • Use internal_sysctl* rather than REAL(sysctl*)
delcypher marked 2 inline comments as done.Dec 16 2018, 11:14 AM
delcypher added inline comments.Dec 16 2018, 11:16 AM
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 added inline comments.Dec 16 2018, 1:48 PM
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 added inline comments.Dec 16 2018, 1:58 PM
lib/sanitizer_common/sanitizer_common_interceptors.inc
7542 ↗(On Diff #178403)

Drop brackets.

7567 ↗(On Diff #178403)

Drop brackets.

delcypher marked an inline comment as done.Dec 16 2018, 2:43 PM
delcypher added inline comments.
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 added inline comments.Dec 16 2018, 11:04 PM
lib/sanitizer_common/sanitizer_common_interceptors.inc
7542 ↗(On Diff #178403)

Right {} curly ones.

Drop curly braces for single line true branch statement.

delcypher marked 2 inline comments as done.Dec 17 2018, 10:20 AM

@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 marked an inline comment as done.Dec 17 2018, 10:33 AM
krytarowski accepted this revision.Dec 17 2018, 11:02 AM
This revision is now accepted and ready to land.Dec 17 2018, 11:02 AM
vitalybuka accepted this revision.Dec 17 2018, 1:39 PM
This revision was automatically updated to reflect the committed changes.

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