Page MenuHomePhabricator

[TSan][Darwin] Avoid crashes due to interpreting non-zero shadow content as a pointer
AbandonedPublic

Authored by yln on Sep 21 2021, 6:00 AM.

Details

Summary

We would like to use TLS to store the ThreadState object (or at least a
reference ot it), but on Darwin accessing TLS via __thread or manually
by using pthread_key_* is problematic, because there are several places
where interceptors are called when TLS is not accessible (early process
startup, thread cleanup, ...).

Previously, we used a "poor man's TLS" implementation, where we use the
shadow memory of the pointer returned by pthread_self() to store a
pointer to the ThreadState object.

The problem with that was that certain operations can populate shadow
bytes unbeknownst to TSan, and we later interpret these non-zero bytes
as the pointer to our ThreadState object and crash on when dereferencing
the pointer.

This patch changes how we store the reference to the ThreadState object.
Instead, of simulating TLS via the shadow memory, we use a global,
thread-safe hash map to store a pointer to our ThreadState objects and
use mmap() to allocate the backing memory. The main thread's
ThreadState is stored separately in a static variable, because we need
to access it even before we can allocate and initialize the hash map.

Radar-Id: rdar://problem/72010355

Diff Detail

Unit TestsFailed

TimeTest
50 msx64 debian > ORC-x86_64-linux.TestCases/Linux/x86-64::trivial-cxa-atexit.S
Script: -- : 'RUN: at line 3'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang -m64 -c -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/orc/X86_64LinuxConfig/TestCases/Linux/x86-64/Output/trivial-cxa-atexit.S.tmp /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/orc/TestCases/Linux/x86-64/trivial-cxa-atexit.S
60 msx64 debian > ORC-x86_64-linux.TestCases/Linux/x86-64::trivial-static-initializer.S
Script: -- : 'RUN: at line 7'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang -m64 -c -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/orc/X86_64LinuxConfig/TestCases/Linux/x86-64/Output/trivial-static-initializer.S.tmp /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/orc/TestCases/Linux/x86-64/trivial-static-initializer.S
50 msx64 debian > ORC-x86_64-linux.TestCases/Linux/x86-64::trivial-tls.S
Script: -- : 'RUN: at line 1'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang -m64 -c -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/orc/X86_64LinuxConfig/TestCases/Linux/x86-64/Output/trivial-tls.S.tmp /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/orc/TestCases/Linux/x86-64/trivial-tls.S

Event Timeline

yln requested review of this revision.Sep 21 2021, 6:00 AM
yln created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptSep 21 2021, 6:00 AM
Herald added a subscriber: Restricted Project. · View Herald Transcript

Hi Julian,

I assume the garbage in shadow you are referring to is written to the shadow before the thread is created, is it correct? If yes, couldn't we reset the shadow to 0 on thread creation? We intercept all thread creations anyway.
I am quite worried performance overhead. cur_thread is called for every memory access.

delcypher added inline comments.Sep 21 2021, 4:47 PM
compiler-rt/lib/tsan/rtl/tsan_platform_mac.cpp
69

Is this how TSan internals do allocation? I would have expected a call to the internal allocator.

96

Is a write to the value stored in the map safe to do? The comments on the data structure make it sound like its only safe to write to the stored value when h.created() is true.

// {
//   Map::Handle h(&m, addr);
//   use h.operator->() to access the data
//   if h.created() then the element was just created, and the current thread
//     has exclusive access to it
//   otherwise the current thread has only read access to the data
// }
292

The code in the else block is a little confusing. It looks like it's assuming the address returned by pthread_self is going to be somewhere in the TLS and so it tries to avoid updating the part of the shadow that we're using to store the ThreadState pointer. Presumably it can be removed in this patch because we're not storing the ThreadState pointer in the shadow?

yln added a comment.EditedWed, Sep 29, 10:14 AM

Hi Julian,

I assume the garbage in shadow you are referring to is written to the shadow before the thread is created, is it correct? If yes, couldn't we reset the shadow to 0 on thread creation? We intercept all thread creations anyway.
I am quite worried performance overhead. cur_thread is called for every memory access.

Hi Dimitry,

I agree that this would be the best solution, i.e., it would solve the root cause (corrupted shadow memory bytes) and not just the symptom (crash).

Unfortunately, I can't figure out why this approach doesn't work in our customer's setup:
https://reviews.llvm.org/D109184 (not sufficient)

I've confirmed that storing the reference in "true" TLS (via a trick) works. Would you be happy with this approach?
https://reviews.llvm.org/D110236 (confirmed fix)

Thanks,
Julian

yln abandoned this revision.Wed, Sep 29, 10:16 AM

Abandoning in favor of D110236

Hi Julian,

I assume the garbage in shadow you are referring to is written to the shadow before the thread is created, is it correct? If yes, couldn't we reset the shadow to 0 on thread creation? We intercept all thread creations anyway.
I am quite worried performance overhead. cur_thread is called for every memory access.

Hi Dimitry,

I agree that this would be the best solution, i.e., it would solve the root cause (corrupted shadow memory bytes) and not just the symptom (crash).

Unfortunately, I can't figure out why this approach doesn't work in our customer's setup:
https://reviews.llvm.org/D109184 (not sufficient)

MemoryRangeImitateWriteOrResetRange writes non-0's to shadow. Maybe MemoryResetRange will help?

yln added a comment.Thu, Sep 30, 8:49 AM

MemoryRangeImitateWriteOrResetRange writes non-0's to shadow. Maybe MemoryResetRange will help?

To give a bit more context:
Calling any of the "proper" shadow memory functions already requires a ThreadState thr, which we don't have in the cases where this matters: we want to initialize the pointer with 0 to make sure that later on other code recognizes that it needs to be created in the first place!

There are 2 more issues:

  • MemoryRangeReset() doesn't necessarily force 0 in the shadow bytes, but may only marks the region as "deleted" (like deleting a file in a filesystem)
  • Even when it resets bytes, it doesn't necessarily reset all bytes in a large region (just the first and last few pages)

To sidestep these issues I put a blunt internal_memset(shadow_addr, 0, shadow_size) in all cases (even the ones that show early returns, e.g., because thr isn't initialized, in the current patch) in mach_vm_map and mach_vm_allocator interceptor just to see if it would resolve the issue (and then work on a refined patch for the approach), but our customer still reported the same crashes.