Fix bug for recursive/errorcheck mutex on 32-bit devices.
Bug: 19216648
Change-Id: I3b43b2d18d25b9bde352da1e35f9568133dec7cf
diff --git a/libc/bionic/pthread_mutex.cpp b/libc/bionic/pthread_mutex.cpp
index 5bdc5ed..4fec753 100644
--- a/libc/bionic/pthread_mutex.cpp
+++ b/libc/bionic/pthread_mutex.cpp
@@ -392,6 +392,30 @@
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 @@
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);
diff --git a/tests/Android.mk b/tests/Android.mk
index 995877e..c942375 100644
--- a/tests/Android.mk
+++ b/tests/Android.mk
@@ -128,6 +128,9 @@
bionic/libc \
external/tinyxml2 \
+libBionicStandardTests_static_libraries := \
+ libbase \
+
libBionicStandardTests_ldlibs_host := \
-lrt \
@@ -257,6 +260,7 @@
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 @@
libdl \
libtinyxml2 \
liblog \
+ libbase \
bionic-unit-tests-static_force_static_executable := true
@@ -355,6 +360,11 @@
libBionicGtestMain \
$(fortify_libs) \
+bionic-unit-tests-glibc_static_libraries := \
+ libbase \
+ liblog \
+ libcutils \
+
bionic-unit-tests-glibc_ldlibs := \
-lrt -ldl -lutil \
diff --git a/tests/pthread_test.cpp b/tests/pthread_test.cpp
index 5ab1f11..f96ccf9 100644
--- a/tests/pthread_test.cpp
+++ b/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 @@
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 @@
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 @@
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 @@
}
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 @@
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 @@
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 @@
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 @@
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));
diff --git a/tests/stack_protector_test.cpp b/tests/stack_protector_test.cpp
index 8007711..22285d1 100644
--- a/tests/stack_protector_test.cpp
+++ b/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__)