From ff88fb0d3adbc67a4f94f5ef7e2b5bcc7a96c8f3 Mon Sep 17 00:00:00 2001 From: Christopher Ferris Date: Mon, 4 Nov 2019 18:40:00 -0800 Subject: Fix allocations escaping malloc debug. When using a FILE object for some malloc debug functions, calling fprintf will trigger an allocation to be put in the object. The problem is that these allocations were not allocated by the malloc debug wrapper and they get freed during the fclose as if they are malloc debug allocation. In most cases, the code will detect the bad pointer and leak the memory, but it might also cause a crash. The fix is to avoid using fprintf so that no allocations are made in the object that survive and need to be freed in the fclose call. Change the MallocXmlElem.h to use a file decsriptor not a FILE object. Add new unit and system tests to detect this case. Bug: 143742907 Test: Ran unit and system tests. Test: Ran bionic unit tests. Change-Id: I524392de822a29483aa5be8f14c680e70033eba2 --- libc/malloc_debug/malloc_debug.cpp | 45 ++++++++++++++++++++++++-------------- 1 file changed, 28 insertions(+), 17 deletions(-) (limited to 'libc/malloc_debug/malloc_debug.cpp') diff --git a/libc/malloc_debug/malloc_debug.cpp b/libc/malloc_debug/malloc_debug.cpp index c030d5429..3c0e630cf 100644 --- a/libc/malloc_debug/malloc_debug.cpp +++ b/libc/malloc_debug/malloc_debug.cpp @@ -785,16 +785,23 @@ int debug_malloc_info(int options, FILE* fp) { if (DebugCallsDisabled() || !g_debug->TrackPointers()) { return g_dispatch->malloc_info(options, fp); } + + // Make sure any pending output is written to the file. + fflush(fp); + ScopedConcurrentLock lock; ScopedDisableDebugCalls disable; - MallocXmlElem root(fp, "malloc", "version=\"debug-malloc-1\""); + // Avoid any issues where allocations are made that will be freed + // in the fclose. + int fd = fileno(fp); + MallocXmlElem root(fd, "malloc", "version=\"debug-malloc-1\""); std::vector list; PointerData::GetAllocList(&list); size_t alloc_num = 0; for (size_t i = 0; i < list.size(); i++) { - MallocXmlElem alloc(fp, "allocation", "nr=\"%zu\"", alloc_num); + MallocXmlElem alloc(fd, "allocation", "nr=\"%zu\"", alloc_num); size_t total = 1; size_t size = list[i].size; @@ -802,8 +809,8 @@ int debug_malloc_info(int options, FILE* fp) { i++; total++; } - MallocXmlElem(fp, "size").Contents("%zu", list[i].size); - MallocXmlElem(fp, "total").Contents("%zu", total); + MallocXmlElem(fd, "size").Contents("%zu", list[i].size); + MallocXmlElem(fd, "total").Contents("%zu", total); alloc_num++; } return 0; @@ -905,25 +912,28 @@ void* debug_valloc(size_t size) { static std::mutex g_dump_lock; -static void write_dump(FILE* fp) { - fprintf(fp, "Android Native Heap Dump v1.2\n\n"); +static void write_dump(int fd) { + dprintf(fd, "Android Native Heap Dump v1.2\n\n"); std::string fingerprint = android::base::GetProperty("ro.build.fingerprint", "unknown"); - fprintf(fp, "Build fingerprint: '%s'\n\n", fingerprint.c_str()); + dprintf(fd, "Build fingerprint: '%s'\n\n", fingerprint.c_str()); - PointerData::DumpLiveToFile(fp); + PointerData::DumpLiveToFile(fd); - fprintf(fp, "MAPS\n"); + dprintf(fd, "MAPS\n"); std::string content; if (!android::base::ReadFileToString("/proc/self/maps", &content)) { - fprintf(fp, "Could not open /proc/self/maps\n"); + dprintf(fd, "Could not open /proc/self/maps\n"); } else { - fprintf(fp, "%s", content.c_str()); + dprintf(fd, "%s", content.c_str()); } - fprintf(fp, "END\n"); + dprintf(fd, "END\n"); } bool debug_write_malloc_leak_info(FILE* fp) { + // Make sure any pending output is written to the file. + fflush(fp); + ScopedConcurrentLock lock; ScopedDisableDebugCalls disable; @@ -933,7 +943,8 @@ bool debug_write_malloc_leak_info(FILE* fp) { return false; } - write_dump(fp); + write_dump(fileno(fp)); + return true; } @@ -943,13 +954,13 @@ void debug_dump_heap(const char* file_name) { std::lock_guard guard(g_dump_lock); - FILE* fp = fopen(file_name, "w+e"); - if (fp == nullptr) { + int fd = open(file_name, O_RDWR | O_CREAT | O_NOFOLLOW | O_TRUNC | O_CLOEXEC, 0644); + if (fd == -1) { error_log("Unable to create file: %s", file_name); return; } error_log("Dumping to file: %s\n", file_name); - write_dump(fp); - fclose(fp); + write_dump(fd); + close(fd); } -- cgit v1.2.3 From 6f517cd7a1142191fde2201b6c529758e7ff6895 Mon Sep 17 00:00:00 2001 From: Christopher Ferris Date: Fri, 8 Nov 2019 11:28:38 -0800 Subject: Rename iterate to malloc_iterate internally. I have no idea why I used the iterate name internally which is completely unlike every other function name. Change this to match everyone else so that it's now malloc_iterate everywhere. This is probably the last chance to change this before mainline modules begin, so make everything consistent. Test: Compiles, unit tests passes. Change-Id: I56d293377fa0fe1a3dc3dd85d6432f877cc2003c --- libc/malloc_debug/malloc_debug.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) (limited to 'libc/malloc_debug/malloc_debug.cpp') diff --git a/libc/malloc_debug/malloc_debug.cpp b/libc/malloc_debug/malloc_debug.cpp index 3c0e630cf..c0765a983 100644 --- a/libc/malloc_debug/malloc_debug.cpp +++ b/libc/malloc_debug/malloc_debug.cpp @@ -91,8 +91,8 @@ struct mallinfo debug_mallinfo(); int debug_mallopt(int param, int value); int debug_malloc_info(int options, FILE* fp); int debug_posix_memalign(void** memptr, size_t alignment, size_t size); -int debug_iterate(uintptr_t base, size_t size, - void (*callback)(uintptr_t base, size_t size, void* arg), void* arg); +int debug_malloc_iterate(uintptr_t base, size_t size, + void (*callback)(uintptr_t base, size_t size, void* arg), void* arg); void debug_malloc_disable(); void debug_malloc_enable(); @@ -841,7 +841,7 @@ int debug_posix_memalign(void** memptr, size_t alignment, size_t size) { return (*memptr != nullptr) ? 0 : ENOMEM; } -int debug_iterate(uintptr_t base, size_t size, void (*callback)(uintptr_t, size_t, void*), +int debug_malloc_iterate(uintptr_t base, size_t size, void (*callback)(uintptr_t, size_t, void*), void* arg) { ScopedConcurrentLock lock; if (g_debug->TrackPointers()) { @@ -854,7 +854,7 @@ int debug_iterate(uintptr_t base, size_t size, void (*callback)(uintptr_t, size_ // An option that adds a header will add pointer tracking, so no need to // check if headers are enabled. - return g_dispatch->iterate(base, size, callback, arg); + return g_dispatch->malloc_iterate(base, size, callback, arg); } void debug_malloc_disable() { -- cgit v1.2.3