From be8ae904c0f12b8c2443b88ac64db844ce20e082 Mon Sep 17 00:00:00 2001 From: Officeyutong Date: Mon, 16 Oct 2023 22:18:53 +0800 Subject: [PATCH] runtime: fix a double-free bug (#37) * Fix a CI issue * Update --- runtime/src/attach/bpf_attach_ctx.cpp | 16 +++++++++++++++- runtime/src/bpftime_shm_internal.cpp | 3 ++- runtime/src/ffi.cpp | 14 +++++++++++--- runtime/src/handler/perf_event_handler.cpp | 4 ++++ runtime/test/CMakeLists.txt | 8 +++++++- runtime/test/src/test_attach_hook.cpp | 10 ++++++---- runtime/test/src/test_attach_uprobe.cpp | 8 +++++--- runtime/test/src/test_shm_progs_attach.cpp | 11 +++++++++-- vm/test/include/test_minimal_bpf_host_ffi.h | 15 +++++++++++---- 9 files changed, 70 insertions(+), 19 deletions(-) diff --git a/runtime/src/attach/bpf_attach_ctx.cpp b/runtime/src/attach/bpf_attach_ctx.cpp index a71b15c1..6974dc4e 100644 --- a/runtime/src/attach/bpf_attach_ctx.cpp +++ b/runtime/src/attach/bpf_attach_ctx.cpp @@ -387,13 +387,16 @@ int bpf_attach_ctx::detach(const bpftime_prog *prog) bpf_attach_ctx::~bpf_attach_ctx() { + spdlog::debug("Destructor: bpf_attach_ctx"); std::vector id_to_remove = {}; for (auto &m : hook_entry_table) { id_to_remove.push_back(m.second.id); } for (auto i : id_to_remove) { + spdlog::debug("Destroy attach of {}", i); destory_attach(i); } + gum_object_unref(interceptor); } int bpf_attach_ctx::revert_func(void *target_function) { @@ -411,6 +414,8 @@ int bpf_attach_ctx::destory_attach(int id) if (entry == hook_entry_table.end()) { return -1; } + spdlog::debug("Revert attach of id {}, type {}", id, + (int)entry->second.type); switch (entry->second.type) { case BPFTIME_REPLACE: { if (entry->second.hook_func != nullptr) { @@ -423,22 +428,29 @@ int bpf_attach_ctx::destory_attach(int id) } case BPFTIME_UPROBE: { if (entry->second.uretprobe_id == id) { + spdlog::debug("Revert uretprobe"); hook_entry_index.erase(entry->second.uretprobe_id); entry->second.uretprobe_id = -1; entry->second.ret_progs.clear(); } if (entry->second.id == id) { + spdlog::debug("Revert uprobe"); hook_entry_index.erase(entry->second.id); entry->second.id = -1; entry->second.progs.clear(); } if (entry->second.listener != nullptr && entry->second.id < 0 && entry->second.uretprobe_id < 0) { + spdlog::debug("Freeing listener"); // detach the listener when no one is using it gum_interceptor_detach(interceptor, entry->second.listener); - gum_free(entry->second.listener); + spdlog::debug("Frida detached"); + // No need to manually free, because frida uses RefCounting GC + // gum_free(entry->second.listener); + gum_object_unref(entry->second.listener); hook_entry_table.erase(function->second); + spdlog::debug("Listener freed"); } return 0; } @@ -724,6 +736,8 @@ int bpf_attach_ctx::create_uprobe(void *function, int id, bool retprobe) auto entry_ptr = &entry->second; entry->second.listener = (GumInvocationListener *)g_object_new( uprobe_listener_get_type(), NULL); + spdlog::debug("Created listener instance {:x} for hook_entry id {}", + (uintptr_t)entry->second.listener, id); // gum_make_call_listener(frida_uprobe_listener_on_enter, // frida_uprobe_listener_on_leave, diff --git a/runtime/src/bpftime_shm_internal.cpp b/runtime/src/bpftime_shm_internal.cpp index 7cd4ec41..25f31287 100644 --- a/runtime/src/bpftime_shm_internal.cpp +++ b/runtime/src/bpftime_shm_internal.cpp @@ -4,6 +4,7 @@ #include #include #include +#include #include void bpftime_initialize_global_shm() @@ -20,7 +21,7 @@ void bpftime_destroy_global_shm() shm_holder.global_shared_memory.~bpftime_shm(); // Why not spdlog? because global variables that spdlog used were // already destroyed.. - puts("INFO: Global shm destructed"); + printf("INFO [%d]: Global shm destructed\n", (int)getpid()); } static __attribute__((destructor(65535))) void __destruct_shm() { diff --git a/runtime/src/ffi.cpp b/runtime/src/ffi.cpp index ce696240..716c915b 100644 --- a/runtime/src/ffi.cpp +++ b/runtime/src/ffi.cpp @@ -94,7 +94,9 @@ extern "C" uint64_t __ebpf_call_ffi_dispatcher(uint64_t id, uint64_t arg_list) union arg_val to_arg_val(enum ffi_types type, uint64_t val) { - union arg_val arg; + union arg_val arg { + .uint64 = 0 + }; switch (type) { case FFI_TYPE_INT8: case FFI_TYPE_INT16: @@ -109,7 +111,10 @@ union arg_val to_arg_val(enum ffi_types type, uint64_t val) arg.uint64 = val; break; case FFI_TYPE_DOUBLE: - arg.double_val = *(double *)&val; +#pragma GCC diagnostic push +#pragma GCC diagnostic ignored "-Wstrict-aliasing" + arg.double_val = *(double *)(uintptr_t)&val; +#pragma GCC diagnostic pop break; case FFI_TYPE_POINTER: arg.ptr = (void *)val; @@ -139,7 +144,10 @@ uint64_t from_arg_val(enum ffi_types type, union arg_val val) case FFI_TYPE_UINT64: return val.uint64; case FFI_TYPE_DOUBLE: - return *(uint64_t *)&val.double_val; +#pragma GCC diagnostic push +#pragma GCC diagnostic ignored "-Wstrict-aliasing" + return *(uint64_t *)(uintptr_t)&val.double_val; +#pragma GCC diagnostic pop case FFI_TYPE_POINTER: return (uint64_t)val.ptr; case FFI_TYPE_VOID: diff --git a/runtime/src/handler/perf_event_handler.cpp b/runtime/src/handler/perf_event_handler.cpp index 1ec0e72a..d886784e 100644 --- a/runtime/src/handler/perf_event_handler.cpp +++ b/runtime/src/handler/perf_event_handler.cpp @@ -9,6 +9,10 @@ #include #include #include + +#pragma GCC diagnostic ignored "-Wstrict-aliasing" + + #define READ_ONCE_U64(x) (*(volatile uint64_t *)&x) #define WRITE_ONCE_U64(x, v) (*(volatile uint64_t *)&x) = (v) diff --git a/runtime/test/CMakeLists.txt b/runtime/test/CMakeLists.txt index db434f01..e81ba30a 100644 --- a/runtime/test/CMakeLists.txt +++ b/runtime/test/CMakeLists.txt @@ -22,10 +22,10 @@ set(test_sources src/test_shm_hash_maps.cpp src/test_shm_progs_attach.cpp + # src/test_shm_client.cpp # src/test_shm_server.cpp # src/test_ffi.cpp - src/test_helpers.cpp ) @@ -95,9 +95,15 @@ foreach(file ${test_sources}) runtime bpftime-object ) + # message("Unknown testing library ${test_name}_Tests. Please setup your desired unit testing library by using `target_link_libraries`.") endif() + if(CMAKE_BUILD_TYPE STREQUAL "Debug") + # Enable sanitizers if we are debugging + target_link_options(${test_name}_Tests PRIVATE -fsanitize=undefined) + endif() + # # Add the unit tests # diff --git a/runtime/test/src/test_attach_hook.cpp b/runtime/test/src/test_attach_hook.cpp index 8d89b6b0..dae44bdc 100644 --- a/runtime/test/src/test_attach_hook.cpp +++ b/runtime/test/src/test_attach_hook.cpp @@ -9,7 +9,8 @@ using namespace bpftime; -const shm_open_type bpftime::global_shm_open_type = shm_open_type::SHM_NO_CREATE; +const shm_open_type bpftime::global_shm_open_type = + shm_open_type::SHM_NO_CREATE; // This is the hook function. int my_hook_function() @@ -29,18 +30,19 @@ unsigned char orig_bytes[256]; int main() { - int res = my_function(); + int res [[maybe_unused]] = my_function(); assert(res == 67); bpf_attach_ctx probe_ctx; - probe_ctx.replace_func((void*)my_hook_function, (void*)my_function, NULL); + probe_ctx.replace_func((void *)my_hook_function, (void *)my_function, + NULL); // Now calling the function will actually call the hook function. res = my_function(); assert(res == 11); - probe_ctx.revert_func((void*)my_function); + probe_ctx.revert_func((void *)my_function); // Now calling the function will call the original function. res = my_function(); diff --git a/runtime/test/src/test_attach_uprobe.cpp b/runtime/test/src/test_attach_uprobe.cpp index fe3fa3e0..c27792ab 100644 --- a/runtime/test/src/test_attach_uprobe.cpp +++ b/runtime/test/src/test_attach_uprobe.cpp @@ -12,7 +12,8 @@ #include "bpftime_shm.hpp" using namespace bpftime; -const shm_open_type bpftime::global_shm_open_type = shm_open_type::SHM_NO_CREATE; +const shm_open_type bpftime::global_shm_open_type = + shm_open_type::SHM_NO_CREATE; // This is the original function to hook. int my_function(int parm1, const char *str, char c) @@ -28,8 +29,9 @@ bpftime_prog *get_prog(const char *name, bpftime_object *obj) bpftime_prog *prog = bpftime_object_find_program_by_name(obj, name); assert(prog); // add ffi support - int res = bpftime_helper_group::get_kernel_utils_helper_group() - .add_helper_group_to_prog(prog); + int res [[maybe_unused]] = + bpftime_helper_group::get_kernel_utils_helper_group() + .add_helper_group_to_prog(prog); assert(res == 0); res = prog->bpftime_prog_load(false); assert(res == 0); diff --git a/runtime/test/src/test_shm_progs_attach.cpp b/runtime/test/src/test_shm_progs_attach.cpp index 14a19b5d..16db723c 100644 --- a/runtime/test/src/test_shm_progs_attach.cpp +++ b/runtime/test/src/test_shm_progs_attach.cpp @@ -12,11 +12,13 @@ #include #include #include +#include using namespace boost::interprocess; using namespace bpftime; -const shm_open_type bpftime::global_shm_open_type = shm_open_type::SHM_NO_CREATE; +const shm_open_type bpftime::global_shm_open_type = + shm_open_type::SHM_NO_CREATE; const char *HANDLER_NAME = "my_handler"; const char *SHM_NAME = "my_shm_attach_test"; @@ -106,6 +108,7 @@ void attach_replace(bpftime::handler_manager &manager_ref, int main(int argc, const char **argv) { + spdlog::set_level(spdlog::level::debug); if (argc == 1) { std::cout << "parent process start" << std::endl; shm_remove remover(SHM_NAME); @@ -139,11 +142,15 @@ int main(int argc, const char **argv) std::cout << "Starting sub process" << std::endl; system((std::string(argv[0]) + " sub").c_str()); + std::cout << "Subprocess exited, from server" << std::endl; assert(segment.find(HANDLER_NAME).first == nullptr); + std::cout << "Server exiting.." << std::endl; + bpftime_object_close(obj); } else { int res = 0; - std::cout << "Subprocess started" << std::endl; + std::cout << "Subprocess started, pid=" << getpid() + << std::endl; // test for no attach res = my_function(1, "hello aaa", 'c'); diff --git a/vm/test/include/test_minimal_bpf_host_ffi.h b/vm/test/include/test_minimal_bpf_host_ffi.h index 85bee85d..934189df 100644 --- a/vm/test/include/test_minimal_bpf_host_ffi.h +++ b/vm/test/include/test_minimal_bpf_host_ffi.h @@ -72,7 +72,10 @@ static inline union arg_val to_arg_val(enum ffi_types type, uint64_t val) arg.uint64 = val; break; case FFI_TYPE_DOUBLE: - arg.double_val = *(double *)&val; +#pragma GCC diagnostic push +#pragma GCC diagnostic ignored "-Wstrict-aliasing" + arg.double_val = *(double *)(uintptr_t)&val; +#pragma GCC diagnostic pop break; case FFI_TYPE_POINTER: arg.ptr = (void *)(uintptr_t)val; @@ -91,7 +94,10 @@ static inline uint64_t from_arg_val(enum ffi_types type, union arg_val val) case FFI_TYPE_UINT: return val.uint64; case FFI_TYPE_DOUBLE: - return *(uint64_t *)&val.double_val; +#pragma GCC diagnostic push +#pragma GCC diagnostic ignored "-Wstrict-aliasing" + return *(uint64_t *)(uintptr_t)&val.double_val; +#pragma GCC diagnostic pop case FFI_TYPE_POINTER: return (uint64_t)(uintptr_t)val.ptr; default: @@ -106,15 +112,16 @@ static uint64_t __ebpf_call_ffi_dispatcher(uint64_t id, uint64_t arg_list) assert(id < MAX_FFI_FUNCS); struct ebpf_ffi_func_info *func_info = ebpf_resovle_ffi_func(id); assert(func_info->func != NULL); - // Prepare arguments struct arg_list *raw_args = (struct arg_list *)(uintptr_t)arg_list; union arg_val args[5]; +#pragma GCC diagnostic push +#pragma GCC diagnostic ignored "-Wnull-dereference" for (int i = 0; i < func_info->num_args; i++) { args[i] = to_arg_val(func_info->arg_types[i], raw_args->args[i]); } - +#pragma GCC diagnostic pop // Call the function union arg_val ret = { .uint64 = 0 }; switch (func_info->num_args) {