[libc++] Avoid lifetime UB in __thread_local_data()

[vitalybuka]

Summary

Detected on many lld tests with -fsanitize-memory-use-after-dtor.
Also https://lab.llvm.org/buildbot/#/builders/sanitizer-x86_64-linux-fast after D122869 will report a lot of them.

Threads may outlive static variables. Even if ~__thread_specific_ptr() does nothing, lifetime of members ends with ~ and accessing the value is UB https://eel.is/c++draft/basic.life#1

==9214==WARNING: MemorySanitizer: use-of-uninitialized-value
    #0 0x557e1cec4539 in __libcpp_tls_set ../include/c++/v1/__threading_support:428:12
    #1 0x557e1cec4539 in set_pointer ../include/c++/v1/thread:196:5
    #2 0x557e1cec4539 in void* std::__msan::__thread_proxy<
      std::__msan::tuple<...>, llvm::parallel::detail::(anonymous namespace)::ThreadPoolExecutor::ThreadPoolExecutor(llvm::ThreadPoolStrategy)::'lambda'()::operator()() const::'lambda'()> >(void*) ../include/c++/v1/thread:285:27

  Memory was marked as uninitialized
    #0 0x557e10a0759d in __sanitizer_dtor_callback compiler-rt/lib/msan/msan_interceptors.cpp:940:5
    #1 0x557e1d8c478d in std::__msan::__thread_specific_ptr<std::__msan::__thread_struct>::~__thread_specific_ptr() libcxx/include/thread:188:1
    #2 0x557e10a07dc0 in MSanCxaAtExitWrapper(void*) compiler-rt/lib/msan/msan_interceptors.cpp:1151:3

The test needs D123979 or -fsanitize-memory-param-retval enabled by default.

[vitalybuka]

ping

[ldionne]

Thanks for the patch! I'm not sure I understand how we'd catch this though, since D122869 doesn't touch any libc++ tests. Is there something we can do to fix our test setup in libc++ so as to catch this issue in our own tests?

libcxx/src/thread.cpp

118–125

I think we should add a comment like:

// Even though __thread_specific_ptr's destructor doesn't actually destroy anything
// (see comments there), we can't call it at all because threads may outlive the static
// variable and calling its destructor means accessing an object outside of its lifetime,
// which is UB.

[vitalybuka]

comment

[vitalybuka]

In D122864#3442952, @ldionne wrote:

Thanks for the patch! I'm not sure I understand how we'd catch this though, since D122869 doesn't touch any libc++ tests. Is there something we can do to fix our test setup in libc++ so as to catch this issue in our own tests?

I am not sure as well. Do we have bot which runs libc++ tests with sanitizers?
My team runs some clang/llvm tests with msan and instrumented libc++, but no libc++ tests.

WDYT about:

__thread_specific_ptr::~__thread_specific_ptr() {
  destroyed = true;
}

__thread_specific_ptr::get() {
  _LIBCPP_ASSERT(!destroyed, ...
}

and then I can try to add test to simulated this behavior?

[ldionne]

In D122864#3443536, @vitalybuka wrote:

In D122864#3442952, @ldionne wrote:

Thanks for the patch! I'm not sure I understand how we'd catch this though, since D122869 doesn't touch any libc++ tests. Is there something we can do to fix our test setup in libc++ so as to catch this issue in our own tests?

I am not sure as well. Do we have bot which runs libc++ tests with sanitizers?
My team runs some clang/llvm tests with msan and instrumented libc++, but no libc++ tests.

Yes, we run Asan, Tsan, Msan and UBsan on each patch in our pre-commit ci: https://buildkite.com/llvm-project/libcxx-ci/builds/10156. It looks like the patch as-is is failing under Msan, too.

To detect this, perhaps we're just missing -fsanitize-memory-use-after-dtor in our test setup? Is it enabled by default?

[vitalybuka]

In D122864#3449022, @ldionne wrote:

In D122864#3443536, @vitalybuka wrote:

In D122864#3442952, @ldionne wrote:

Thanks for the patch! I'm not sure I understand how we'd catch this though, since D122869 doesn't touch any libc++ tests. Is there something we can do to fix our test setup in libc++ so as to catch this issue in our own tests?

I am not sure as well. Do we have bot which runs libc++ tests with sanitizers?
My team runs some clang/llvm tests with msan and instrumented libc++, but no libc++ tests.

Yes, we run Asan, Tsan, Msan and UBsan on each patch in our pre-commit ci: https://buildkite.com/llvm-project/libcxx-ci/builds/10156. It looks like the patch as-is is failing under Msan, too.

To detect this, perhaps we're just missing -fsanitize-memory-use-after-dtor in our test setup? Is it enabled by default?

It's not, also it needs env MSAN_OPTIONS=poison_in_dtor=1
However those are good to be enabled by default, but it will break here and some other llvm places I need to fix.

[vitalybuka]

rebase

[vitalybuka]

add test

[vitalybuka]

rebase

[vitalybuka]

remove include

[vitalybuka]

ping

[vitalybuka]

ping

libcxx/src/thread.cpp

sleep_for(const chrono::nanoseconds& ns)
     }
 }
 
 }  // this_thread
 
 __thread_specific_ptr<__thread_struct>&
 __thread_local_data()
 {
-    static __thread_specific_ptr<__thread_struct> __p;
-    return __p;
+  // Even though __thread_specific_ptr's destructor doesn't actually destroy
+  // anything (see comments there), we can't call it at all because threads may
+  // outlive the static variable and calling its destructor means accessing an
+  // object outside of its lifetime, which is UB.
+  alignas(__thread_specific_ptr<__thread_struct>) static char __b[sizeof(__thread_specific_ptr<__thread_struct>)];
+  static __thread_specific_ptr<__thread_struct>* __p = new (__b) __thread_specific_ptr<__thread_struct>();
+  return *__p;
 }
 
 // __thread_struct_imp
 
 template <class T>
 class _LIBCPP_HIDDEN __hidden_allocator
 {
 public:
     typedef T  value_type;

libcxx/test/libcxx/thread/thread.threads/create_late.pass.cpp

+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// UNSUPPORTED: libcpp-has-no-threads
+// UNSUPPORTED: c++03
+
+#include "make_test_thread.h"
+
+void func() {}
+
+struct T {
+  ~T() {
+    // __thread_local_data is expected to be destroyed as it was created
+    // from the main(). Now trigger another access.
+    support::make_test_thread(func).join();
+  }
+} t;
+
+int main(int, char**) {
+  // Triggers construction of __thread_local_data.
+  support::make_test_thread(func).join();
+
+  return 0;
+}