From 5ddbb3f936ee44555a46020239e49ab45109a806 Mon Sep 17 00:00:00 2001
From: Yabin Cui <yabinc@google.com>
Date: Thu, 5 Mar 2015 20:35:32 -0800
Subject: [PATCH] Prevent using static-allocated pthread keys before creation.

Bug: 19993460

Change-Id: I244dea7f5df3c8384f88aa48d635348fafc9cbaf
---
 libc/bionic/pthread_key.cpp | 24 ++++++++++++++++++------
 tests/pthread_test.cpp      | 13 +++++++++++++
 2 files changed, 31 insertions(+), 6 deletions(-)

diff --git a/libc/bionic/pthread_key.cpp b/libc/bionic/pthread_key.cpp
index 65e087902..6d77afac6 100644
--- a/libc/bionic/pthread_key.cpp
+++ b/libc/bionic/pthread_key.cpp
@@ -57,8 +57,15 @@ static inline bool SeqOfKeyInUse(uintptr_t seq) {
   return seq & (1 << SEQ_KEY_IN_USE_BIT);
 }
 
+#define KEY_VALID_FLAG (1 << 31)
+
+static_assert(sizeof(pthread_key_t) == sizeof(int) && static_cast<pthread_key_t>(-1) < 0,
+              "pthread_key_t should be typedef to int");
+
 static inline bool KeyInValidRange(pthread_key_t key) {
-  return key >= 0 && key < BIONIC_PTHREAD_KEY_COUNT;
+  // key < 0 means bit 31 is set.
+  // Then key < (2^31 | BIONIC_PTHREAD_KEY_COUNT) means the index part of key < BIONIC_PTHREAD_KEY_COUNT.
+  return (key < (KEY_VALID_FLAG | BIONIC_PTHREAD_KEY_COUNT));
 }
 
 // Called from pthread_exit() to remove all pthread keys. This must call the destructor of
@@ -114,7 +121,7 @@ int pthread_key_create(pthread_key_t* key, void (*key_destructor)(void*)) {
     while (!SeqOfKeyInUse(seq)) {
       if (atomic_compare_exchange_weak(&key_map[i].seq, &seq, seq + SEQ_INCREMENT_STEP)) {
         atomic_store(&key_map[i].key_destructor, reinterpret_cast<uintptr_t>(key_destructor));
-        *key = i;
+        *key = i | KEY_VALID_FLAG;
         return 0;
       }
     }
@@ -127,9 +134,10 @@ int pthread_key_create(pthread_key_t* key, void (*key_destructor)(void*)) {
 // responsibility of the caller to properly dispose of the corresponding data
 // and resources, using any means it finds suitable.
 int pthread_key_delete(pthread_key_t key) {
-  if (!KeyInValidRange(key)) {
+  if (__predict_false(!KeyInValidRange(key))) {
     return EINVAL;
   }
+  key &= ~KEY_VALID_FLAG;
   // Increase seq to invalidate values in all threads.
   uintptr_t seq = atomic_load_explicit(&key_map[key].seq, memory_order_relaxed);
   if (SeqOfKeyInUse(seq)) {
@@ -141,9 +149,10 @@ int pthread_key_delete(pthread_key_t key) {
 }
 
 void* pthread_getspecific(pthread_key_t key) {
-  if (!KeyInValidRange(key)) {
+  if (__predict_false(!KeyInValidRange(key))) {
     return NULL;
   }
+  key &= ~KEY_VALID_FLAG;
   uintptr_t seq = atomic_load_explicit(&key_map[key].seq, memory_order_relaxed);
   pthread_key_data_t* data = &(__get_thread()->key_data[key]);
   // It is user's responsibility to synchornize between the creation and use of pthread keys,
@@ -151,16 +160,19 @@ void* pthread_getspecific(pthread_key_t key) {
   if (__predict_true(SeqOfKeyInUse(seq) && data->seq == seq)) {
     return data->data;
   }
+  // We arrive here when current thread holds the seq of an deleted pthread key. So the
+  // data is for the deleted pthread key, and should be cleared.
   data->data = NULL;
   return NULL;
 }
 
 int pthread_setspecific(pthread_key_t key, const void* ptr) {
-  if (!KeyInValidRange(key)) {
+  if (__predict_false(!KeyInValidRange(key))) {
     return EINVAL;
   }
+  key &= ~KEY_VALID_FLAG;
   uintptr_t seq = atomic_load_explicit(&key_map[key].seq, memory_order_relaxed);
-  if (SeqOfKeyInUse(seq)) {
+  if (__predict_true(SeqOfKeyInUse(seq))) {
     pthread_key_data_t* data = &(__get_thread()->key_data[key]);
     data->seq = seq;
     data->data = const_cast<void*>(ptr);
diff --git a/tests/pthread_test.cpp b/tests/pthread_test.cpp
index f96ccf9d0..6dbd964d1 100644
--- a/tests/pthread_test.cpp
+++ b/tests/pthread_test.cpp
@@ -181,6 +181,19 @@ TEST(pthread, pthread_key_dirty) {
   ASSERT_EQ(0, pthread_key_delete(key));
 }
 
+TEST(pthread, static_pthread_key_used_before_creation) {
+#if defined(__BIONIC__)
+  // See http://b/19625804. The bug is about a static/global pthread key being used before creation.
+  // So here tests if the static/global default value 0 can be detected as invalid key.
+  static pthread_key_t key;
+  ASSERT_EQ(nullptr, pthread_getspecific(key));
+  ASSERT_EQ(EINVAL, pthread_setspecific(key, nullptr));
+  ASSERT_EQ(EINVAL, pthread_key_delete(key));
+#else
+  GTEST_LOG_(INFO) << "This test tests bionic pthread key implementation detail.\n";
+#endif
+}
+
 static void* IdFn(void* arg) {
   return arg;
 }