Fix races in record allocs.
When recording allocation operations, it's possible to create invalid
traces if operations get out of order because an entry is added
after the operation is completed. To fix this, reserve the entry
before executing the operation, then fill it in afterwards.
Need to allow for a reserved entry to be invalid in cases where the
operation can fail.
Test: All unit tests pass.
Change-Id: Ic7d01a1682da8742f34750e86dd163c085e709a3
diff --git a/libc/malloc_debug/RecordData.cpp b/libc/malloc_debug/RecordData.cpp
index 1641732..f832f03 100644
--- a/libc/malloc_debug/RecordData.cpp
+++ b/libc/malloc_debug/RecordData.cpp
@@ -55,7 +55,7 @@
size_t count = 0;
};
-static void ThreadKeyDelete(void* data) {
+void RecordData::ThreadKeyDelete(void* data) {
ThreadData* thread_data = reinterpret_cast<ThreadData*>(data);
thread_data->count++;
@@ -64,8 +64,11 @@
if (thread_data->count == 4) {
ScopedDisableDebugCalls disable;
- thread_data->record_data->AddEntryOnly(memory_trace::Entry{
- .tid = gettid(), .type = memory_trace::THREAD_DONE, .end_ns = Nanotime()});
+ memory_trace::Entry* entry = thread_data->record_data->InternalReserveEntry();
+ if (entry != nullptr) {
+ *entry = memory_trace::Entry{
+ .tid = gettid(), .type = memory_trace::THREAD_DONE, .end_ns = Nanotime()};
+ }
delete thread_data;
} else {
pthread_setspecific(thread_data->record_data->key(), data);
@@ -107,6 +110,11 @@
}
for (size_t i = 0; i < cur_index_; i++) {
+ if (entries_[i].type == memory_trace::UNKNOWN) {
+ // This can happen if an entry was reserved but not filled in due to some
+ // type of error during the operation.
+ continue;
+ }
if (!memory_trace::WriteEntryToFd(dump_fd, entries_[i])) {
error_log("Failed to write record alloc information: %s", strerror(errno));
break;
@@ -149,25 +157,26 @@
pthread_key_delete(key_);
}
-void RecordData::AddEntryOnly(const memory_trace::Entry& entry) {
+memory_trace::Entry* RecordData::InternalReserveEntry() {
std::lock_guard<std::mutex> entries_lock(entries_lock_);
if (cur_index_ == entries_.size()) {
- // Maxed out, throw the entry away.
- return;
+ return nullptr;
}
- entries_[cur_index_++] = entry;
- if (cur_index_ == entries_.size()) {
+ memory_trace::Entry* entry = &entries_[cur_index_];
+ entry->type = memory_trace::UNKNOWN;
+ if (++cur_index_ == entries_.size()) {
info_log("Maximum number of records added, all new operations will be dropped.");
}
+ return entry;
}
-void RecordData::AddEntry(const memory_trace::Entry& entry) {
+memory_trace::Entry* RecordData::ReserveEntry() {
void* data = pthread_getspecific(key_);
if (data == nullptr) {
ThreadData* thread_data = new ThreadData(this);
pthread_setspecific(key_, thread_data);
}
- AddEntryOnly(entry);
+ return InternalReserveEntry();
}
diff --git a/libc/malloc_debug/RecordData.h b/libc/malloc_debug/RecordData.h
index f4b0d82..ce71da1 100644
--- a/libc/malloc_debug/RecordData.h
+++ b/libc/malloc_debug/RecordData.h
@@ -51,8 +51,7 @@
bool Initialize(const Config& config);
- void AddEntry(const memory_trace::Entry& entry);
- void AddEntryOnly(const memory_trace::Entry& entry);
+ memory_trace::Entry* ReserveEntry();
const std::string& file() { return file_; }
pthread_key_t key() { return key_; }
@@ -63,9 +62,13 @@
static void WriteData(int, siginfo_t*, void*);
static RecordData* record_obj_;
+ static void ThreadKeyDelete(void* data);
+
void WriteEntries();
void WriteEntries(const std::string& file);
+ memory_trace::Entry* InternalReserveEntry();
+
std::mutex entries_lock_;
pthread_key_t key_;
std::vector<memory_trace::Entry> entries_;
diff --git a/libc/malloc_debug/malloc_debug.cpp b/libc/malloc_debug/malloc_debug.cpp
index a662529..fce6c24 100644
--- a/libc/malloc_debug/malloc_debug.cpp
+++ b/libc/malloc_debug/malloc_debug.cpp
@@ -590,16 +590,22 @@
ScopedDisableDebugCalls disable;
ScopedBacktraceSignalBlocker blocked;
+ memory_trace::Entry* entry = nullptr;
+ if (g_debug->config().options() & RECORD_ALLOCS) {
+ // In order to preserve the order of operations, reserve the entry before
+ // performing the operation.
+ entry = g_debug->record->ReserveEntry();
+ }
+
TimedResult result = InternalMalloc(size);
- if (g_debug->config().options() & RECORD_ALLOCS) {
- g_debug->record->AddEntry(
- memory_trace::Entry{.tid = gettid(),
- .type = memory_trace::MALLOC,
- .ptr = reinterpret_cast<uint64_t>(result.getValue<void*>()),
- .size = size,
- .start_ns = result.GetStartTimeNS(),
- .end_ns = result.GetEndTimeNS()});
+ if (entry != nullptr) {
+ *entry = memory_trace::Entry{.tid = gettid(),
+ .type = memory_trace::MALLOC,
+ .ptr = reinterpret_cast<uint64_t>(result.getValue<void*>()),
+ .size = size,
+ .start_ns = result.GetStartTimeNS(),
+ .end_ns = result.GetEndTimeNS()};
}
return result.getValue<void*>();
@@ -684,14 +690,21 @@
return;
}
+ memory_trace::Entry* entry = nullptr;
+ if (g_debug->config().options() & RECORD_ALLOCS) {
+ // In order to preserve the order of operations, reserve the entry before
+ // performing the operation.
+ entry = g_debug->record->ReserveEntry();
+ }
+
TimedResult result = InternalFree(pointer);
- if (g_debug->config().options() & RECORD_ALLOCS) {
- g_debug->record->AddEntry(memory_trace::Entry{.tid = gettid(),
- .type = memory_trace::FREE,
- .ptr = reinterpret_cast<uint64_t>(pointer),
- .start_ns = result.GetStartTimeNS(),
- .end_ns = result.GetEndTimeNS()});
+ if (entry != nullptr) {
+ *entry = memory_trace::Entry{.tid = gettid(),
+ .type = memory_trace::FREE,
+ .ptr = reinterpret_cast<uint64_t>(pointer),
+ .start_ns = result.GetStartTimeNS(),
+ .end_ns = result.GetEndTimeNS()};
}
}
@@ -714,6 +727,13 @@
return nullptr;
}
+ memory_trace::Entry* entry = nullptr;
+ if (g_debug->config().options() & RECORD_ALLOCS) {
+ // In order to preserve the order of operations, reserve the entry before
+ // performing the operation.
+ entry = g_debug->record->ReserveEntry();
+ }
+
TimedResult result;
void* pointer;
if (g_debug->HeaderEnabled()) {
@@ -761,27 +781,29 @@
pointer = result.getValue<void*>();
}
- if (pointer != nullptr) {
- if (g_debug->TrackPointers()) {
- PointerData::Add(pointer, bytes);
- }
+ if (pointer == nullptr) {
+ return nullptr;
+ }
- if (g_debug->config().options() & FILL_ON_ALLOC) {
- size_t bytes = InternalMallocUsableSize(pointer);
- size_t fill_bytes = g_debug->config().fill_on_alloc_bytes();
- bytes = (bytes < fill_bytes) ? bytes : fill_bytes;
- memset(pointer, g_debug->config().fill_alloc_value(), bytes);
- }
+ if (g_debug->TrackPointers()) {
+ PointerData::Add(pointer, bytes);
+ }
- if (g_debug->config().options() & RECORD_ALLOCS) {
- g_debug->record->AddEntry(memory_trace::Entry{.tid = gettid(),
- .type = memory_trace::MEMALIGN,
- .ptr = reinterpret_cast<uint64_t>(pointer),
- .size = bytes,
- .u.align = alignment,
- .start_ns = result.GetStartTimeNS(),
- .end_ns = result.GetEndTimeNS()});
- }
+ if (g_debug->config().options() & FILL_ON_ALLOC) {
+ size_t bytes = InternalMallocUsableSize(pointer);
+ size_t fill_bytes = g_debug->config().fill_on_alloc_bytes();
+ bytes = (bytes < fill_bytes) ? bytes : fill_bytes;
+ memset(pointer, g_debug->config().fill_alloc_value(), bytes);
+ }
+
+ if (entry != nullptr) {
+ *entry = memory_trace::Entry{.tid = gettid(),
+ .type = memory_trace::MEMALIGN,
+ .ptr = reinterpret_cast<uint64_t>(pointer),
+ .size = bytes,
+ .u.align = alignment,
+ .start_ns = result.GetStartTimeNS(),
+ .end_ns = result.GetEndTimeNS()};
}
return pointer;
@@ -797,17 +819,24 @@
ScopedDisableDebugCalls disable;
ScopedBacktraceSignalBlocker blocked;
+ memory_trace::Entry* entry = nullptr;
+ if (g_debug->config().options() & RECORD_ALLOCS) {
+ // In order to preserve the order of operations, reserve the entry before
+ // performing the operation.
+ entry = g_debug->record->ReserveEntry();
+ }
+
if (pointer == nullptr) {
TimedResult result = InternalMalloc(bytes);
pointer = result.getValue<void*>();
- if (g_debug->config().options() & RECORD_ALLOCS) {
- g_debug->record->AddEntry(memory_trace::Entry{.tid = gettid(),
- .type = memory_trace::REALLOC,
- .ptr = reinterpret_cast<uint64_t>(pointer),
- .size = bytes,
- .u.old_ptr = 0,
- .start_ns = result.GetStartTimeNS(),
- .end_ns = result.GetEndTimeNS()});
+ if (entry != nullptr) {
+ *entry = memory_trace::Entry{.tid = gettid(),
+ .type = memory_trace::REALLOC,
+ .ptr = reinterpret_cast<uint64_t>(pointer),
+ .size = bytes,
+ .u.old_ptr = 0,
+ .start_ns = result.GetStartTimeNS(),
+ .end_ns = result.GetEndTimeNS()};
}
return pointer;
}
@@ -819,15 +848,14 @@
if (bytes == 0) {
TimedResult result = InternalFree(pointer);
- if (g_debug->config().options() & RECORD_ALLOCS) {
- g_debug->record->AddEntry(
- memory_trace::Entry{.tid = gettid(),
- .type = memory_trace::REALLOC,
- .ptr = 0,
- .size = 0,
- .u.old_ptr = reinterpret_cast<uint64_t>(pointer),
- .start_ns = result.GetStartTimeNS(),
- .end_ns = result.GetEndTimeNS()});
+ if (entry != nullptr) {
+ *entry = memory_trace::Entry{.tid = gettid(),
+ .type = memory_trace::REALLOC,
+ .ptr = 0,
+ .size = 0,
+ .u.old_ptr = reinterpret_cast<uint64_t>(pointer),
+ .start_ns = result.GetStartTimeNS(),
+ .end_ns = result.GetEndTimeNS()};
}
return nullptr;
@@ -923,14 +951,14 @@
}
}
- if (g_debug->config().options() & RECORD_ALLOCS) {
- g_debug->record->AddEntry(memory_trace::Entry{.tid = gettid(),
- .type = memory_trace::REALLOC,
- .ptr = reinterpret_cast<uint64_t>(new_pointer),
- .size = bytes,
- .u.old_ptr = reinterpret_cast<uint64_t>(pointer),
- .start_ns = result.GetStartTimeNS(),
- .end_ns = result.GetEndTimeNS()});
+ if (entry != nullptr) {
+ *entry = memory_trace::Entry{.tid = gettid(),
+ .type = memory_trace::REALLOC,
+ .ptr = reinterpret_cast<uint64_t>(new_pointer),
+ .size = bytes,
+ .u.old_ptr = reinterpret_cast<uint64_t>(pointer),
+ .start_ns = result.GetStartTimeNS(),
+ .end_ns = result.GetEndTimeNS()};
}
return new_pointer;
@@ -969,6 +997,13 @@
return nullptr;
}
+ memory_trace::Entry* entry = nullptr;
+ if (g_debug->config().options() & RECORD_ALLOCS) {
+ // In order to preserve the order of operations, reserve the entry before
+ // performing the operation.
+ entry = g_debug->record->ReserveEntry();
+ }
+
void* pointer;
TimedResult result;
if (g_debug->HeaderEnabled()) {
@@ -985,14 +1020,14 @@
pointer = result.getValue<void*>();
}
- if (g_debug->config().options() & RECORD_ALLOCS) {
- g_debug->record->AddEntry(memory_trace::Entry{.tid = gettid(),
- .type = memory_trace::CALLOC,
- .ptr = reinterpret_cast<uint64_t>(pointer),
- .size = bytes,
- .u.n_elements = nmemb,
- .start_ns = result.GetStartTimeNS(),
- .end_ns = result.GetEndTimeNS()});
+ if (entry != nullptr) {
+ *entry = memory_trace::Entry{.tid = gettid(),
+ .type = memory_trace::CALLOC,
+ .ptr = reinterpret_cast<uint64_t>(pointer),
+ .size = bytes,
+ .u.n_elements = nmemb,
+ .start_ns = result.GetStartTimeNS(),
+ .end_ns = result.GetEndTimeNS()};
}
if (pointer != nullptr && g_debug->TrackPointers()) {