Fix potential spinlock timeout for heap disable callbacks.
If the disabled callback for one heap that was enabled triggered
operations on another one, we could re-enter AHeapProfile_report*
while holding the spinlock.
Change-Id: I43d6f9d7676c3a57ed62eedf615db8fe1c3d71ac
diff --git a/src/profiling/memory/client_api.cc b/src/profiling/memory/client_api.cc
index bdede0d..9c3f6d1 100644
--- a/src/profiling/memory/client_api.cc
+++ b/src/profiling/memory/client_api.cc
@@ -133,32 +133,49 @@
std::atomic<uint32_t> g_next_heap_id{kMinHeapId};
+// This can get called while holding the spinlock (in normal operation), or
+// without holding the spinlock (from OnSpinlockTimeout).
void DisableAllHeaps() {
- for (uint32_t i = kMinHeapId; i < g_next_heap_id.load(); ++i) {
+ bool disabled[perfetto::base::ArraySize(g_heaps)] = {};
+ uint32_t max_heap = g_next_heap_id.load();
+ // This has to be done in two passes, in case the disabled_callback for one
+ // enabled heap uses another. In that case, the callbacks for the other heap
+ // would time out trying to acquire the spinlock, which we hold here.
+ for (uint32_t i = kMinHeapId; i < max_heap; ++i) {
AHeapInfo& info = GetHeap(i);
if (!info.ready.load(std::memory_order_acquire))
continue;
- if (info.enabled.load(std::memory_order_acquire)) {
- info.enabled.store(false, std::memory_order_release);
- if (info.disabled_callback) {
- AHeapProfileDisableCallbackInfo disable_info;
- info.disabled_callback(info.disabled_callback_data, &disable_info);
- }
+ disabled[i] = info.enabled.exchange(false, std::memory_order_acq_rel);
+ }
+ for (uint32_t i = kMinHeapId; i < max_heap; ++i) {
+ if (!disabled[i]) {
+ continue;
+ }
+ AHeapInfo& info = GetHeap(i);
+ if (info.disabled_callback) {
+ AHeapProfileDisableCallbackInfo disable_info;
+ info.disabled_callback(info.disabled_callback_data, &disable_info);
}
}
}
+#pragma GCC diagnostic push
+#if PERFETTO_DCHECK_IS_ON()
+#pragma GCC diagnostic ignored "-Wmissing-noreturn"
+#endif
+
void OnSpinlockTimeout() {
// Give up on profiling the process but leave it running.
// The process enters into a poisoned state and will reject all
// subsequent profiling requests. The current session is kept
// running but no samples are reported to it.
- PERFETTO_ELOG(
+ PERFETTO_DFATAL_OR_ELOG(
"Timed out on the spinlock - something is horribly wrong. "
"Leaking heapprofd client.");
DisableAllHeaps();
perfetto::profiling::PoisonSpinlock(&g_client_lock);
}
+#pragma GCC diagnostic pop
// Note: g_client can be reset by AHeapProfile_initSession without calling this
// function.
diff --git a/src/profiling/memory/heapprofd_end_to_end_test.cc b/src/profiling/memory/heapprofd_end_to_end_test.cc
index d406ace..525d865 100644
--- a/src/profiling/memory/heapprofd_end_to_end_test.cc
+++ b/src/profiling/memory/heapprofd_end_to_end_test.cc
@@ -19,6 +19,7 @@
#include <vector>
#include <fcntl.h>
+#include <stdint.h>
#include <string.h>
#include <sys/stat.h>
#include <sys/types.h>
@@ -454,6 +455,7 @@
static std::atomic<bool> disabled{false};
static std::atomic<uint64_t> sampling_interval;
+ static uint32_t other_heap_id = 0;
auto enabled_callback = [](void*,
const AHeapProfileEnableCallbackInfo* info) {
sampling_interval =
@@ -461,6 +463,8 @@
initialized = true;
};
auto disabled_callback = [](void*, const AHeapProfileDisableCallbackInfo*) {
+ PERFETTO_CHECK(other_heap_id);
+ AHeapProfile_reportFree(other_heap_id, 0);
disabled = true;
};
static uint32_t heap_id =
@@ -469,6 +473,7 @@
enabled_callback, nullptr),
disabled_callback, nullptr));
+ other_heap_id = AHeapProfile_registerHeap(AHeapInfo_create("othertest"));
ChildFinishHandshake();
// heapprofd_client needs malloc to see the signal.
@@ -1136,6 +1141,7 @@
cfg->set_sampling_interval_bytes(1000000);
cfg->add_pid(pid);
cfg->add_heaps("test");
+ cfg->add_heaps("othertest");
});
auto helper = Trace(trace_config);
