Merge "Fix bug for recursive/errorcheck mutex on 32-bit devices."

libc/bionic/pthread_mutex.cpp

@@ -392,6 +392,30 @@ static inline __always_inline int __recursive_increment(pthread_mutex_internal_t
     return 0;
 }
 
+static inline __always_inline int __recursive_or_errorcheck_mutex_wait(
+                                                      pthread_mutex_internal_t* mutex,
+                                                      uint16_t shared,
+                                                      uint16_t old_state,
+                                                      const timespec* rel_timeout) {
+// __futex_wait always waits on a 32-bit value. But state is 16-bit. For a normal mutex, the owner_tid
+// field in mutex is not used. On 64-bit devices, the __pad field in mutex is not used.
+// But when a recursive or errorcheck mutex is used on 32-bit devices, we need to add the
+// owner_tid value in the value argument for __futex_wait, otherwise we may always get EAGAIN error.
+
+#if defined(__LP64__)
+  return __futex_wait_ex(&mutex->state, shared, old_state, rel_timeout);
+
+#else
+  // This implementation works only when the layout of pthread_mutex_internal_t matches below expectation.
+  // And it is based on the assumption that Android is always in little-endian devices.
+  static_assert(offsetof(pthread_mutex_internal_t, state) == 0, "");
+  static_assert(offsetof(pthread_mutex_internal_t, owner_tid) == 2, "");
+
+  uint32_t owner_tid = atomic_load_explicit(&mutex->owner_tid, memory_order_relaxed);
+  return __futex_wait_ex(&mutex->state, shared, (owner_tid << 16) | old_state, rel_timeout);
+#endif
+}
+
 static int __pthread_mutex_lock_with_timeout(pthread_mutex_internal_t* mutex,
                                            const timespec* abs_timeout_or_null, clockid_t clock) {
     uint16_t old_state = atomic_load_explicit(&mutex->state, memory_order_relaxed);
@@ -469,7 +493,7 @@ static int __pthread_mutex_lock_with_timeout(pthread_mutex_internal_t* mutex,
                 return ETIMEDOUT;
             }
         }
-        if (__futex_wait_ex(&mutex->state, shared, old_state, rel_timeout) == -ETIMEDOUT) {
+        if (__recursive_or_errorcheck_mutex_wait(mutex, shared, old_state, rel_timeout) == -ETIMEDOUT) {
             return ETIMEDOUT;
         }
         old_state = atomic_load_explicit(&mutex->state, memory_order_relaxed);

tests/Android.mk

@@ -128,6 +128,9 @@ libBionicStandardTests_c_includes := \
     bionic/libc \
     external/tinyxml2 \
 
+libBionicStandardTests_static_libraries := \
+    libbase \
+
 libBionicStandardTests_ldlibs_host := \
     -lrt \
 
@@ -257,6 +260,7 @@ bionic-unit-tests_whole_static_libraries := \
 bionic-unit-tests_static_libraries := \
     libtinyxml2 \
     liblog \
+    libbase \
 
 # TODO: Include __cxa_thread_atexit_test.cpp to glibc tests once it is upgraded (glibc 2.18+)
 bionic-unit-tests_src_files := \
@@ -317,6 +321,7 @@ bionic-unit-tests-static_static_libraries := \
     libdl \
     libtinyxml2 \
     liblog \
+    libbase \
 
 bionic-unit-tests-static_force_static_executable := true
 
@@ -355,6 +360,11 @@ bionic-unit-tests-glibc_whole_static_libraries := \
     libBionicGtestMain \
     $(fortify_libs) \
 
+bionic-unit-tests-glibc_static_libraries := \
+    libbase \
+    liblog \
+    libcutils \
+
 bionic-unit-tests-glibc_ldlibs := \
     -lrt -ldl -lutil \
 

tests/pthread_test.cpp

@@ -29,13 +29,19 @@
 #include <unistd.h>
 
 #include <atomic>
+#include <regex>
 #include <vector>
 
+#include <base/file.h>
+#include <base/stringprintf.h>
+
 #include "private/bionic_macros.h"
 #include "private/ScopeGuard.h"
 #include "BionicDeathTest.h"
 #include "ScopedSignalHandler.h"
 
+extern "C" pid_t gettid();
+
 TEST(pthread, pthread_key_create) {
   pthread_key_t key;
   ASSERT_EQ(0, pthread_key_create(&key, NULL));
@@ -704,6 +710,23 @@ TEST(pthread, pthread_rwlock_smoke) {
   ASSERT_EQ(0, pthread_rwlock_destroy(&l));
 }
 
+static void WaitUntilThreadSleep(std::atomic<pid_t>& pid) {
+  while (pid == 0) {
+    usleep(1000);
+  }
+  std::string filename = android::base::StringPrintf("/proc/%d/stat", pid.load());
+  std::regex regex {R"(\s+S\s+)"};
+
+  while (true) {
+    std::string content;
+    ASSERT_TRUE(android::base::ReadFileToString(filename, &content));
+    if (std::regex_search(content, regex)) {
+      break;
+    }
+    usleep(1000);
+  }
+}
+
 struct RwlockWakeupHelperArg {
   pthread_rwlock_t lock;
   enum Progress {
@@ -713,9 +736,11 @@ struct RwlockWakeupHelperArg {
     LOCK_ACCESSED
   };
   std::atomic<Progress> progress;
+  std::atomic<pid_t> tid;
 };
 
 static void pthread_rwlock_reader_wakeup_writer_helper(RwlockWakeupHelperArg* arg) {
+  arg->tid = gettid();
   ASSERT_EQ(RwlockWakeupHelperArg::LOCK_INITIALIZED, arg->progress);
   arg->progress = RwlockWakeupHelperArg::LOCK_WAITING;
 
@@ -732,14 +757,14 @@ TEST(pthread, pthread_rwlock_reader_wakeup_writer) {
   ASSERT_EQ(0, pthread_rwlock_init(&wakeup_arg.lock, NULL));
   ASSERT_EQ(0, pthread_rwlock_rdlock(&wakeup_arg.lock));
   wakeup_arg.progress = RwlockWakeupHelperArg::LOCK_INITIALIZED;
+  wakeup_arg.tid = 0;
 
   pthread_t thread;
   ASSERT_EQ(0, pthread_create(&thread, NULL,
     reinterpret_cast<void* (*)(void*)>(pthread_rwlock_reader_wakeup_writer_helper), &wakeup_arg));
-  while (wakeup_arg.progress != RwlockWakeupHelperArg::LOCK_WAITING) {
-    usleep(5000);
-  }
-  usleep(5000);
+  WaitUntilThreadSleep(wakeup_arg.tid);
+  ASSERT_EQ(RwlockWakeupHelperArg::LOCK_WAITING, wakeup_arg.progress);
+
   wakeup_arg.progress = RwlockWakeupHelperArg::LOCK_RELEASED;
   ASSERT_EQ(0, pthread_rwlock_unlock(&wakeup_arg.lock));
 
@@ -749,6 +774,7 @@ TEST(pthread, pthread_rwlock_reader_wakeup_writer) {
 }
 
 static void pthread_rwlock_writer_wakeup_reader_helper(RwlockWakeupHelperArg* arg) {
+  arg->tid = gettid();
   ASSERT_EQ(RwlockWakeupHelperArg::LOCK_INITIALIZED, arg->progress);
   arg->progress = RwlockWakeupHelperArg::LOCK_WAITING;
 
@@ -765,14 +791,14 @@ TEST(pthread, pthread_rwlock_writer_wakeup_reader) {
   ASSERT_EQ(0, pthread_rwlock_init(&wakeup_arg.lock, NULL));
   ASSERT_EQ(0, pthread_rwlock_wrlock(&wakeup_arg.lock));
   wakeup_arg.progress = RwlockWakeupHelperArg::LOCK_INITIALIZED;
+  wakeup_arg.tid = 0;
 
   pthread_t thread;
   ASSERT_EQ(0, pthread_create(&thread, NULL,
     reinterpret_cast<void* (*)(void*)>(pthread_rwlock_writer_wakeup_reader_helper), &wakeup_arg));
-  while (wakeup_arg.progress != RwlockWakeupHelperArg::LOCK_WAITING) {
-    usleep(5000);
-  }
-  usleep(5000);
+  WaitUntilThreadSleep(wakeup_arg.tid);
+  ASSERT_EQ(RwlockWakeupHelperArg::LOCK_WAITING, wakeup_arg.progress);
+
   wakeup_arg.progress = RwlockWakeupHelperArg::LOCK_RELEASED;
   ASSERT_EQ(0, pthread_rwlock_unlock(&wakeup_arg.lock));
 
@@ -1263,7 +1289,6 @@ TEST(pthread, pthread_mutex_init_same_as_static_initializers) {
   ASSERT_EQ(0, memcmp(&lock_recursive, &m3.lock, sizeof(pthread_mutex_t)));
   ASSERT_EQ(0, pthread_mutex_destroy(&lock_recursive));
 }
-
 class MutexWakeupHelper {
  private:
   PthreadMutex m;
@@ -1274,8 +1299,10 @@ class MutexWakeupHelper {
     LOCK_ACCESSED
   };
   std::atomic<Progress> progress;
+  std::atomic<pid_t> tid;
 
   static void thread_fn(MutexWakeupHelper* helper) {
+    helper->tid = gettid();
     ASSERT_EQ(LOCK_INITIALIZED, helper->progress);
     helper->progress = LOCK_WAITING;
 
@@ -1293,15 +1320,15 @@ class MutexWakeupHelper {
   void test() {
     ASSERT_EQ(0, pthread_mutex_lock(&m.lock));
     progress = LOCK_INITIALIZED;
+    tid = 0;
 
     pthread_t thread;
     ASSERT_EQ(0, pthread_create(&thread, NULL,
       reinterpret_cast<void* (*)(void*)>(MutexWakeupHelper::thread_fn), this));
 
-    while (progress != LOCK_WAITING) {
-      usleep(5000);
-    }
-    usleep(5000);
+    WaitUntilThreadSleep(tid);
+    ASSERT_EQ(LOCK_WAITING, progress);
+
     progress = LOCK_RELEASED;
     ASSERT_EQ(0, pthread_mutex_unlock(&m.lock));
 

tests/stack_protector_test.cpp

@@ -24,14 +24,10 @@
 #include <pthread.h>
 #include <stdint.h>
 #include <stdio.h>
-#include <sys/syscall.h>
 #include <unistd.h>
 #include <set>
 
-#if defined(__GLIBC__)
-// glibc doesn't expose gettid(2).
-pid_t gettid() { return syscall(__NR_gettid); }
-#endif // __GLIBC__
+extern "C" pid_t gettid();
 
 // For x86, bionic and glibc have per-thread stack guard values (all identical).
 #if defined(__i386__)