[libc-commits] [libc] 6f1a9ed - [LIBC] Fix incorrect behavior with pthread_key_t when value was nullptr

[Noah Goldstein via libc-commits]

Author: Noah Goldstein
Date: 2023-04-19T19:54:56-05:00
New Revision: 6f1a9ed072de612584a90adf3469744781da17fc

URL: https://github.com/llvm/llvm-project/commit/6f1a9ed072de612584a90adf3469744781da17fc
DIFF: https://github.com/llvm/llvm-project/commit/6f1a9ed072de612584a90adf3469744781da17fc.diff

LOG: [LIBC] Fix incorrect behavior with pthread_key_t when value was nullptr

We should not call destructor if value is nullptr.

Reviewed By: sivachandra

Differential Revision: https://reviews.llvm.org/D148291

Added: 
    

Modified: 
    libc/src/__support/threads/thread.cpp
    libc/test/integration/src/pthread/pthread_tss_test.cpp

Removed: 
    


################################################################################
diff  --git a/libc/src/__support/threads/thread.cpp b/libc/src/__support/threads/thread.cpp
index b2680ecc0bda0..03e93a42fb04c 100644
--- a/libc/src/__support/threads/thread.cpp
+++ b/libc/src/__support/threads/thread.cpp
@@ -151,7 +151,8 @@ void call_atexit_callbacks(ThreadAttributes *attrib) {
   attrib->atexit_callback_mgr->call();
   for (size_t i = 0; i < TSS_KEY_COUNT; ++i) {
     TSSValueUnit &unit = tss_values[i];
-    if (unit.dtor != nullptr)
+    // Both dtor and value need to nonnull to call dtor
+    if (unit.dtor != nullptr && unit.payload != nullptr)
       unit.dtor(unit.payload);
   }
 }

diff  --git a/libc/test/integration/src/pthread/pthread_tss_test.cpp b/libc/test/integration/src/pthread/pthread_tss_test.cpp
index 5d649026db0e6..b5843d226ff10 100644
--- a/libc/test/integration/src/pthread/pthread_tss_test.cpp
+++ b/libc/test/integration/src/pthread/pthread_tss_test.cpp
@@ -21,16 +21,19 @@ static constexpr int THREAD_DATA_INITVAL = 0x1234;
 static constexpr int THREAD_DATA_FINIVAL = 0x4321;
 static constexpr int THREAD_RUN_VAL = 0x600D;
 
-int child_thread_data = THREAD_DATA_INITVAL;
-int main_thread_data = THREAD_DATA_INITVAL;
+static int child_thread_data = THREAD_DATA_INITVAL;
+static int main_thread_data = THREAD_DATA_INITVAL;
 
-pthread_key_t key;
-void dtor(void *data) {
+static pthread_key_t key;
+static void dtor(void *data) {
   auto *v = reinterpret_cast<int *>(data);
   *v = THREAD_DATA_FINIVAL;
 }
 
-void *func(void *obj) {
+// Used to test that we don't call the destructor when the mapped value in NULL.
+static void dtor_failure(void *) { ASSERT_TRUE(false); }
+
+static void *func(void *obj) {
   ASSERT_EQ(__llvm_libc::pthread_setspecific(key, &child_thread_data), 0);
   int *d = reinterpret_cast<int *>(__llvm_libc::pthread_getspecific(key));
   ASSERT_TRUE(d != nullptr);
@@ -40,7 +43,14 @@ void *func(void *obj) {
   return nullptr;
 }
 
-TEST_MAIN() {
+static void *func_null_val(void *) {
+  // null value, we should not call dtor
+  ASSERT_EQ(__llvm_libc::pthread_setspecific(key, nullptr), 0);
+  ASSERT_EQ(__llvm_libc::pthread_getspecific(key), nullptr);
+  return nullptr;
+}
+
+static void standard_usage_test() {
   ASSERT_EQ(__llvm_libc::pthread_key_create(&key, &dtor), 0);
   ASSERT_EQ(__llvm_libc::pthread_setspecific(key, &main_thread_data), 0);
   int *d = reinterpret_cast<int *>(__llvm_libc::pthread_getspecific(key));
@@ -56,7 +66,20 @@ TEST_MAIN() {
   ASSERT_EQ(retval, nullptr);
   ASSERT_EQ(arg, THREAD_RUN_VAL);
   ASSERT_EQ(child_thread_data, THREAD_DATA_FINIVAL);
+  ASSERT_EQ(__llvm_libc::pthread_key_delete(key), 0);
+}
 
+static void null_value_test() {
+  pthread_t th;
+  ASSERT_EQ(__llvm_libc::pthread_key_create(&key, &dtor_failure), 0);
+  ASSERT_EQ(__llvm_libc::pthread_create(&th, nullptr, &func_null_val, nullptr),
+            0);
+  ASSERT_EQ(__llvm_libc::pthread_join(th, nullptr), 0);
   ASSERT_EQ(__llvm_libc::pthread_key_delete(key), 0);
+}
+
+TEST_MAIN() {
+  standard_usage_test();
+  null_value_test();
   return 0;
 }