From: Christopher Ferris Date: Wed, 16 Mar 2016 05:39:39 +0000 (-0700) Subject: Fix race in malloc debug option free_track. X-Git-Tag: android-x86-7.1-r1~101 X-Git-Url: http://git.osdn.net/view?a=commitdiff_plain;h=97fce67;p=android-x86%2Fbionic.git Fix race in malloc debug option free_track. The free track mechanism could fail if, at the same time a free occurs, another thread is trying to free and verify the same allocation. This doesn't work if the freed allocation is added to the list and we still do work on it. The fix is to only add to the free list when we are done with the allocation. Also fix a problem where the usable size is computed incorrectly because two of the arguments where reversed. In addition, add a check that the allocation being verified has the correct tag before trying to check the body of the allocation. Add a test to catch the original failure. Add a test for the tag being different. Bug: 27601650 (cherry picked from commit d0919623a2ef56107590eca9a9522a250fb8bd4a) Change-Id: Ie1aa4d9a829da9a96de9b8bd1cc8fc681e9cab15 --- diff --git a/libc/malloc_debug/FreeTrackData.cpp b/libc/malloc_debug/FreeTrackData.cpp index 3ac54bf3d..ed419812d 100644 --- a/libc/malloc_debug/FreeTrackData.cpp +++ b/libc/malloc_debug/FreeTrackData.cpp @@ -67,18 +67,25 @@ void FreeTrackData::VerifyAndFree(DebugData& debug, const Header* header, const void* pointer) { ScopedDisableDebugCalls disable; - const uint8_t* memory = reinterpret_cast(pointer); - size_t bytes = header->usable_size; - bytes = (bytes < debug.config().fill_on_free_bytes) ? bytes : debug.config().fill_on_free_bytes; - while (bytes > 0) { - size_t bytes_to_cmp = (bytes < cmp_mem_.size()) ? bytes : cmp_mem_.size(); - if (memcmp(memory, cmp_mem_.data(), bytes_to_cmp) != 0) { - LogFreeError(debug, header, reinterpret_cast(pointer)); - break; + if (header->tag != DEBUG_FREE_TAG) { + error_log(LOG_DIVIDER); + error_log("+++ ALLOCATION %p HAS CORRUPTED HEADER TAG 0x%x AFTER FREE", pointer, header->tag); + error_log(LOG_DIVIDER); + } else { + const uint8_t* memory = reinterpret_cast(pointer); + size_t bytes = header->usable_size; + bytes = (bytes < debug.config().fill_on_free_bytes) ? bytes : debug.config().fill_on_free_bytes; + while (bytes > 0) { + size_t bytes_to_cmp = (bytes < cmp_mem_.size()) ? bytes : cmp_mem_.size(); + if (memcmp(memory, cmp_mem_.data(), bytes_to_cmp) != 0) { + LogFreeError(debug, header, reinterpret_cast(pointer)); + break; + } + bytes -= bytes_to_cmp; + memory = &memory[bytes_to_cmp]; } - bytes -= bytes_to_cmp; - memory = &memory[bytes_to_cmp]; } + auto back_iter = backtraces_.find(header); if (back_iter != backtraces_.end()) { g_dispatch->free(reinterpret_cast(back_iter->second)); @@ -98,7 +105,6 @@ void FreeTrackData::Add(DebugData& debug, const Header* header) { list_.pop_back(); } - // Only log the free backtrace if we are using the free track feature. if (backtrace_num_frames_ > 0) { BacktraceHeader* back_header = reinterpret_cast( g_dispatch->malloc(sizeof(BacktraceHeader) + backtrace_num_frames_ * sizeof(uintptr_t))); diff --git a/libc/malloc_debug/malloc_debug.cpp b/libc/malloc_debug/malloc_debug.cpp index 568192d69..1ee76897d 100644 --- a/libc/malloc_debug/malloc_debug.cpp +++ b/libc/malloc_debug/malloc_debug.cpp @@ -112,6 +112,7 @@ static void InitAtfork() { ); }); } + static void LogTagError(const Header* header, const void* pointer, const char* name) { ScopedDisableDebugCalls disable; @@ -145,7 +146,7 @@ static void* InitHeader(Header* header, void* orig_pointer, size_t size) { return nullptr; } header->usable_size -= g_debug->pointer_offset() + - reinterpret_cast(orig_pointer) - reinterpret_cast(header); + reinterpret_cast(header) - reinterpret_cast(orig_pointer); if (g_debug->config().options & FRONT_GUARD) { uint8_t* guard = g_debug->GetFrontGuard(header); @@ -326,8 +327,9 @@ void debug_free(void* pointer) { void* free_pointer = pointer; size_t bytes; + Header* header; if (g_debug->need_header()) { - Header* header = g_debug->GetHeader(pointer); + header = g_debug->GetHeader(pointer); if (header->tag != DEBUG_TAG) { LogTagError(header, pointer, "free"); return; @@ -353,13 +355,6 @@ void debug_free(void* pointer) { } g_debug->track->Remove(header, backtrace_found); } - - if (g_debug->config().options & FREE_TRACK) { - g_debug->free_track->Add(*g_debug, header); - - // Do not free this pointer just yet. - free_pointer = nullptr; - } header->tag = DEBUG_FREE_TAG; bytes = header->usable_size; @@ -373,7 +368,16 @@ void debug_free(void* pointer) { memset(pointer, g_debug->config().fill_free_value, bytes); } - g_dispatch->free(free_pointer); + if (g_debug->config().options & FREE_TRACK) { + // Do not add the allocation until we are done modifying the pointer + // itself. This avoids a race if a lot of threads are all doing + // frees at the same time and we wind up trying to really free this + // pointer from another thread, while still trying to free it in + // this function. + g_debug->free_track->Add(*g_debug, header); + } else { + g_dispatch->free(free_pointer); + } } void* debug_memalign(size_t alignment, size_t bytes) { diff --git a/libc/malloc_debug/tests/malloc_debug_unit_tests.cpp b/libc/malloc_debug/tests/malloc_debug_unit_tests.cpp index 2503981b0..b6bf7bc43 100644 --- a/libc/malloc_debug/tests/malloc_debug_unit_tests.cpp +++ b/libc/malloc_debug/tests/malloc_debug_unit_tests.cpp @@ -24,6 +24,7 @@ #include #include +#include #include #include @@ -898,6 +899,53 @@ TEST_F(MallocDebugTest, free_track_use_after_free_call_free) { ASSERT_STREQ(expected_log.c_str(), getFakeLogPrint().c_str()); } +TEST_F(MallocDebugTest, free_track_header_tag_corrupted) { + Init("free_track=100 free_track_backtrace_num_frames=0"); + + uint8_t* pointer = reinterpret_cast(debug_malloc(100)); + ASSERT_TRUE(pointer != nullptr); + memset(pointer, 0, 100); + debug_free(pointer); + + pointer[-get_tag_offset()] = 0x00; + + ASSERT_STREQ("", getFakeLogBuf().c_str()); + ASSERT_STREQ("", getFakeLogPrint().c_str()); + + debug_finalize(); + initialized = false; + + ASSERT_STREQ("", getFakeLogBuf().c_str()); + std::string expected_log(DIVIDER); + expected_log += android::base::StringPrintf( + "6 malloc_debug +++ ALLOCATION %p HAS CORRUPTED HEADER TAG 0x1cc7dc00 AFTER FREE\n", + pointer); + expected_log += DIVIDER; + ASSERT_STREQ(expected_log.c_str(), getFakeLogPrint().c_str()); +} + +TEST_F(MallocDebugTest, free_track_multiple_thread) { + Init("free_track=10 free_track_backtrace_num_frames=0"); + + std::vector threads(1000); + for (size_t i = 0; i < threads.size(); i++) { + threads[i] = new std::thread([](){ + for (size_t j = 0; j < 100; j++) { + void* mem = debug_malloc(100); + write(0, mem, 0); + debug_free(mem); + } + }); + } + for (size_t i = 0; i < threads.size(); i++) { + threads[i]->join(); + delete threads[i]; + } + + ASSERT_STREQ("", getFakeLogBuf().c_str()); + ASSERT_STREQ("", getFakeLogPrint().c_str()); +} + TEST_F(MallocDebugTest, get_malloc_leak_info_invalid) { Init("fill");