Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 4ad53837 authored by Yong Li's avatar Yong Li Committed by Christopher Ferris
Browse files

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: default avatarYong Li <yongl0722@gmail.com>
Change-Id: I0627e1e255eb6644aba51e940c1a79ff78d568d7
(cherry picked from commit 489c3a8b)
parent baaf2a44

libunwindstack/DexFile.cpp

+2 −2
Original line number Diff line number Diff line
@@ -89,7 +89,7 @@ std::unique_ptr<DexFileFromFile> DexFileFromFile::Create(uint64_t dex_file_offse 
    return nullptr;

  }



  return std::unique_ptr<DexFileFromFile>(new DexFileFromFile(std::move(*art_dex_file.release())));

  return std::unique_ptr<DexFileFromFile>(new DexFileFromFile(art_dex_file));

}



std::unique_ptr<DexFileFromMemory> DexFileFromMemory::Create(uint64_t dex_file_offset_in_memory,
@@ -108,7 +108,7 @@ std::unique_ptr<DexFileFromMemory> DexFileFromMemory::Create(uint64_t dex_file_o 


    if (art_dex_file != nullptr) {

      return std::unique_ptr<DexFileFromMemory>(

          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()) {

libunwindstack/DexFile.h

+6 −4
Original line number Diff line number Diff line
@@ -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_api::dex::DexFile>& 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_api::dex::DexFile>& 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<uint8_t>&& memory)

      : DexFile(std::move(art_dex_file)), memory_(std::move(memory)) {}

  DexFileFromMemory(std::unique_ptr<art_api::dex::DexFile>& art_dex_file,

                    std::vector<uint8_t>&& memory)

      : DexFile(art_dex_file), memory_(std::move(memory)) {}



  std::vector<uint8_t> memory_;

};

libunwindstack/tests/DexFileTest.cpp

+45 −0
Original line number Diff line number Diff line
@@ -14,6 +14,7 @@ 
 * limitations under the License.

 */



#include <malloc.h>

#include <stdint.h>

#include <sys/types.h>

#include <unistd.h>
@@ -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<size_t>(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);