diff options
author | Florian Mayer <fmayer@google.com> | 2019-02-27 18:00:37 +0000 |
---|---|---|
committer | Florian Mayer <fmayer@google.com> | 2019-03-05 13:05:36 +0000 |
commit | d0ffad3d45f6ebff74948389433b0edec2ddc8b4 (patch) | |
tree | c7ecab5fa65f85da694c7fd88624697d82bb2668 | |
parent | 89fe3cedf6b0f3c9736317b53c33f6b9983d5e1c (diff) | |
download | unwinding-d0ffad3d45f6ebff74948389433b0edec2ddc8b4.tar.gz |
Fix copy / move behaviour of Maps object.
Currently, moving or copying a Maps object leads to double free of MapInfo.
Even moving a Maps object did not prevent this, as after a move
the object only has to be in an "unspecified but valid state", which can
be the original state for a vector of raw pointers (but not for a vector
of unique_ptrs).
Changing to unique_ptrs is the most failsafe way to make sure we never
accidentally destruct MapInfo.
Test: atest libuwindstack_test
Failed LocalUnwinderTest#unwind_after_dlopen which also fails at master.
Change-Id: Id1c9739b334da5c1ba532fd55366e115940a66d3
-rw-r--r-- | libbacktrace/UnwindStackMap.cpp | 2 | ||||
-rw-r--r-- | libunwindstack/Global.cpp | 4 | ||||
-rw-r--r-- | libunwindstack/Maps.cpp | 55 | ||||
-rw-r--r-- | libunwindstack/benchmarks/unwind_benchmarks.cpp | 4 | ||||
-rw-r--r-- | libunwindstack/include/unwindstack/Maps.h | 23 | ||||
-rw-r--r-- | libunwindstack/tests/MapsTest.cpp | 20 | ||||
-rw-r--r-- | libunwindstack/tests/UnwinderTest.cpp | 8 |
7 files changed, 66 insertions, 50 deletions
diff --git a/libbacktrace/UnwindStackMap.cpp b/libbacktrace/UnwindStackMap.cpp index 9d15af2..4518891 100644 --- a/libbacktrace/UnwindStackMap.cpp +++ b/libbacktrace/UnwindStackMap.cpp @@ -55,7 +55,7 @@ bool UnwindStackMap::Build() { } // Iterate through the maps and fill in the backtrace_map_t structure. - for (auto* map_info : *stack_maps_) { + for (const auto& map_info : *stack_maps_) { backtrace_map_t map; map.start = map_info->start; map.end = map_info->end; diff --git a/libunwindstack/Global.cpp b/libunwindstack/Global.cpp index fdfd705..a20be00 100644 --- a/libunwindstack/Global.cpp +++ b/libunwindstack/Global.cpp @@ -77,7 +77,7 @@ void Global::FindAndReadVariable(Maps* maps, const char* var_str) { // f0000-f2000 0 r-- /system/lib/libc.so // f2000-f3000 2000 rw- /system/lib/libc.so MapInfo* map_start = nullptr; - for (MapInfo* info : *maps) { + for (const auto& info : *maps) { if (map_start != nullptr) { if (map_start->name == info->name) { if (info->offset != 0 && @@ -96,7 +96,7 @@ void Global::FindAndReadVariable(Maps* maps, const char* var_str) { } if (map_start == nullptr && (info->flags & PROT_READ) && info->offset == 0 && !info->name.empty()) { - map_start = info; + map_start = info.get(); } } } diff --git a/libunwindstack/Maps.cpp b/libunwindstack/Maps.cpp index 1e4f72e..5da73e4 100644 --- a/libunwindstack/Maps.cpp +++ b/libunwindstack/Maps.cpp @@ -47,9 +47,9 @@ MapInfo* Maps::Find(uint64_t pc) { size_t last = maps_.size(); while (first < last) { size_t index = (first + last) / 2; - MapInfo* cur = maps_[index]; + const auto& cur = maps_[index]; if (pc >= cur->start && pc < cur->end) { - return cur; + return cur.get(); } else if (pc < cur->start) { last = index; } else { @@ -67,34 +67,31 @@ bool Maps::Parse() { if (strncmp(name, "/dev/", 5) == 0 && strncmp(name + 5, "ashmem/", 7) != 0) { flags |= unwindstack::MAPS_FLAGS_DEVICE_MAP; } - maps_.push_back( - new MapInfo(maps_.empty() ? nullptr : maps_.back(), start, end, pgoff, flags, name)); + maps_.emplace_back( + new MapInfo(maps_.empty() ? nullptr : maps_.back().get(), start, end, pgoff, + flags, name)); }); } void Maps::Add(uint64_t start, uint64_t end, uint64_t offset, uint64_t flags, const std::string& name, uint64_t load_bias) { - MapInfo* map_info = - new MapInfo(maps_.empty() ? nullptr : maps_.back(), start, end, offset, flags, name); + auto map_info = + std::make_unique<MapInfo>(maps_.empty() ? nullptr : maps_.back().get(), start, end, offset, + flags, name); map_info->load_bias = load_bias; - maps_.push_back(map_info); + maps_.emplace_back(std::move(map_info)); } void Maps::Sort() { std::sort(maps_.begin(), maps_.end(), - [](const MapInfo* a, const MapInfo* b) { return a->start < b->start; }); + [](const std::unique_ptr<MapInfo>& a, const std::unique_ptr<MapInfo>& b) { + return a->start < b->start; }); // Set the prev_map values on the info objects. MapInfo* prev_map = nullptr; - for (MapInfo* map_info : maps_) { + for (const auto& map_info : maps_) { map_info->prev_map = prev_map; - prev_map = map_info; - } -} - -Maps::~Maps() { - for (auto& map : maps_) { - delete map; + prev_map = map_info.get(); } } @@ -107,8 +104,9 @@ bool BufferMaps::Parse() { if (strncmp(name, "/dev/", 5) == 0 && strncmp(name + 5, "ashmem/", 7) != 0) { flags |= unwindstack::MAPS_FLAGS_DEVICE_MAP; } - maps_.push_back( - new MapInfo(maps_.empty() ? nullptr : maps_.back(), start, end, pgoff, flags, name)); + maps_.emplace_back( + new MapInfo(maps_.empty() ? nullptr : maps_.back().get(), start, end, pgoff, + flags, name)); }); } @@ -124,10 +122,6 @@ bool LocalUpdatableMaps::Reparse() { // New maps will be added at the end without deleting the old ones. size_t last_map_idx = maps_.size(); if (!Parse()) { - // Delete any maps added by the Parse call. - for (size_t i = last_map_idx; i < maps_.size(); i++) { - delete maps_[i]; - } maps_.resize(last_map_idx); return false; } @@ -135,17 +129,16 @@ bool LocalUpdatableMaps::Reparse() { size_t total_entries = maps_.size(); size_t search_map_idx = 0; for (size_t new_map_idx = last_map_idx; new_map_idx < maps_.size(); new_map_idx++) { - MapInfo* new_map_info = maps_[new_map_idx]; + auto& new_map_info = maps_[new_map_idx]; uint64_t start = new_map_info->start; uint64_t end = new_map_info->end; uint64_t flags = new_map_info->flags; std::string* name = &new_map_info->name; for (size_t old_map_idx = search_map_idx; old_map_idx < last_map_idx; old_map_idx++) { - MapInfo* info = maps_[old_map_idx]; + auto& info = maps_[old_map_idx]; if (start == info->start && end == info->end && flags == info->flags && *name == info->name) { // No need to check search_map_idx = old_map_idx + 1; - delete new_map_info; maps_[new_map_idx] = nullptr; total_entries--; break; @@ -158,7 +151,7 @@ bool LocalUpdatableMaps::Reparse() { // Never delete these maps, they may be in use. The assumption is // that there will only every be a handfull of these so waiting // to destroy them is not too expensive. - saved_maps_.push_back(info); + saved_maps_.emplace_back(std::move(info)); maps_[old_map_idx] = nullptr; total_entries--; } @@ -169,14 +162,14 @@ bool LocalUpdatableMaps::Reparse() { // Now move out any of the maps that never were found. for (size_t i = search_map_idx; i < last_map_idx; i++) { - saved_maps_.push_back(maps_[i]); + saved_maps_.emplace_back(std::move(maps_[i])); maps_[i] = nullptr; total_entries--; } // Sort all of the values such that the nullptrs wind up at the end, then // resize them away. - std::sort(maps_.begin(), maps_.end(), [](const auto* a, const auto* b) { + std::sort(maps_.begin(), maps_.end(), [](const auto& a, const auto& b) { if (a == nullptr) { return false; } else if (b == nullptr) { @@ -189,10 +182,4 @@ bool LocalUpdatableMaps::Reparse() { return true; } -LocalUpdatableMaps::~LocalUpdatableMaps() { - for (auto map_info : saved_maps_) { - delete map_info; - } -} - } // namespace unwindstack diff --git a/libunwindstack/benchmarks/unwind_benchmarks.cpp b/libunwindstack/benchmarks/unwind_benchmarks.cpp index 8caecc7..de9137a 100644 --- a/libunwindstack/benchmarks/unwind_benchmarks.cpp +++ b/libunwindstack/benchmarks/unwind_benchmarks.cpp @@ -92,9 +92,9 @@ static void Initialize(benchmark::State& state, unwindstack::Maps& maps, // Find the libc.so share library and use that for benchmark purposes. *build_id_map_info = nullptr; - for (unwindstack::MapInfo* map_info : maps) { + for (auto& map_info : maps) { if (map_info->offset == 0 && map_info->GetBuildID() != "") { - *build_id_map_info = map_info; + *build_id_map_info = map_info.get(); break; } } diff --git a/libunwindstack/include/unwindstack/Maps.h b/libunwindstack/include/unwindstack/Maps.h index 67fbed2..1784394 100644 --- a/libunwindstack/include/unwindstack/Maps.h +++ b/libunwindstack/include/unwindstack/Maps.h @@ -20,6 +20,7 @@ #include <sys/types.h> #include <unistd.h> +#include <memory> #include <string> #include <vector> @@ -37,8 +38,16 @@ static constexpr int MAPS_FLAGS_JIT_SYMFILE_MAP = 0x4000; class Maps { public: + virtual ~Maps() = default; + Maps() = default; - virtual ~Maps(); + + // Maps are not copyable but movable, because they own pointers to MapInfo + // objects. + Maps(const Maps&) = delete; + Maps& operator=(const Maps&) = delete; + Maps(Maps&&) = default; + Maps& operator=(Maps&&) = default; MapInfo* Find(uint64_t pc); @@ -51,11 +60,11 @@ class Maps { void Sort(); - typedef std::vector<MapInfo*>::iterator iterator; + typedef std::vector<std::unique_ptr<MapInfo>>::iterator iterator; iterator begin() { return maps_.begin(); } iterator end() { return maps_.end(); } - typedef std::vector<MapInfo*>::const_iterator const_iterator; + typedef std::vector<std::unique_ptr<MapInfo>>::const_iterator const_iterator; const_iterator begin() const { return maps_.begin(); } const_iterator end() const { return maps_.end(); } @@ -63,11 +72,11 @@ class Maps { MapInfo* Get(size_t index) { if (index >= maps_.size()) return nullptr; - return maps_[index]; + return maps_[index].get(); } protected: - std::vector<MapInfo*> maps_; + std::vector<std::unique_ptr<MapInfo>> maps_; }; class RemoteMaps : public Maps { @@ -90,14 +99,14 @@ class LocalMaps : public RemoteMaps { class LocalUpdatableMaps : public Maps { public: LocalUpdatableMaps() : Maps() {} - virtual ~LocalUpdatableMaps(); + virtual ~LocalUpdatableMaps() = default; bool Reparse(); const std::string GetMapsFile() const override; private: - std::vector<MapInfo*> saved_maps_; + std::vector<std::unique_ptr<MapInfo>> saved_maps_; }; class BufferMaps : public Maps { diff --git a/libunwindstack/tests/MapsTest.cpp b/libunwindstack/tests/MapsTest.cpp index b4197f2..9e7a6ab 100644 --- a/libunwindstack/tests/MapsTest.cpp +++ b/libunwindstack/tests/MapsTest.cpp @@ -61,6 +61,26 @@ TEST(MapsTest, map_add) { ASSERT_EQ(0U, info->load_bias.load()); } +TEST(MapsTest, map_move) { + Maps maps; + + maps.Add(0x1000, 0x2000, 0, PROT_READ, "fake_map", 0); + maps.Add(0x3000, 0x4000, 0x10, 0, "", 0x1234); + maps.Add(0x5000, 0x6000, 1, 2, "fake_map2", static_cast<uint64_t>(-1)); + + Maps maps2 = std::move(maps); + + ASSERT_EQ(3U, maps2.Total()); + MapInfo* info = maps2.Get(0); + ASSERT_EQ(0x1000U, info->start); + ASSERT_EQ(0x2000U, info->end); + ASSERT_EQ(0U, info->offset); + ASSERT_EQ(PROT_READ, info->flags); + ASSERT_EQ("fake_map", info->name); + ASSERT_EQ(0U, info->elf_offset); + ASSERT_EQ(0U, info->load_bias.load()); +} + TEST(MapsTest, verify_parse_line) { MapInfo info(nullptr, 0, 0, 0, 0, ""); diff --git a/libunwindstack/tests/UnwinderTest.cpp b/libunwindstack/tests/UnwinderTest.cpp index d88531f..2dc5118 100644 --- a/libunwindstack/tests/UnwinderTest.cpp +++ b/libunwindstack/tests/UnwinderTest.cpp @@ -49,7 +49,7 @@ class UnwinderTest : public ::testing::Test { std::string str_name(name); maps_->Add(start, end, offset, flags, name, static_cast<uint64_t>(-1)); if (elf != nullptr) { - MapInfo* map_info = *--maps_->end(); + const auto& map_info = *--maps_->end(); map_info->elf.reset(elf); } } @@ -85,7 +85,7 @@ class UnwinderTest : public ::testing::Test { AddMapInfo(0x53000, 0x54000, 0, PROT_READ | PROT_WRITE, "/fake/fake.oat"); AddMapInfo(0xa3000, 0xa4000, 0, PROT_READ | PROT_WRITE | PROT_EXEC, "/fake/fake.vdex"); - MapInfo* info = *--maps_->end(); + const auto& info = *--maps_->end(); info->load_bias = 0; elf = new ElfFake(new MemoryFake); @@ -98,8 +98,8 @@ class UnwinderTest : public ::testing::Test { elf->FakeSetInterface(new ElfInterfaceFake(nullptr)); AddMapInfo(0xa7000, 0xa8000, 0, PROT_READ | PROT_WRITE | PROT_EXEC, "/fake/fake_offset.oat", elf); - info = *--maps_->end(); - info->elf_offset = 0x8000; + const auto& info2 = *--maps_->end(); + info2->elf_offset = 0x8000; process_memory_.reset(new MemoryFake); } |