Fix deadlock/timeout in thread unwinding.
When malloc debug is enabled, using libbacktrace to unwind can
result in a deadlock. This happens when an unwind of a thread
is occuring which triggers a signal to be sent to that thread. If
that thread is interrupted while a malloc debug function is
executing and owns a lock, that thread is then stuck in the signal
handler. Then the original unwinding thread attempts to do an
allocation and gets stuck waiting for the same malloc debug lock.
This is not a complete deadlock since the unwinder has timeouts,
but it results in truncated unwinds that take at least five
seconds to complete.
Only the backtrace signals needs to be blocked because it is the only
known signal that will result in a thread being paused in a signal
handler.
Also, added a named signal in the reserved signal list for the
special bionic backtrace signal.
Bug: 150833265
Test: New unit tests pass with fix, fail without fix.
Change-Id: If3e41f092ebd40ce62a59ef51d636a91bc31ed80
diff --git a/libc/malloc_debug/Android.bp b/libc/malloc_debug/Android.bp
index b04cfe1..760039b 100644
--- a/libc/malloc_debug/Android.bp
+++ b/libc/malloc_debug/Android.bp
@@ -173,6 +173,7 @@
shared_libs: [
"libbase",
+ "libbacktrace",
"liblog",
"libunwindstack",
],
diff --git a/libc/malloc_debug/malloc_debug.cpp b/libc/malloc_debug/malloc_debug.cpp
index 6eaac7d..609f030 100644
--- a/libc/malloc_debug/malloc_debug.cpp
+++ b/libc/malloc_debug/malloc_debug.cpp
@@ -30,11 +30,13 @@
#include <inttypes.h>
#include <malloc.h>
#include <pthread.h>
+#include <signal.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <sys/cdefs.h>
#include <sys/param.h>
+#include <sys/syscall.h>
#include <unistd.h>
#include <mutex>
@@ -44,8 +46,9 @@
#include <android-base/properties.h>
#include <android-base/stringprintf.h>
#include <bionic/malloc_tagged_pointers.h>
-#include <private/bionic_malloc_dispatch.h>
+#include <platform/bionic/reserved_signals.h>
#include <private/MallocXmlElem.h>
+#include <private/bionic_malloc_dispatch.h>
#include "Config.h"
#include "DebugData.h"
@@ -131,6 +134,40 @@
};
pthread_rwlock_t ScopedConcurrentLock::lock_;
+// Use this because the sigprocmask* functions filter out the reserved bionic
+// signals including the signal this code blocks.
+static inline int __rt_sigprocmask(int how, const sigset64_t* new_set, sigset64_t* old_set,
+ size_t sigset_size) {
+ return syscall(SYS_rt_sigprocmask, how, new_set, old_set, sigset_size);
+}
+
+// Need to block the backtrace signal while in malloc debug routines
+// otherwise there is a chance of a deadlock and timeout when unwinding.
+// This can occur if a thread is paused while owning a malloc debug
+// internal lock.
+class ScopedBacktraceSignalBlocker {
+ public:
+ ScopedBacktraceSignalBlocker() {
+ sigemptyset64(&backtrace_set_);
+ sigaddset64(&backtrace_set_, BIONIC_SIGNAL_BACKTRACE);
+ sigset64_t old_set;
+ __rt_sigprocmask(SIG_BLOCK, &backtrace_set_, &old_set, sizeof(backtrace_set_));
+ if (sigismember64(&old_set, BIONIC_SIGNAL_BACKTRACE)) {
+ unblock_ = false;
+ }
+ }
+
+ ~ScopedBacktraceSignalBlocker() {
+ if (unblock_) {
+ __rt_sigprocmask(SIG_UNBLOCK, &backtrace_set_, nullptr, sizeof(backtrace_set_));
+ }
+ }
+
+ private:
+ bool unblock_ = true;
+ sigset64_t backtrace_set_;
+};
+
static void InitAtfork() {
static pthread_once_t atfork_init = PTHREAD_ONCE_INIT;
pthread_once(&atfork_init, []() {
@@ -334,8 +371,8 @@
void debug_get_malloc_leak_info(uint8_t** info, size_t* overall_size, size_t* info_size,
size_t* total_memory, size_t* backtrace_size) {
ScopedConcurrentLock lock;
-
ScopedDisableDebugCalls disable;
+ ScopedBacktraceSignalBlocker blocked;
// Verify the arguments.
if (info == nullptr || overall_size == nullptr || info_size == nullptr || total_memory == nullptr ||
@@ -370,6 +407,7 @@
}
ScopedConcurrentLock lock;
ScopedDisableDebugCalls disable;
+ ScopedBacktraceSignalBlocker blocked;
if (!VerifyPointer(pointer, "malloc_usable_size")) {
return 0;
@@ -434,6 +472,7 @@
}
ScopedConcurrentLock lock;
ScopedDisableDebugCalls disable;
+ ScopedBacktraceSignalBlocker blocked;
void* pointer = InternalMalloc(size);
@@ -510,6 +549,7 @@
}
ScopedConcurrentLock lock;
ScopedDisableDebugCalls disable;
+ ScopedBacktraceSignalBlocker blocked;
if (g_debug->config().options() & RECORD_ALLOCS) {
g_debug->record->AddEntry(new FreeEntry(pointer));
@@ -528,6 +568,7 @@
}
ScopedConcurrentLock lock;
ScopedDisableDebugCalls disable;
+ ScopedBacktraceSignalBlocker blocked;
if (bytes == 0) {
bytes = 1;
@@ -607,6 +648,7 @@
}
ScopedConcurrentLock lock;
ScopedDisableDebugCalls disable;
+ ScopedBacktraceSignalBlocker blocked;
if (pointer == nullptr) {
pointer = InternalMalloc(bytes);
@@ -726,6 +768,7 @@
}
ScopedConcurrentLock lock;
ScopedDisableDebugCalls disable;
+ ScopedBacktraceSignalBlocker blocked;
size_t size;
if (__builtin_mul_overflow(nmemb, bytes, &size)) {
@@ -792,6 +835,7 @@
ScopedConcurrentLock lock;
ScopedDisableDebugCalls disable;
+ ScopedBacktraceSignalBlocker blocked;
// Avoid any issues where allocations are made that will be freed
// in the fclose.
@@ -880,6 +924,7 @@
}
ScopedConcurrentLock lock;
ScopedDisableDebugCalls disable;
+ ScopedBacktraceSignalBlocker blocked;
if (!(g_debug->config().options() & BACKTRACE)) {
return 0;
@@ -938,6 +983,7 @@
ScopedConcurrentLock lock;
ScopedDisableDebugCalls disable;
+ ScopedBacktraceSignalBlocker blocked;
std::lock_guard<std::mutex> guard(g_dump_lock);
@@ -953,6 +999,7 @@
void debug_dump_heap(const char* file_name) {
ScopedConcurrentLock lock;
ScopedDisableDebugCalls disable;
+ ScopedBacktraceSignalBlocker blocked;
std::lock_guard<std::mutex> guard(g_dump_lock);
diff --git a/libc/malloc_debug/tests/malloc_debug_system_tests.cpp b/libc/malloc_debug/tests/malloc_debug_system_tests.cpp
index ac19cf9..1bfe61e 100644
--- a/libc/malloc_debug/tests/malloc_debug_system_tests.cpp
+++ b/libc/malloc_debug/tests/malloc_debug_system_tests.cpp
@@ -42,12 +42,20 @@
#include <gtest/gtest.h>
#include <log/log_read.h>
+#include <atomic>
#include <string>
#include <thread>
#include <vector>
+#include <backtrace/Backtrace.h>
+#include <backtrace/BacktraceMap.h>
+
#include <bionic/malloc.h>
+// All DISABLED_ tests are designed to be executed after malloc debug
+// is enabled. These tests don't run be default, and are executed
+// by wrappers that will enable various malloc debug features.
+
static constexpr time_t kTimeoutSeconds = 10;
extern "C" bool GetInitialArgs(const char*** args, size_t* num_args) {
@@ -525,3 +533,42 @@
ASSERT_NO_FATAL_FAILURE(FindStrings(pid, std::vector<const char*>{"malloc debug enabled"},
std::vector<const char*>{" HAS INVALID TAG ", "USED AFTER FREE ", "UNKNOWN POINTER "}));
}
+
+TEST(MallocTests, DISABLED_malloc_and_backtrace_deadlock) {
+ std::atomic_bool running(false);
+ pid_t tid;
+ std::thread thread([&tid, &running] {
+ tid = gettid();
+ running = true;
+ while (running) {
+ void* ptr = malloc(200);
+ if (ptr == nullptr) {
+ return;
+ }
+ free(ptr);
+ }
+ });
+
+ while (!running) {
+ }
+
+ static constexpr size_t kNumUnwinds = 1000;
+ for (size_t i = 0; i < kNumUnwinds; i++) {
+ std::unique_ptr<Backtrace> backtrace(Backtrace::Create(getpid(), tid));
+ ASSERT_TRUE(backtrace->Unwind(0)) << "Failed on unwind " << i;
+ ASSERT_LT(1, backtrace->NumFrames()) << "Failed on unwind " << i;
+ }
+ running = false;
+ thread.join();
+}
+
+TEST(MallocDebugSystemTest, malloc_and_backtrace_deadlock) {
+ pid_t pid;
+ ASSERT_NO_FATAL_FAILURE(Exec("MallocTests.DISABLED_malloc_and_backtrace_deadlock",
+ "verbose verify_pointers", &pid, 0, 180));
+
+ // Make sure that malloc debug is enabled and that no timeouts occur during
+ // unwinds.
+ ASSERT_NO_FATAL_FAILURE(FindStrings(pid, std::vector<const char*>{"malloc debug enabled"},
+ std::vector<const char*>{"Timed out waiting for "}));
+}
diff --git a/libc/platform/bionic/reserved_signals.h b/libc/platform/bionic/reserved_signals.h
index c90fc06..e8e517e 100644
--- a/libc/platform/bionic/reserved_signals.h
+++ b/libc/platform/bionic/reserved_signals.h
@@ -48,6 +48,7 @@
// in <android/legacy_signal_inlines.h> to match.
#define BIONIC_SIGNAL_POSIX_TIMERS (__SIGRTMIN + 0)
+#define BIONIC_SIGNAL_BACKTRACE (__SIGRTMIN + 1)
#define BIONIC_SIGNAL_DEBUGGER (__SIGRTMIN + 3)
#define BIONIC_SIGNAL_PROFILER (__SIGRTMIN + 4)
#define BIONIC_SIGNAL_ART_PROFILER (__SIGRTMIN + 6)