From c1cd8324f786cfe0251d75aa48a92fa783a2c19f Mon Sep 17 00:00:00 2001 From: officeyutong Date: Wed, 22 Nov 2023 17:25:25 +0800 Subject: [PATCH 1/2] Remove usage of weak_ptr in attach manager --- .../attach_manager/frida_attach_manager.cpp | 109 ++++++++---------- .../attach_manager/frida_attach_manager.hpp | 8 +- .../unit-test/attach/test_filter_attach.cpp | 9 +- .../unit-test/attach/test_replace_attach.cpp | 10 +- .../test_attach_filter_with_ebpf.cpp | 4 +- 5 files changed, 68 insertions(+), 72 deletions(-) diff --git a/runtime/src/attach/attach_manager/frida_attach_manager.cpp b/runtime/src/attach/attach_manager/frida_attach_manager.cpp index 39bb933c..e3fc6a42 100644 --- a/runtime/src/attach/attach_manager/frida_attach_manager.cpp +++ b/runtime/src/attach/attach_manager/frida_attach_manager.cpp @@ -76,7 +76,7 @@ int frida_attach_manager::attach_at(void *func_addr, callback_variant &&cb) // Create a frida attach entry itr = internal_attaches .emplace(func_addr, - std::make_shared< + std::make_unique< frida_internal_attach_entry>( func_addr, (attach_type)cb.index(), @@ -97,11 +97,12 @@ int frida_attach_manager::attach_at(void *func_addr, callback_variant &&cb) auto inserted_attach_entry = this->attaches .emplace(ent.self_id, - std::make_shared( + std::make_unique( std::move(ent))) .first; - inner_attach->user_attaches.push_back(inserted_attach_entry->second); - inserted_attach_entry->second->internal_attaches = inner_attach; + inner_attach->user_attaches.push_back( + inserted_attach_entry->second.get()); + inserted_attach_entry->second->internal_attache = inner_attach.get(); return result; } @@ -140,20 +141,19 @@ int frida_attach_manager::destroy_attach(int id) { void *drop_func_addr = nullptr; if (auto itr = attaches.find(id); itr != attaches.end()) { - if (auto p = itr->second->internal_attaches.lock(); p) { - auto &user_attaches = p->user_attaches; - auto tail = - std::remove_if(user_attaches.begin(), - user_attaches.end(), - [&](const auto &v) -> bool { - return v.lock().get() == - itr->second.get(); - }); - user_attaches.resize(tail - user_attaches.begin()); - attaches.erase(itr); - if (p->user_attaches.empty()) { - drop_func_addr = p->function; - } + auto p = itr->second->internal_attache; + + auto &user_attaches = p->user_attaches; + auto tail = + std::remove_if(user_attaches.begin(), + user_attaches.end(), + [&](const auto &v) -> bool { + return v == itr->second.get(); + }); + user_attaches.resize(tail - user_attaches.begin()); + attaches.erase(itr); + if (p->user_attaches.empty()) { + drop_func_addr = p->function; } } else { spdlog::error("Unable to find attach id {}", id); @@ -184,10 +184,8 @@ int frida_attach_manager::destroy_attach_by_func_addr(const void *func) if (auto itr = internal_attaches.find((void *)func); itr != internal_attaches.end()) { auto uattaches = itr->second->user_attaches; - for (auto p : uattaches) { - if (auto attach_entry = p.lock(); attach_entry) { - attaches.erase(attach_entry->self_id); - } + for (auto attach_entry : uattaches) { + attaches.erase(attach_entry->self_id); } internal_attaches.erase(itr); return 0; @@ -255,8 +253,8 @@ frida_internal_attach_entry::frida_internal_attach_entry( this, nullptr); err < 0) { spdlog::error( - "Failed to execute frida replace for function {:x}, when attaching filter", - (uintptr_t)function); + "Failed to execute frida replace for function {:x}, when attaching filter, err={}", + (uintptr_t)function, err); throw std::runtime_error("Failed to attach filter"); } override_return_callback = override_return_set_callback( @@ -272,8 +270,8 @@ frida_internal_attach_entry::frida_internal_attach_entry( this, nullptr); err < 0) { spdlog::error( - "Failed to execute frida replace for function {:x}, when attaching replace", - (uintptr_t)function); + "Failed to execute frida replace for function {:x}, when attaching replace, err={}", + (uintptr_t)function, err); throw std::runtime_error("Failed to attach replace"); } } @@ -297,12 +295,10 @@ frida_internal_attach_entry::~frida_internal_attach_entry() bool frida_internal_attach_entry::has_replace_or_override() const { - for (auto p : user_attaches) { - if (auto v = p.lock(); v) { - if (v->get_type() == attach_type::REPLACE || - v->get_type() == attach_type::UPROBE_OVERRIDE) { - return true; - } + for (auto v : user_attaches) { + if (v->get_type() == attach_type::REPLACE || + v->get_type() == attach_type::UPROBE_OVERRIDE) { + return true; } } return false; @@ -310,12 +306,10 @@ bool frida_internal_attach_entry::has_replace_or_override() const bool frida_internal_attach_entry::has_uprobe_or_uretprobe() const { - for (auto p : user_attaches) { - if (auto v = p.lock(); v) { - if (v->get_type() == attach_type::UPROBE || - v->get_type() == attach_type::URETPROBE) { - return true; - } + for (auto v : user_attaches) { + if (v->get_type() == attach_type::UPROBE || + v->get_type() == attach_type::URETPROBE) { + return true; } } return false; @@ -324,13 +318,10 @@ bool frida_internal_attach_entry::has_uprobe_or_uretprobe() const base_attach_manager::replace_callback & frida_internal_attach_entry::get_replace_callback() const { - for (auto p : user_attaches) { - if (auto v = p.lock(); v) { - if (v->get_type() == attach_type::REPLACE) { - return std::get< - base_attach_manager::replace_callback>( - v->cb); - } + for (auto v : user_attaches) { + if (v->get_type() == attach_type::REPLACE) { + return std::get( + v->cb); } } spdlog::error( @@ -342,13 +333,10 @@ frida_internal_attach_entry::get_replace_callback() const base_attach_manager::filter_callback & frida_internal_attach_entry::get_filter_callback() const { - for (auto p : user_attaches) { - if (auto v = p.lock(); v) { - if (v->get_type() == attach_type::UPROBE_OVERRIDE) { - return std::get< - base_attach_manager::filter_callback>( - v->cb); - } + for (auto v : user_attaches) { + if (v->get_type() == attach_type::UPROBE_OVERRIDE) { + return std::get( + v->cb); } } spdlog::error( @@ -360,11 +348,9 @@ frida_internal_attach_entry::get_filter_callback() const void frida_internal_attach_entry::iterate_uprobe_callbacks( const pt_regs ®s) const { - for (auto p : user_attaches) { - if (auto v = p.lock(); v) { - if (v->get_type() == attach_type::UPROBE) { - std::get<(int)attach_type::UPROBE>(v->cb)(regs); - } + for (auto v : user_attaches) { + if (v->get_type() == attach_type::UPROBE) { + std::get<(int)attach_type::UPROBE>(v->cb)(regs); } } } @@ -372,12 +358,9 @@ void frida_internal_attach_entry::iterate_uprobe_callbacks( void frida_internal_attach_entry::iterate_uretprobe_callbacks( const pt_regs ®s) const { - for (auto p : user_attaches) { - if (auto v = p.lock(); v) { - if (v->get_type() == attach_type::URETPROBE) { - std::get<(int)attach_type::URETPROBE>(v->cb)( - regs); - } + for (auto v : user_attaches) { + if (v->get_type() == attach_type::URETPROBE) { + std::get<(int)attach_type::URETPROBE>(v->cb)(regs); } } } diff --git a/runtime/src/attach/attach_manager/frida_attach_manager.hpp b/runtime/src/attach/attach_manager/frida_attach_manager.hpp index 8fec1cc5..61dfe6f0 100644 --- a/runtime/src/attach/attach_manager/frida_attach_manager.hpp +++ b/runtime/src/attach/attach_manager/frida_attach_manager.hpp @@ -22,7 +22,7 @@ class frida_attach_entry { base_attach_manager::callback_variant cb; void *function; - std::weak_ptr internal_attaches; + class frida_internal_attach_entry *internal_attache; friend class frida_attach_manager; friend class frida_internal_attach_entry; @@ -44,7 +44,7 @@ class frida_attach_entry { class frida_internal_attach_entry { void *function; GumInterceptor *interceptor; - std::vector > user_attaches; + std::vector user_attaches; GumInvocationListener *frida_gum_invocation_listener = nullptr; friend class frida_attach_manager; @@ -96,8 +96,8 @@ class frida_attach_manager final : public base_attach_manager { private: GumInterceptor *interceptor; int next_id = 1; - std::unordered_map > attaches; - std::unordered_map > + std::unordered_map > attaches; + std::unordered_map > internal_attaches; friend class frida_internal_attach_entry; diff --git a/runtime/unit-test/attach/test_filter_attach.cpp b/runtime/unit-test/attach/test_filter_attach.cpp index 897fb94a..81eea84d 100644 --- a/runtime/unit-test/attach/test_filter_attach.cpp +++ b/runtime/unit-test/attach/test_filter_attach.cpp @@ -1,6 +1,7 @@ #include #include #include +#include #if !defined(__x86_64__) && defined(_M_X64) #error Only supports x86_64 #endif @@ -14,6 +15,12 @@ __bpftime_func_to_filter(uint64_t a, uint64_t b) return (a << 32) | b; } +__attribute__((__noinline__, optnone, noinline)) static uint64_t +call_filter_func(uint64_t a, uint64_t b) +{ + return __bpftime_func_to_filter(a, b); +} + TEST_CASE("Test attaching filter programs and revert") { frida_attach_manager man; @@ -37,7 +44,7 @@ TEST_CASE("Test attaching filter programs and revert") }); REQUIRE(id >= 0); REQUIRE(__bpftime_func_to_filter(1, 2) == (((uint64_t)1 << 32) | 2)); - REQUIRE(__bpftime_func_to_filter(a, b) == a + b); + REQUIRE(call_filter_func(a, b) == a + b); // Revert it SECTION("Revert by id") diff --git a/runtime/unit-test/attach/test_replace_attach.cpp b/runtime/unit-test/attach/test_replace_attach.cpp index b90c3256..30d15c0d 100644 --- a/runtime/unit-test/attach/test_replace_attach.cpp +++ b/runtime/unit-test/attach/test_replace_attach.cpp @@ -1,17 +1,23 @@ #include #include +#include #if !defined(__x86_64__) && defined(_M_X64) #error Only supports x86_64 #endif using namespace bpftime; -__attribute__((__noinline__)) extern "C" uint64_t +__attribute__((__noinline__, optnone, noinline)) extern "C" uint64_t __bpftime_func_to_replace(uint64_t a, uint64_t b) { // Forbid inline asm(""); return (a << 32) | b; } +__attribute__((__noinline__, optnone, noinline)) static uint64_t +call_replace_func(uint64_t a, uint64_t b) +{ + return __bpftime_func_to_replace(a, b); +} TEST_CASE("Test attaching replace programs and revert") { frida_attach_manager man; @@ -30,7 +36,7 @@ TEST_CASE("Test attaching replace programs and revert") return regs.si + regs.di; }); REQUIRE(id >= 0); - REQUIRE(__bpftime_func_to_replace(a, b) == a + b); + REQUIRE(call_replace_func(a, b) == a + b); REQUIRE(invoke_times == 1); // Revert it invoke_times = 0; diff --git a/runtime/unit-test/attach_with_ebpf/test_attach_filter_with_ebpf.cpp b/runtime/unit-test/attach_with_ebpf/test_attach_filter_with_ebpf.cpp index c5bbc27e..ad925f8e 100644 --- a/runtime/unit-test/attach_with_ebpf/test_attach_filter_with_ebpf.cpp +++ b/runtime/unit-test/attach_with_ebpf/test_attach_filter_with_ebpf.cpp @@ -6,14 +6,14 @@ using namespace bpftime; // This is the original function to hook. -__attribute__((__noinline__)) extern "C" int +__attribute__((__noinline__, optnone)) extern "C" int __bpftime_attach_filter_with_ebpf__my_function(const char *str, char c, long long parm1) { asm(""); // buggy code: not check str is NULL int i = str[0]; - spdlog::info("origin func: Args: %s, %c, %d", str, c, i); + spdlog::info("origin func: Args: {}, {}, {}", str, c, i); return (int)parm1; } From 9cdfedd26d60526f7217822fa316e266e020dcf0 Mon Sep 17 00:00:00 2001 From: officeyutong Date: Wed, 22 Nov 2023 17:49:56 +0800 Subject: [PATCH 2/2] Fix typo --- runtime/src/attach/attach_manager/frida_attach_manager.cpp | 4 ++-- runtime/src/attach/attach_manager/frida_attach_manager.hpp | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/runtime/src/attach/attach_manager/frida_attach_manager.cpp b/runtime/src/attach/attach_manager/frida_attach_manager.cpp index e3fc6a42..87a6c592 100644 --- a/runtime/src/attach/attach_manager/frida_attach_manager.cpp +++ b/runtime/src/attach/attach_manager/frida_attach_manager.cpp @@ -102,7 +102,7 @@ int frida_attach_manager::attach_at(void *func_addr, callback_variant &&cb) .first; inner_attach->user_attaches.push_back( inserted_attach_entry->second.get()); - inserted_attach_entry->second->internal_attache = inner_attach.get(); + inserted_attach_entry->second->internal_attach = inner_attach.get(); return result; } @@ -141,7 +141,7 @@ int frida_attach_manager::destroy_attach(int id) { void *drop_func_addr = nullptr; if (auto itr = attaches.find(id); itr != attaches.end()) { - auto p = itr->second->internal_attache; + auto p = itr->second->internal_attach; auto &user_attaches = p->user_attaches; auto tail = diff --git a/runtime/src/attach/attach_manager/frida_attach_manager.hpp b/runtime/src/attach/attach_manager/frida_attach_manager.hpp index 61dfe6f0..7d9b56bf 100644 --- a/runtime/src/attach/attach_manager/frida_attach_manager.hpp +++ b/runtime/src/attach/attach_manager/frida_attach_manager.hpp @@ -22,7 +22,7 @@ class frida_attach_entry { base_attach_manager::callback_variant cb; void *function; - class frida_internal_attach_entry *internal_attache; + class frida_internal_attach_entry *internal_attach; friend class frida_attach_manager; friend class frida_internal_attach_entry;