From f796985923e2d8308e00ed9567f36546dafb98d7 Mon Sep 17 00:00:00 2001 From: Yabin Cui Date: Thu, 2 Apr 2015 17:47:48 -0700 Subject: [PATCH] Fix bug for recursive/errorcheck mutex on 32-bit devices. Bug: 19216648 Change-Id: I3b43b2d18d25b9bde352da1e35f9568133dec7cf --- libc/bionic/pthread_mutex.cpp | 26 ++++++++++++++++- tests/Android.mk | 10 +++++++ tests/pthread_test.cpp | 53 +++++++++++++++++++++++++--------- tests/stack_protector_test.cpp | 6 +--- 4 files changed, 76 insertions(+), 19 deletions(-) diff --git a/libc/bionic/pthread_mutex.cpp b/libc/bionic/pthread_mutex.cpp index 5bdc5ed98..4fec7535c 100644 --- a/libc/bionic/pthread_mutex.cpp +++ b/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); diff --git a/tests/Android.mk b/tests/Android.mk index 995877eaf..c94237593 100644 --- a/tests/Android.mk +++ b/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 \ diff --git a/tests/pthread_test.cpp b/tests/pthread_test.cpp index 5ab1f116b..f96ccf9d0 100644 --- a/tests/pthread_test.cpp +++ b/tests/pthread_test.cpp @@ -29,13 +29,19 @@ #include #include +#include #include +#include +#include + #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) { + 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; + std::atomic 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(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(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; + std::atomic 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(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 80077113b..22285d123 100644 --- a/tests/stack_protector_test.cpp +++ b/tests/stack_protector_test.cpp @@ -24,14 +24,10 @@ #include #include #include -#include #include #include -#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__)