From bb4f2b440a5c970a3a522e94d69dae5fbaea81c6 Mon Sep 17 00:00:00 2001 From: Martin Stjernholm Date: Wed, 19 Dec 2018 14:28:33 +0000 Subject: [PATCH] Revert^2 "Use libdexfile external API in libunwindstack." This reverts commit cacf5bf6bca7e9806739a27589d8b6101c567c32. Reason for revert: Re-apply with proper fix for VNDK visibility on marlin and sailfish. Test: Manual repro of http://b/121110092#comment1 on reported branch Test: atest CtsRenderscriptTestCases Test: mmma system/core/{libunwindstack,libbacktrace}, run host gtests Test: Make image, flash, and reboot device. Test: Forrest cts/art/gce-all: https://android-build.googleplex.com/builds/forrest/run/L00300000240828791 Test: Forrest cts/bionic/gce-all: https://android-build.googleplex.com/builds/forrest/run/L05600000240682947 (shows 27/2958 failed, but it doesn't pass on Blackbox either: http://screen/xbjioEf6UgR) Test: Forrest cts/renderscript/gce-all: https://android-build.googleplex.com/builds/forrest/run/L66200000240680523 Bug: 119632407 Change-Id: I601aa97eac8127e30d753405f8bc1fc4ae7f849f --- debuggerd/Android.bp | 23 ++- libunwindstack/Android.bp | 14 +- libunwindstack/DexFile.cpp | 192 ++++-------------- libunwindstack/DexFile.h | 33 ++- libunwindstack/DexFiles.cpp | 13 +- libunwindstack/include/unwindstack/DexFiles.h | 2 +- libunwindstack/tests/DexFileTest.cpp | 66 ++---- 7 files changed, 115 insertions(+), 228 deletions(-) diff --git a/debuggerd/Android.bp b/debuggerd/Android.bp index d9fae5275..516748176 100644 --- a/debuggerd/Android.bp +++ b/debuggerd/Android.bp @@ -116,14 +116,21 @@ cc_library_static { "libdebuggerd", "libbacktrace", "libunwindstack", - "libdexfile", + "libdexfile", // libunwindstack dependency + "libdexfile_external", // libunwindstack dependency + "libdexfile_support", // libunwindstack dependency "liblzma", "libcutils", ], target: { recovery: { cflags: ["-DNO_LIBDEXFILE_SUPPORT"], - exclude_static_libs: ["libdexfile"], + exclude_static_libs: [ + "libartbase", + "libdexfile", + "libdexfile_external", + "libdexfile_support", + ], }, }, @@ -170,12 +177,22 @@ cc_library_static { static_libs: [ "libbacktrace", + "libdexfile_external", // libunwindstack dependency + "libdexfile_support", // libunwindstack dependency "libunwindstack", "liblzma", "libbase", "libcutils", "liblog", ], + target: { + recovery: { + exclude_static_libs: [ + "libdexfile_external", + "libdexfile_support", + ], + }, + }, } cc_test { @@ -216,6 +233,8 @@ cc_test { static_libs: [ "libdebuggerd", + "libdexfile_external", // libunwindstack dependency + "libdexfile_support", // libunwindstack dependency "libunwindstack", ], diff --git a/libunwindstack/Android.bp b/libunwindstack/Android.bp index 4e0470e38..89d4fc0e8 100644 --- a/libunwindstack/Android.bp +++ b/libunwindstack/Android.bp @@ -94,7 +94,10 @@ cc_library { "DexFile.cpp", "DexFiles.cpp", ], - exclude_shared_libs: ["libdexfile"], + exclude_shared_libs: [ + "libdexfile_external", + "libdexfile_support", + ], }, recovery: { cflags: ["-DNO_LIBDEXFILE_SUPPORT"], @@ -102,7 +105,10 @@ cc_library { "DexFile.cpp", "DexFiles.cpp", ], - exclude_shared_libs: ["libdexfile"], + exclude_shared_libs: [ + "libdexfile_external", + "libdexfile_support", + ], }, }, @@ -127,7 +133,8 @@ cc_library { shared_libs: [ "libbase", - "libdexfile", + "libdexfile_external", + "libdexfile_support", "liblog", "liblzma", ], @@ -215,6 +222,7 @@ cc_test { "liblzma", "libunwindstack", "libdexfile", + "libdexfile_support", ], static_libs: [ diff --git a/libunwindstack/DexFile.cpp b/libunwindstack/DexFile.cpp index 8ec560c3b..9b0b2328f 100644 --- a/libunwindstack/DexFile.cpp +++ b/libunwindstack/DexFile.cpp @@ -23,13 +23,7 @@ #include #include - -#include -#include -#include -#include -#include -#include +#include #include #include @@ -38,169 +32,71 @@ namespace unwindstack { -DexFile* DexFile::Create(uint64_t dex_file_offset_in_memory, Memory* memory, MapInfo* info) { +std::unique_ptr DexFile::Create(uint64_t dex_file_offset_in_memory, Memory* memory, + MapInfo* info) { if (!info->name.empty()) { - std::unique_ptr dex_file(new DexFileFromFile); - if (dex_file->Open(dex_file_offset_in_memory - info->start + info->offset, info->name)) { - return dex_file.release(); + std::unique_ptr dex_file = + DexFileFromFile::Create(dex_file_offset_in_memory - info->start + info->offset, info->name); + if (dex_file) { + return dex_file; } } - - std::unique_ptr dex_file(new DexFileFromMemory); - if (dex_file->Open(dex_file_offset_in_memory, memory)) { - return dex_file.release(); - } - return nullptr; -} - -DexFileFromFile::~DexFileFromFile() { - if (size_ != 0) { - munmap(mapped_memory_, size_); - } + return DexFileFromMemory::Create(dex_file_offset_in_memory, memory, info->name); } bool DexFile::GetMethodInformation(uint64_t dex_offset, std::string* method_name, uint64_t* method_offset) { - if (dex_file_ == nullptr) { + art_api::dex::MethodInfo method_info = GetMethodInfoForOffset(dex_offset); + if (method_info.offset == 0) { return false; } - - if (!dex_file_->IsInDataSection(dex_file_->Begin() + dex_offset)) { - return false; // The DEX offset is not within the bytecode of this dex file. - } - - if (dex_file_->IsCompactDexFile()) { - // The data section of compact dex files might be shared. - // Check the subrange unique to this compact dex. - const auto& cdex_header = dex_file_->AsCompactDexFile()->GetHeader(); - uint32_t begin = cdex_header.data_off_ + cdex_header.OwnedDataBegin(); - uint32_t end = cdex_header.data_off_ + cdex_header.OwnedDataEnd(); - if (dex_offset < begin || dex_offset >= end) { - return false; // The DEX offset is not within the bytecode of this dex file. - } - } - - // The method data is cached in a std::map indexed by method end offset and - // contains the start offset and the method member index. - // Only cache the method data as it is searched. Do not read the entire - // set of method data into the cache at once. - // This is done because many unwinds only find a single frame with dex file - // info, so reading the entire method data is wasteful. However, still cache - // the data so that anything doing multiple unwinds will have this data - // cached for future use. - - // First look in the method cache. - auto entry = method_cache_.upper_bound(dex_offset); - if (entry != method_cache_.end() && dex_offset >= entry->second.first) { - *method_name = dex_file_->PrettyMethod(entry->second.second, false); - *method_offset = dex_offset - entry->second.first; - return true; - } - - // Check the methods we haven't cached. - for (; class_def_index_ < dex_file_->NumClassDefs(); class_def_index_++) { - art::ClassAccessor accessor(*dex_file_, dex_file_->GetClassDef(class_def_index_)); - - for (const art::ClassAccessor::Method& method : accessor.GetMethods()) { - art::CodeItemInstructionAccessor code = method.GetInstructions(); - if (!code.HasCodeItem()) { - continue; - } - uint32_t offset = reinterpret_cast(code.Insns()) - dex_file_->Begin(); - uint32_t offset_end = offset + code.InsnsSizeInBytes(); - uint32_t member_index = method.GetIndex(); - method_cache_[offset_end] = std::make_pair(offset, member_index); - if (offset <= dex_offset && dex_offset < offset_end) { - *method_name = dex_file_->PrettyMethod(member_index, false); - *method_offset = dex_offset - offset; - return true; - } - } - } - return false; + *method_name = method_info.name; + *method_offset = dex_offset - method_info.offset; + return true; } -bool DexFileFromFile::Open(uint64_t dex_file_offset_in_file, const std::string& file) { +std::unique_ptr DexFileFromFile::Create(uint64_t dex_file_offset_in_file, + const std::string& file) { android::base::unique_fd fd(TEMP_FAILURE_RETRY(open(file.c_str(), O_RDONLY | O_CLOEXEC))); if (fd == -1) { - return false; - } - struct stat buf; - if (fstat(fd, &buf) == -1) { - return false; - } - uint64_t length; - if (buf.st_size < 0 || - __builtin_add_overflow(dex_file_offset_in_file, sizeof(art::DexFile::Header), &length) || - static_cast(buf.st_size) < length) { - return false; + return nullptr; } - mapped_memory_ = mmap(nullptr, buf.st_size, PROT_READ, MAP_PRIVATE, fd, 0); - if (mapped_memory_ == MAP_FAILED) { - return false; - } - size_ = buf.st_size; - - uint8_t* memory = reinterpret_cast(mapped_memory_); - - art::DexFile::Header* header = - reinterpret_cast(&memory[dex_file_offset_in_file]); - if (!art::StandardDexFile::IsMagicValid(header->magic_) && - !art::CompactDexFile::IsMagicValid(header->magic_)) { - return false; - } - - if (__builtin_add_overflow(dex_file_offset_in_file, header->file_size_, &length) || - static_cast(buf.st_size) < length) { - return false; - } - - art::DexFileLoader loader; std::string error_msg; - auto dex = loader.Open(&memory[dex_file_offset_in_file], header->file_size_, "", 0, nullptr, - false, false, &error_msg); - dex_file_.reset(dex.release()); - return dex_file_ != nullptr; + std::unique_ptr art_dex_file = + OpenFromFd(fd, dex_file_offset_in_file, file, &error_msg); + if (art_dex_file == nullptr) { + return nullptr; + } + + return std::unique_ptr(new DexFileFromFile(std::move(*art_dex_file.release()))); } -bool DexFileFromMemory::Open(uint64_t dex_file_offset_in_memory, Memory* memory) { - memory_.resize(sizeof(art::DexFile::Header)); - if (!memory->ReadFully(dex_file_offset_in_memory, memory_.data(), memory_.size())) { - return false; - } +std::unique_ptr DexFileFromMemory::Create(uint64_t dex_file_offset_in_memory, + Memory* memory, + const std::string& name) { + std::vector backing_memory; - art::DexFile::Header* header = reinterpret_cast(memory_.data()); - uint32_t file_size = header->file_size_; - if (art::CompactDexFile::IsMagicValid(header->magic_)) { - // Compact dex file store data section separately so that it can be shared. - // Therefore we need to extend the read memory range to include it. - // TODO: This might be wasteful as we might read data in between as well. - // In practice, this should be fine, as such sharing only happens on disk. - uint32_t computed_file_size; - if (__builtin_add_overflow(header->data_off_, header->data_size_, &computed_file_size)) { - return false; + for (size_t size = 0;;) { + std::string error_msg; + std::unique_ptr art_dex_file = + OpenFromMemory(backing_memory.data(), &size, name, &error_msg); + + if (art_dex_file != nullptr) { + return std::unique_ptr( + new DexFileFromMemory(std::move(*art_dex_file.release()), std::move(backing_memory))); } - if (computed_file_size > file_size) { - file_size = computed_file_size; + + if (!error_msg.empty()) { + return nullptr; + } + + backing_memory.resize(size); + if (!memory->ReadFully(dex_file_offset_in_memory, backing_memory.data(), + backing_memory.size())) { + return nullptr; } - } else if (!art::StandardDexFile::IsMagicValid(header->magic_)) { - return false; } - - memory_.resize(file_size); - if (!memory->ReadFully(dex_file_offset_in_memory, memory_.data(), memory_.size())) { - return false; - } - - header = reinterpret_cast(memory_.data()); - - art::DexFileLoader loader; - std::string error_msg; - auto dex = - loader.Open(memory_.data(), header->file_size_, "", 0, nullptr, false, false, &error_msg); - dex_file_.reset(dex.release()); - return dex_file_ != nullptr; } } // namespace unwindstack diff --git a/libunwindstack/DexFile.h b/libunwindstack/DexFile.h index c123158ef..5797deec6 100644 --- a/libunwindstack/DexFile.h +++ b/libunwindstack/DexFile.h @@ -25,48 +25,41 @@ #include #include -#include +#include namespace unwindstack { -class DexFile { +class DexFile : protected art_api::dex::DexFile { public: - DexFile() = default; virtual ~DexFile() = default; bool GetMethodInformation(uint64_t dex_offset, std::string* method_name, uint64_t* method_offset); - static DexFile* Create(uint64_t dex_file_offset_in_memory, Memory* memory, MapInfo* info); + static std::unique_ptr Create(uint64_t dex_file_offset_in_memory, Memory* memory, + MapInfo* info); protected: - void Init(); - - std::unique_ptr dex_file_; - std::map> method_cache_; // dex offset to method index. - - uint32_t class_def_index_ = 0; + DexFile(art_api::dex::DexFile&& art_dex_file) : art_api::dex::DexFile(std::move(art_dex_file)) {} }; class DexFileFromFile : public DexFile { public: - DexFileFromFile() = default; - virtual ~DexFileFromFile(); - - bool Open(uint64_t dex_file_offset_in_file, const std::string& name); + static std::unique_ptr Create(uint64_t dex_file_offset_in_file, + const std::string& file); private: - void* mapped_memory_ = nullptr; - size_t size_ = 0; + DexFileFromFile(art_api::dex::DexFile&& art_dex_file) : DexFile(std::move(art_dex_file)) {} }; class DexFileFromMemory : public DexFile { public: - DexFileFromMemory() = default; - virtual ~DexFileFromMemory() = default; - - bool Open(uint64_t dex_file_offset_in_memory, Memory* memory); + static std::unique_ptr Create(uint64_t dex_file_offset_in_memory, + 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)) {} + std::vector memory_; }; diff --git a/libunwindstack/DexFiles.cpp b/libunwindstack/DexFiles.cpp index 451a0b90d..63a77e50f 100644 --- a/libunwindstack/DexFiles.cpp +++ b/libunwindstack/DexFiles.cpp @@ -48,11 +48,7 @@ DexFiles::DexFiles(std::shared_ptr& memory) : Global(memory) {} DexFiles::DexFiles(std::shared_ptr& memory, std::vector& search_libs) : Global(memory, search_libs) {} -DexFiles::~DexFiles() { - for (auto& entry : files_) { - delete entry.second; - } -} +DexFiles::~DexFiles() {} void DexFiles::ProcessArch() { switch (arch()) { @@ -137,10 +133,11 @@ DexFile* DexFiles::GetDexFile(uint64_t dex_file_offset, MapInfo* info) { DexFile* dex_file; auto entry = files_.find(dex_file_offset); if (entry == files_.end()) { - dex_file = DexFile::Create(dex_file_offset, memory_.get(), info); - files_[dex_file_offset] = dex_file; + std::unique_ptr new_dex_file = DexFile::Create(dex_file_offset, memory_.get(), info); + dex_file = new_dex_file.get(); + files_[dex_file_offset] = std::move(new_dex_file); } else { - dex_file = entry->second; + dex_file = entry->second.get(); } return dex_file; } diff --git a/libunwindstack/include/unwindstack/DexFiles.h b/libunwindstack/include/unwindstack/DexFiles.h index c202a334d..033617362 100644 --- a/libunwindstack/include/unwindstack/DexFiles.h +++ b/libunwindstack/include/unwindstack/DexFiles.h @@ -66,7 +66,7 @@ class DexFiles : public Global { std::mutex lock_; bool initialized_ = false; - std::unordered_map files_; + std::unordered_map> files_; uint64_t entry_addr_ = 0; uint64_t (DexFiles::*read_entry_ptr_func_)(uint64_t) = nullptr; diff --git a/libunwindstack/tests/DexFileTest.cpp b/libunwindstack/tests/DexFileTest.cpp index 95d217631..21ca47b52 100644 --- a/libunwindstack/tests/DexFileTest.cpp +++ b/libunwindstack/tests/DexFileTest.cpp @@ -21,44 +21,37 @@ #include #include - +#include +#include #include #include -#include -#include - -#include - #include "DexFile.h" - #include "DexFileData.h" #include "MemoryFake.h" namespace unwindstack { TEST(DexFileTest, from_file_open_non_exist) { - DexFileFromFile dex_file; - ASSERT_FALSE(dex_file.Open(0, "/file/does/not/exist")); + EXPECT_TRUE(DexFileFromFile::Create(0, "/file/does/not/exist") == nullptr); } TEST(DexFileTest, from_file_open_too_small) { TemporaryFile tf; ASSERT_TRUE(tf.fd != -1); - ASSERT_EQ(sizeof(art::DexFile::Header) - 2, + ASSERT_EQ(sizeof(art::DexFile::Header) - 1, static_cast( - TEMP_FAILURE_RETRY(write(tf.fd, kDexData, sizeof(art::DexFile::Header)) - 2))); + TEMP_FAILURE_RETRY(write(tf.fd, kDexData, sizeof(art::DexFile::Header) - 1)))); // Header too small. - DexFileFromFile dex_file; - ASSERT_FALSE(dex_file.Open(0, tf.path)); + EXPECT_TRUE(DexFileFromFile::Create(0, tf.path) == nullptr); // Header correct, file too small. ASSERT_EQ(0, lseek(tf.fd, 0, SEEK_SET)); ASSERT_EQ(sizeof(art::DexFile::Header), static_cast(TEMP_FAILURE_RETRY(write( tf.fd, kDexData, sizeof(art::DexFile::Header))))); - ASSERT_FALSE(dex_file.Open(0, tf.path)); + EXPECT_TRUE(DexFileFromFile::Create(0, tf.path) == nullptr); } TEST(DexFileTest, from_file_open) { @@ -68,8 +61,7 @@ TEST(DexFileTest, from_file_open) { ASSERT_EQ(sizeof(kDexData), static_cast(TEMP_FAILURE_RETRY(write(tf.fd, kDexData, sizeof(kDexData))))); - DexFileFromFile dex_file; - ASSERT_TRUE(dex_file.Open(0, tf.path)); + EXPECT_TRUE(DexFileFromFile::Create(0, tf.path) != nullptr); } TEST(DexFileTest, from_file_open_non_zero_offset) { @@ -80,35 +72,31 @@ TEST(DexFileTest, from_file_open_non_zero_offset) { ASSERT_EQ(sizeof(kDexData), static_cast(TEMP_FAILURE_RETRY(write(tf.fd, kDexData, sizeof(kDexData))))); - DexFileFromFile dex_file; - ASSERT_TRUE(dex_file.Open(0x100, tf.path)); + EXPECT_TRUE(DexFileFromFile::Create(0x100, tf.path) != nullptr); } TEST(DexFileTest, from_memory_fail_too_small_for_header) { MemoryFake memory; memory.SetMemory(0x1000, kDexData, sizeof(art::DexFile::Header) - 1); - DexFileFromMemory dex_file; - ASSERT_FALSE(dex_file.Open(0x1000, &memory)); + EXPECT_TRUE(DexFileFromMemory::Create(0x1000, &memory, "") == nullptr); } TEST(DexFileTest, from_memory_fail_too_small_for_data) { MemoryFake memory; memory.SetMemory(0x1000, kDexData, sizeof(kDexData) - 2); - DexFileFromMemory dex_file; - ASSERT_FALSE(dex_file.Open(0x1000, &memory)); + EXPECT_TRUE(DexFileFromMemory::Create(0x1000, &memory, "") == nullptr); } TEST(DexFileTest, from_memory_open) { MemoryFake memory; memory.SetMemory(0x1000, kDexData, sizeof(kDexData)); - DexFileFromMemory dex_file; - ASSERT_TRUE(dex_file.Open(0x1000, &memory)); + EXPECT_TRUE(DexFileFromMemory::Create(0x1000, &memory, "") != nullptr); } TEST(DexFileTest, create_using_file) { @@ -121,8 +109,7 @@ TEST(DexFileTest, create_using_file) { MemoryFake memory; MapInfo info(nullptr, 0, 0x10000, 0, 0x5, tf.path); - std::unique_ptr dex_file(DexFile::Create(0x500, &memory, &info)); - ASSERT_TRUE(dex_file != nullptr); + EXPECT_TRUE(DexFile::Create(0x500, &memory, &info) != nullptr); } TEST(DexFileTest, create_using_file_non_zero_start) { @@ -135,8 +122,7 @@ TEST(DexFileTest, create_using_file_non_zero_start) { MemoryFake memory; MapInfo info(nullptr, 0x100, 0x10000, 0, 0x5, tf.path); - std::unique_ptr dex_file(DexFile::Create(0x600, &memory, &info)); - ASSERT_TRUE(dex_file != nullptr); + EXPECT_TRUE(DexFile::Create(0x600, &memory, &info) != nullptr); } TEST(DexFileTest, create_using_file_non_zero_offset) { @@ -149,24 +135,21 @@ TEST(DexFileTest, create_using_file_non_zero_offset) { MemoryFake memory; MapInfo info(nullptr, 0x100, 0x10000, 0x200, 0x5, tf.path); - std::unique_ptr dex_file(DexFile::Create(0x400, &memory, &info)); - ASSERT_TRUE(dex_file != nullptr); + EXPECT_TRUE(DexFile::Create(0x400, &memory, &info) != nullptr); } TEST(DexFileTest, create_using_memory_empty_file) { MemoryFake memory; memory.SetMemory(0x4000, kDexData, sizeof(kDexData)); MapInfo info(nullptr, 0x100, 0x10000, 0x200, 0x5, ""); - std::unique_ptr dex_file(DexFile::Create(0x4000, &memory, &info)); - ASSERT_TRUE(dex_file != nullptr); + EXPECT_TRUE(DexFile::Create(0x4000, &memory, &info) != nullptr); } TEST(DexFileTest, create_using_memory_file_does_not_exist) { MemoryFake memory; memory.SetMemory(0x4000, kDexData, sizeof(kDexData)); MapInfo info(nullptr, 0x100, 0x10000, 0x200, 0x5, "/does/not/exist"); - std::unique_ptr dex_file(DexFile::Create(0x4000, &memory, &info)); - ASSERT_TRUE(dex_file != nullptr); + EXPECT_TRUE(DexFile::Create(0x4000, &memory, &info) != nullptr); } TEST(DexFileTest, create_using_memory_file_is_malformed) { @@ -179,22 +162,13 @@ TEST(DexFileTest, create_using_memory_file_is_malformed) { MemoryFake memory; memory.SetMemory(0x4000, kDexData, sizeof(kDexData)); MapInfo info(nullptr, 0x4000, 0x10000, 0x200, 0x5, "/does/not/exist"); - std::unique_ptr dex_file(DexFile::Create(0x4000, &memory, &info)); + std::unique_ptr dex_file = DexFile::Create(0x4000, &memory, &info); ASSERT_TRUE(dex_file != nullptr); // Check it came from memory by clearing memory and verifying it fails. memory.Clear(); - dex_file.reset(DexFile::Create(0x4000, &memory, &info)); - ASSERT_TRUE(dex_file == nullptr); -} - -TEST(DexFileTest, get_method_not_opened) { - std::string method("something"); - uint64_t method_offset = 100; - DexFile dex_file; - dex_file.GetMethodInformation(0x100, &method, &method_offset); - EXPECT_EQ("something", method); - EXPECT_EQ(100U, method_offset); + dex_file = DexFile::Create(0x4000, &memory, &info); + EXPECT_TRUE(dex_file == nullptr); } TEST(DexFileTest, get_method) {