Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OSX: use after free in thread_local when a TLS location isn't initialized until destruction of another TLS location #57534

Closed
mtak- opened this issue Jan 11, 2019 · 7 comments · Fixed by #57655
Labels
A-thread-locals Area: Thread local storage (TLS) I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness O-macos Operating system: macOS P-high High priority T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@mtak-
Copy link
Contributor

mtak- commented Jan 11, 2019

Moved from Amanieu/parking_lot#114 because the issue is not specific to parking_lot.

Repro:

  • toolchain: rustc 1.33.0-nightly (c2d381d 2019-01-10)
  • OSX 10.14
  • RUSTFLAGS="-Z sanitizer=address" cargo run
use std::thread;

struct A;

impl Drop for A {
    fn drop(&mut self) {
        THREAD_STRING.with(|s| {
            println!("{:?}", s);
        })
    }
}

thread_local! {
    static THREAD_STRING: String = String::from("hello there");
}

thread_local! {
    static THREAD_A: A = A;
}

fn main() {
    thread::spawn(|| {
        THREAD_A.with(|_| {});
    })
    .join()
    .unwrap();
}

If thread is changed to access THREAD_STRING before exiting, then the ASAN error goes away.

Address Sanitizer Log

==39852==ERROR: AddressSanitizer: heap-use-after-free on address 0x610000008089 at pc 0x00010c736a3f bp 0x70000fd46790 sp 0x70000fd45f40
READ of size 1 at 0x610000008089 thread T1
    #0 0x10c736a3e in __asan_memcpy (lib__rustc__clang_rt.asan_osx_dynamic.dylib:x86_64+0x56a3e)
    #1 0x10c66c6a6 in core::ptr::read::h3b49a10a9618a90a ptr.rs:575
    #2 0x10c66c0b2 in core::ptr::swap_nonoverlapping_one::he9532edd02cf97c7 ptr.rs:359
    #3 0x10c670d4c in core::mem::swap::hdf87706b2f446181 mem.rs:653
    #4 0x10c6716ed in core::mem::replace::h9ad7bc2c5ad8c60d mem.rs:711
    #5 0x10c665af2 in _$LT$core..cell..Cell$LT$T$GT$$GT$::replace::h5e8fd793dd71bad7 cell.rs:432
    #6 0x10c6658f4 in _$LT$core..cell..Cell$LT$T$GT$$GT$::set::hbc93a146608ff6a2 cell.rs:389
    #7 0x10c6596a6 in std::thread::local::fast::destroy_value::h7198d56987ed7071 local.rs:397
    #8 0x7fff7cc45d7e in tlv_finalize (libdyld.dylib:x86_64+0x16d7e)
    #9 0x7fff7ce386ae in _pthread_tsd_cleanup (libsystem_pthread.dylib:x86_64+0x36ae)
    #10 0x7fff7ce3b6b0 in _pthread_exit (libsystem_pthread.dylib:x86_64+0x66b0)
    #11 0x7fff7ce38347 in _pthread_body (libsystem_pthread.dylib:x86_64+0x3347)
    #12 0x7fff7ce3b2a6 in _pthread_start (libsystem_pthread.dylib:x86_64+0x62a6)
    #13 0x7fff7ce37424 in thread_start (libsystem_pthread.dylib:x86_64+0x2424)

0x610000008089 is located 73 bytes inside of 192-byte region [0x610000008040,0x610000008100)
freed by thread T1 here:
    #0 0x10c73967d in wrap_free (lib__rustc__clang_rt.asan_osx_dynamic.dylib:x86_64+0x5967d)
    #1 0x7fff7ce386ae in _pthread_tsd_cleanup (libsystem_pthread.dylib:x86_64+0x36ae)
    #2 0x7fff7ce3b6b0 in _pthread_exit (libsystem_pthread.dylib:x86_64+0x66b0)
    #3 0x7fff7ce38347 in _pthread_body (libsystem_pthread.dylib:x86_64+0x3347)
    #4 0x7fff7ce3b2a6 in _pthread_start (libsystem_pthread.dylib:x86_64+0x62a6)
    #5 0x7fff7ce37424 in thread_start (libsystem_pthread.dylib:x86_64+0x2424)

previously allocated by thread T1 here:
    #0 0x10c7394c3 in wrap_malloc (lib__rustc__clang_rt.asan_osx_dynamic.dylib:x86_64+0x594c3)
    #1 0x7fff7cc32611 in tlv_allocate_and_initialize_for_key (libdyld.dylib:x86_64+0x3611)
    #2 0x7fff7cc32440 in tlv_get_addr (libdyld.dylib:x86_64+0x3440)
    #3 0x10c67aa3d in std::sys_common::thread_info::set::h515643561df38c4d local.rs:296
    #4 0x10c64c6e9 in std::thread::Builder::spawn_unchecked::_$u7b$$u7b$closure$u7d$$u7d$::he15d93b7c71cc10d mod.rs:466
    #5 0x10c64d3c0 in _$LT$F$u20$as$u20$alloc..boxed..FnBox$LT$A$GT$$GT$::call_box::h77b2d26d5e0b55f2 boxed.rs:734
    #6 0x10c67df6b in std::sys::unix::thread::Thread::new::thread_start::h0b558311a1802cc0 thread.rs:14
    #7 0x7fff7ce3833c in _pthread_body (libsystem_pthread.dylib:x86_64+0x333c)
    #8 0x7fff7ce3b2a6 in _pthread_start (libsystem_pthread.dylib:x86_64+0x62a6)
    #9 0x7fff7ce37424 in thread_start (libsystem_pthread.dylib:x86_64+0x2424)

Thread T1 created by T0 here:
    #0 0x10c73143d in wrap_pthread_create (lib__rustc__clang_rt.asan_osx_dynamic.dylib:x86_64+0x5143d)
    #1 0x10c67dc3c in std::sys::unix::thread::Thread::new::hcbffd486872edde6 thread.rs:69
    #2 0x10c64b494 in std::thread::Builder::spawn_unchecked::h3923920731000341 mod.rs:488
    #3 0x10c64cd0f in std::thread::Builder::spawn::h7e37ea2a6d1dc69b mod.rs:382
    #4 0x10c64a58c in std::thread::spawn::h77287ca628b2c83b mod.rs:610
    #5 0x10c65458c in parking_lot_test::main::h36e62a83a5d578be main.rs:22
    #6 0x10c64db4d in std::rt::lang_start::_$u7b$$u7b$closure$u7d$$u7d$::h38b6bb7d7da5478f rt.rs:64
    #7 0x10c67be17 in std::panicking::try::do_call::hbeca4642433c0ede panicking.rs:297
    #8 0x10c67e55e in __rust_maybe_catch_panic lib.rs:92
    #9 0x10c67c7ec in std::rt::lang_start_internal::h8e4e8fac3cfbbe81 rt.rs:48
    #10 0x10c64dabe in std::rt::lang_start::h9a9dc7a97c4ea753 rt.rs:64
    #11 0x10c6546c1 in main (parking_lot_test:x86_64+0x10000e6c1)
    #12 0x7fff7cc46084 in start (libdyld.dylib:x86_64+0x17084)

SUMMARY: AddressSanitizer: heap-use-after-free (lib__rustc__clang_rt.asan_osx_dynamic.dylib:x86_64+0x56a3e) in __asan_memcpy
Shadow bytes around the buggy address:
  0x1c2000000fc0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x1c2000000fd0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x1c2000000fe0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x1c2000000ff0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x1c2000001000: fa fa fa fa fa fa fa fa fd fd fd fd fd fd fd fd
=>0x1c2000001010: fd[fd]fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x1c2000001020: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x1c2000001030: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x1c2000001040: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x1c2000001050: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x1c2000001060: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
  Shadow gap:              cc
==39852==ABORTING
Illegal instruction: 4

@mtak- mtak- changed the title OSX: use after free in thread_local when a TLS location isn't get initialized until destruction of another TLS location OSX: use after free in thread_local when a TLS location isn't initialized until destruction of another TLS location Jan 11, 2019
@jonas-schievink
Copy link
Contributor

Might be related to / caused by #28129

@mtak-
Copy link
Contributor Author

mtak- commented Jan 12, 2019

Looking at the source code for tlv_finalize and _tlv_atexit, it sure looks like UB to call _tlv_atexit during tlv_finalize which lldb shows is happening in this example.

This is worse than just not running tls destructors for the main thread. It is unsound AFAICT.

@Centril Centril added O-macos Operating system: macOS I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness A-thread-locals Area: Thread local storage (TLS) P-high High priority T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jan 13, 2019
@alexcrichton
Copy link
Member

Thanks for debugging this @mtak-! I wonder if we can fix this on our end? We might be able to prevent new TLS slots from being initialized after a thread has exited perhaps?

@mtak-
Copy link
Contributor Author

mtak- commented Jan 15, 2019

I don't know how to reliably detect that a thread is exiting using the APIs that OSX provides (there could be a way). The logic in Apple's threadLocalVariables.c looks similar to what already exists in register_dtor_fallback, except that register_dtor_fallback handles destructor registering during thread exit properly AFAICT. Perhaps we could call into that here.

FWIW C++ has similar problems with "static local" thread locals when compiled by appleclang.

#include <iostream>

struct A {
    int x;
    ~A();
};

thread_local A thread_a{3557};

A::~A() {
    static thread_local std::string thread_string{"hello there long enough string"};

    // next line triggers a call to _tlv_atexit during tlv_finalize, and makes ASAN unhappy
    std::cout << thread_string << std::endl;
}

int main() {
    std::cout << thread_a.x << std::endl;
    return 0;
}

@alexcrichton
Copy link
Member

Makes sense yeah, perhaps as a fallback solution we could just fix this issue for Rust-spawned threads? For those we know exactly when they're exiting (or at least when user-written Rust code has ceased executing) and that may be enough too! Skipping _tlv_atexit may also be reasonable and we already do that on a number of other platforms too

mtak- added a commit to mtak-/rust that referenced this issue Jan 16, 2019
Centril added a commit to Centril/rust that referenced this issue Jan 17, 2019
…ichton

OSX: fix rust-lang#57534 registering thread dtors while running thread dtors

r? @alexcrichton

- "fast" `thread_local` destructors get run even on the main thread
- "fast" `thread_local` dtors, can initialize other `thread_local`'s

One corner case where this fix doesn't work, is when a C++ `thread_local` triggers the initialization of a rust `thread_local`.

I did not add any std::thread specific flag to indicate that the thread is currently exiting, which would be checked before registering a new dtor (I didn't really know where to stick that). I think this does the trick tho!

Let me know if anything needs tweaking/fixing/etc.

resolves this for macos: rust-lang#28129
fixes: rust-lang#57534
Centril added a commit to Centril/rust that referenced this issue Jan 17, 2019
…ichton

OSX: fix rust-lang#57534 registering thread dtors while running thread dtors

r? @alexcrichton

- "fast" `thread_local` destructors get run even on the main thread
- "fast" `thread_local` dtors, can initialize other `thread_local`'s

One corner case where this fix doesn't work, is when a C++ `thread_local` triggers the initialization of a rust `thread_local`.

I did not add any std::thread specific flag to indicate that the thread is currently exiting, which would be checked before registering a new dtor (I didn't really know where to stick that). I think this does the trick tho!

Let me know if anything needs tweaking/fixing/etc.

resolves this for macos: rust-lang#28129
fixes: rust-lang#57534
@pnkfelix pnkfelix added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jan 17, 2019
@pnkfelix
Copy link
Member

visiting for T-compiler triage. Retagged as T-libs.

bors added a commit that referenced this issue Jan 20, 2019
OSX: fix #57534 registering thread dtors while running thread dtors

r? @alexcrichton

- "fast" `thread_local` destructors get run even on the main thread
- "fast" `thread_local` dtors, can initialize other `thread_local`'s

One corner case where this fix doesn't work, is when a C++ `thread_local` triggers the initialization of a rust `thread_local`.

I did not add any std::thread specific flag to indicate that the thread is currently exiting, which would be checked before registering a new dtor (I didn't really know where to stick that). I think this does the trick tho!

Let me know if anything needs tweaking/fixing/etc.

resolves this for macos: #28129
fixes: #57534
@mtak-
Copy link
Contributor Author

mtak- commented Jan 20, 2019

Filed an apple bug report. openradar link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-thread-locals Area: Thread local storage (TLS) I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness O-macos Operating system: macOS P-high High priority T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
5 participants