From 426b00a29b61a3ac0135373e8c5140957bd867da Mon Sep 17 00:00:00 2001 From: Christopher Ferris Date: Thu, 9 Mar 2017 13:47:37 -0800 Subject: [PATCH] Fix the way malloc debug returns info. When I rewrote malloc debug, I accidentally thought that each value returned in the info buffer contained the number of backtrace frames in the backtrace. This was incorrect, it should have been the total number of allocations with the same backtrace. This is a temporary fix that sets that value to 1. The better fix is to de-dupe backtraces and then return all allocations of the same size with the same backtrace. I updated the documents to describe this. Bug: 31854476 Test: Unit tests pass. Change-Id: Idf9efaa3d363923b5d7543d90dc7c65a0ed553d9 --- libc/malloc_debug/README_api.md | 10 +++++++--- libc/malloc_debug/TrackData.cpp | 3 ++- libc/malloc_debug/tests/malloc_debug_unit_tests.cpp | 16 ++++++++-------- 3 files changed, 17 insertions(+), 12 deletions(-) diff --git a/libc/malloc_debug/README_api.md b/libc/malloc_debug/README_api.md index 9d07cb3a3..b2c46d85c 100644 --- a/libc/malloc_debug/README_api.md +++ b/libc/malloc_debug/README_api.md @@ -26,7 +26,7 @@ In order to free the buffer allocated by the function, call: ### Format of info Buffer size_t size_of_original_allocation - size_t num_backtrace_frames + size_t num_allocations uintptr_t pc1 uintptr_t pc2 uintptr_t pc3 @@ -38,8 +38,12 @@ The number of *uintptr\_t* values is determined by the value *backtrace\_size* as returned by the original call to *get\_malloc\_leak\_info*. This value is not variable, it is the same for all the returned data. The value -*num\_backtrace\_frames* contains the real number of frames found. The -extra frames are set to zero. Each *uintptr\_t* is a pc of the callstack. +*num\_allocations* contains the total number of allocations with the same +backtrace and size as this allocation. On Android Nougat, this value was +incorrectly set to the number of frames in the backtrace. +Each *uintptr\_t* is a pc of the callstack. If the total number +of backtrace entries is less than *backtrace\_size*, the rest of the +entries are zero. The calls from within the malloc debug library are automatically removed. For 32 bit systems, *size\_t* and *uintptr\_t* are both 4 byte values. diff --git a/libc/malloc_debug/TrackData.cpp b/libc/malloc_debug/TrackData.cpp index 18f428b77..d4064f8d7 100644 --- a/libc/malloc_debug/TrackData.cpp +++ b/libc/malloc_debug/TrackData.cpp @@ -123,11 +123,12 @@ void TrackData::GetInfo(uint8_t** info, size_t* overall_size, size_t* info_size, GetList(&list); uint8_t* data = *info; + size_t num_allocations = 1; for (const auto& header : list) { BacktraceHeader* back_header = debug_->GetAllocBacktrace(header); if (back_header->num_frames > 0) { memcpy(data, &header->size, sizeof(size_t)); - memcpy(&data[sizeof(size_t)], &back_header->num_frames, sizeof(size_t)); + memcpy(&data[sizeof(size_t)], &num_allocations, sizeof(size_t)); memcpy(&data[2 * sizeof(size_t)], &back_header->frames[0], back_header->num_frames * sizeof(uintptr_t)); diff --git a/libc/malloc_debug/tests/malloc_debug_unit_tests.cpp b/libc/malloc_debug/tests/malloc_debug_unit_tests.cpp index 1b08a39dc..219c21ebb 100644 --- a/libc/malloc_debug/tests/malloc_debug_unit_tests.cpp +++ b/libc/malloc_debug/tests/malloc_debug_unit_tests.cpp @@ -999,7 +999,7 @@ TEST_F(MallocDebugTest, get_malloc_leak_info_not_enabled) { struct InfoEntry { size_t size; - size_t num_frames; + size_t num_allocations; uintptr_t frames[0]; } __attribute__((packed)); @@ -1033,7 +1033,7 @@ TEST_F(MallocDebugTest, get_malloc_leak_info_single) { InfoEntry* entry = reinterpret_cast(expected_info.data()); entry->size = 200; - entry->num_frames = 3; + entry->num_allocations = 1; entry->frames[0] = 0xf; entry->frames[1] = 0xe; entry->frames[2] = 0xd; @@ -1082,7 +1082,7 @@ TEST_F(MallocDebugTest, get_malloc_leak_info_multi) { // These values will be in the reverse order that we create. entry2->size = 500; - entry2->num_frames = 4; + entry2->num_allocations = 1; entry2->frames[0] = 0xf; entry2->frames[1] = 0xe; entry2->frames[2] = 0xd; @@ -1097,7 +1097,7 @@ TEST_F(MallocDebugTest, get_malloc_leak_info_multi) { memset(pointers[0], 0, entry2->size); entry1->size = 4100; - entry1->num_frames = 16; + entry1->num_allocations = 1; for (size_t i = 0; i < 16; i++) { entry1->frames[i] = 0xbc000 + i; } @@ -1112,7 +1112,7 @@ TEST_F(MallocDebugTest, get_malloc_leak_info_multi) { memset(pointers[1], 0, entry1->size); entry0->size = 9000; - entry0->num_frames = 1; + entry0->num_allocations = 1; entry0->frames[0] = 0x104; backtrace_fake_add(std::vector {0x104}); @@ -1159,7 +1159,7 @@ TEST_F(MallocDebugTest, get_malloc_leak_info_multi_skip_empty_backtrace) { // These values will be in the reverse order that we create. entry1->size = 500; - entry1->num_frames = 4; + entry1->num_allocations = 1; entry1->frames[0] = 0xf; entry1->frames[1] = 0xe; entry1->frames[2] = 0xd; @@ -1174,7 +1174,7 @@ TEST_F(MallocDebugTest, get_malloc_leak_info_multi_skip_empty_backtrace) { memset(pointers[0], 0, entry1->size); entry0->size = 4100; - entry0->num_frames = 16; + entry0->num_allocations = 1; for (size_t i = 0; i < 16; i++) { entry0->frames[i] = 0xbc000 + i; } @@ -1373,7 +1373,7 @@ static void VerifyZygoteSet(size_t memory_bytes) { memset(expected_info.data(), 0, expected_info_size); InfoEntry* entry = reinterpret_cast(expected_info.data()); entry->size = memory_bytes | (1U << 31); - entry->num_frames = 1; + entry->num_allocations = 1; entry->frames[0] = 0x1; uint8_t* info; -- 2.11.0