From 489c3a8b356a7a3ae235afec5e1e954f90301622 Mon Sep 17 00:00:00 2001 From: Yong Li Date: Thu, 19 Mar 2020 18:43:06 +0000 Subject: [PATCH] Fix memory leak of DexFile handle after release The DexFile handle is allocated from heap in OpenFromFd/OpenFromMemory. After releasing the unique_ptr, the DexFile handle itself is no longer managed by the smart pointer. However, the DexFile handle is not freed in the constructor of DexFileFromFile/DexFileFromMemory. This change uses get() method to get the DexFile pointer while allowing it to be managed by smart pointer so that it can be freed after method end. Added new unit tests to detect leaks. Bug: 151966190 Test: Unwinding can still retrieve dex frame information during crash. Test: Ran new unit tests before change and verified they fail, ran them Test: after the change and verified they don't fail. Signed-off-by: Yong Li Change-Id: I0627e1e255eb6644aba51e940c1a79ff78d568d7 --- libunwindstack/DexFile.cpp | 4 +-- libunwindstack/DexFile.h | 10 ++++--- libunwindstack/tests/DexFileTest.cpp | 45 ++++++++++++++++++++++++++++ 3 files changed, 53 insertions(+), 6 deletions(-) diff --git a/libunwindstack/DexFile.cpp b/libunwindstack/DexFile.cpp index dff7a8b3c..bf63abf1c 100644 --- a/libunwindstack/DexFile.cpp +++ b/libunwindstack/DexFile.cpp @@ -89,7 +89,7 @@ std::unique_ptr DexFileFromFile::Create(uint64_t dex_file_offse return nullptr; } - return std::unique_ptr(new DexFileFromFile(std::move(*art_dex_file.release()))); + return std::unique_ptr(new DexFileFromFile(art_dex_file)); } std::unique_ptr DexFileFromMemory::Create(uint64_t dex_file_offset_in_memory, @@ -108,7 +108,7 @@ std::unique_ptr DexFileFromMemory::Create(uint64_t dex_file_o if (art_dex_file != nullptr) { return std::unique_ptr( - new DexFileFromMemory(std::move(*art_dex_file.release()), std::move(backing_memory))); + new DexFileFromMemory(art_dex_file, std::move(backing_memory))); } if (!error_msg.empty()) { diff --git a/libunwindstack/DexFile.h b/libunwindstack/DexFile.h index ca658e688..4e8369f84 100644 --- a/libunwindstack/DexFile.h +++ b/libunwindstack/DexFile.h @@ -39,7 +39,8 @@ class DexFile : protected art_api::dex::DexFile { MapInfo* info); protected: - DexFile(art_api::dex::DexFile&& art_dex_file) : art_api::dex::DexFile(std::move(art_dex_file)) {} + DexFile(std::unique_ptr& art_dex_file) + : art_api::dex::DexFile(art_dex_file) {} }; class DexFileFromFile : public DexFile { @@ -48,7 +49,7 @@ class DexFileFromFile : public DexFile { const std::string& file); private: - DexFileFromFile(art_api::dex::DexFile&& art_dex_file) : DexFile(std::move(art_dex_file)) {} + DexFileFromFile(std::unique_ptr& art_dex_file) : DexFile(art_dex_file) {} }; class DexFileFromMemory : public DexFile { @@ -57,8 +58,9 @@ class DexFileFromMemory : public DexFile { Memory* memory, const std::string& name); private: - DexFileFromMemory(art_api::dex::DexFile&& art_dex_file, std::vector&& memory) - : DexFile(std::move(art_dex_file)), memory_(std::move(memory)) {} + DexFileFromMemory(std::unique_ptr& art_dex_file, + std::vector&& memory) + : DexFile(art_dex_file), memory_(std::move(memory)) {} std::vector memory_; }; diff --git a/libunwindstack/tests/DexFileTest.cpp b/libunwindstack/tests/DexFileTest.cpp index 1b54da6fb..dc935a36e 100644 --- a/libunwindstack/tests/DexFileTest.cpp +++ b/libunwindstack/tests/DexFileTest.cpp @@ -14,6 +14,7 @@ * limitations under the License. */ +#include #include #include #include @@ -72,6 +73,37 @@ TEST(DexFileTest, from_file_open_non_zero_offset) { EXPECT_TRUE(DexFileFromFile::Create(0x100, tf.path) != nullptr); } +static constexpr size_t kNumLeakLoops = 5000; +static constexpr size_t kMaxAllowedLeakBytes = 1024; + +static void CheckForLeak(size_t loop, size_t* first_allocated_bytes, size_t* last_allocated_bytes) { + size_t allocated_bytes = mallinfo().uordblks; + if (*first_allocated_bytes == 0) { + *first_allocated_bytes = allocated_bytes; + } else if (*last_allocated_bytes > *first_allocated_bytes) { + // Check that the total memory did not increase too much over the first loop. + ASSERT_LE(*last_allocated_bytes - *first_allocated_bytes, kMaxAllowedLeakBytes) + << "Failed in loop " << loop << " first_allocated_bytes " << *first_allocated_bytes + << " last_allocated_bytes " << *last_allocated_bytes; + } + *last_allocated_bytes = allocated_bytes; +} + +TEST(DexFileTest, from_file_no_leak) { + TemporaryFile tf; + ASSERT_TRUE(tf.fd != -1); + + ASSERT_EQ(sizeof(kDexData), + static_cast(TEMP_FAILURE_RETRY(write(tf.fd, kDexData, sizeof(kDexData))))); + + size_t first_allocated_bytes = 0; + size_t last_allocated_bytes = 0; + for (size_t i = 0; i < kNumLeakLoops; i++) { + EXPECT_TRUE(DexFileFromFile::Create(0, tf.path) != nullptr); + ASSERT_NO_FATAL_FAILURE(CheckForLeak(i, &first_allocated_bytes, &last_allocated_bytes)); + } +} + TEST(DexFileTest, from_memory_fail_too_small_for_header) { MemoryFake memory; @@ -96,6 +128,19 @@ TEST(DexFileTest, from_memory_open) { EXPECT_TRUE(DexFileFromMemory::Create(0x1000, &memory, "") != nullptr); } +TEST(DexFileTest, from_memory_no_leak) { + MemoryFake memory; + + memory.SetMemory(0x1000, kDexData, sizeof(kDexData)); + + size_t first_allocated_bytes = 0; + size_t last_allocated_bytes = 0; + for (size_t i = 0; i < kNumLeakLoops; i++) { + EXPECT_TRUE(DexFileFromMemory::Create(0x1000, &memory, "") != nullptr); + ASSERT_NO_FATAL_FAILURE(CheckForLeak(i, &first_allocated_bytes, &last_allocated_bytes)); + } +} + TEST(DexFileTest, create_using_file) { TemporaryFile tf; ASSERT_TRUE(tf.fd != -1);