libhook: add optional display name for return hook (#1708)

* libhook: add optional display name for return hook * do not segfault on return hook destruction if someone overwrite trap name after creation
tklengyel · Sep 15, 2023 · 35d1d0b · 35d1d0b
1 parent 28795aa
commit 35d1d0b
Show file tree

Hide file tree

Showing 9 changed files with 50 additions and 44 deletions.
diff --git a/src/libhook/hooks/return.cpp b/src/libhook/hooks/return.cpp
@@ -103,12 +103,15 @@
  ***************************************************************************/
 #include <libhook/hooks/return.hpp>
 
+#include <glib.h>
+
 namespace libhook
 {
 
-ReturnHook::ReturnHook(drakvuf_t drakvuf, cb_wrapper_t cb)
+ReturnHook::ReturnHook(drakvuf_t drakvuf, cb_wrapper_t cb, const char* display_name)
     : BaseHook(drakvuf),
-      callback_(cb)
+      callback_(cb),
+      display_name_(g_strdup(display_name))
 {}
 
 ReturnHook::~ReturnHook()
@@ -122,17 +125,32 @@ ReturnHook::~ReturnHook()
             PRINT_DEBUG("[LIBHOOK] drakvuf called deleted hook, replaced by nullstub\n");
             return VMI_EVENT_RESPONSE_NONE;
         };
-        drakvuf_remove_trap(this->drakvuf_, this->trap_, [](drakvuf_trap_t* trap)
+        if (this->display_name_ != this->trap_->name)
+        {
+            g_free(this->display_name_);
+            drakvuf_remove_trap(this->drakvuf_, this->trap_, [](drakvuf_trap_t* trap)
+            {
+                trap->data = nullptr;
+                delete trap;
+            });
+        }
+        else
         {
-            trap->data = nullptr;
-            delete trap;
-        });
+            drakvuf_remove_trap(this->drakvuf_, this->trap_, [](drakvuf_trap_t* trap)
+            {
+                trap->data = nullptr;
+                g_free(const_cast<char*>(trap->name));
+                delete trap;
+            });
+        }
+        this->display_name_ = nullptr;
     }
     else
     {
         // otherwise this has been moved from and we don't free the trap
         // as the ownership has been passed elsewhere, so we do nothing
         PRINT_DEBUG("[LIBHOOK] destruction not needed, as return hook was moved from\n");
+        g_free(this->display_name_);
     }
 }
 
@@ -141,12 +159,14 @@ ReturnHook::ReturnHook(ReturnHook&& rhs) noexcept
 {
     std::swap(this->callback_, rhs.callback_);
     std::swap(this->trap_, rhs.trap_);
+    std::swap(this->display_name_, rhs.display_name_);
 }
 
 ReturnHook& ReturnHook::operator=(ReturnHook&& rhs) noexcept
 {
     std::swap(this->callback_, rhs.callback_);
     std::swap(this->trap_, rhs.trap_);
+    std::swap(this->display_name_, rhs.display_name_);
     return *this;
 }
 

diff --git a/src/libhook/hooks/return.hpp b/src/libhook/hooks/return.hpp
@@ -119,7 +119,7 @@ class ReturnHook : public BaseHook
      */
     template<typename Params = CallResult>
     [[nodiscard]]
-    static auto create(drakvuf_t, drakvuf_trap_info* info, cb_wrapper_t cb, int ttl)
+    static auto create(drakvuf_t, drakvuf_trap_info* info, cb_wrapper_t cb, const char* display_name, int ttl)
     -> std::unique_ptr<ReturnHook>;
 
     /**
@@ -154,16 +154,17 @@ class ReturnHook : public BaseHook
     cb_wrapper_t callback_ = nullptr;
     drakvuf_trap_t* trap_ = nullptr;
     std::shared_ptr<CallResult> params_;
+    char* display_name_;
 
 protected:
     /**
      * Hide ctor from users, as we enforce factory function usage.
      */
-    ReturnHook(drakvuf_t, cb_wrapper_t cb);
+    ReturnHook(drakvuf_t, cb_wrapper_t cb, const char* display_name);
 };
 
 template<typename Params>
-auto ReturnHook::create(drakvuf_t drakvuf, drakvuf_trap_info* info, cb_wrapper_t cb, int ttl)
+auto ReturnHook::create(drakvuf_t drakvuf, drakvuf_trap_info* info, cb_wrapper_t cb, const char* display_name, int ttl)
 -> std::unique_ptr<ReturnHook>
 {
     PRINT_DEBUG("[LIBHOOK] creating return hook\n");
@@ -184,7 +185,6 @@ auto ReturnHook::create(drakvuf_t drakvuf, drakvuf_trap_info* info, cb_wrapper_t
     trap->breakpoint.module = info->trap->breakpoint.module;
 
     trap->type = BREAKPOINT;
-    trap->name = "ReturnHook";
     trap->ah_cb = nullptr;
     trap->ttl = ttl;
     trap->cb = [](drakvuf_t drakvuf, drakvuf_trap_info_t* info)
@@ -193,8 +193,9 @@ auto ReturnHook::create(drakvuf_t drakvuf, drakvuf_trap_info* info, cb_wrapper_t
     };
 
     // not using std::make_unique because ctor is private
-    auto hook = std::unique_ptr<ReturnHook>(new ReturnHook(drakvuf, cb));
+    auto hook = std::unique_ptr<ReturnHook>(new ReturnHook(drakvuf, cb, display_name ?: "ReturnHook"));
     hook->trap_ = trap;
+    hook->trap_->name = hook->display_name_;
 
     static_assert(std::is_base_of_v<CallResult, Params>, "Params must derive from CallResult");
     static_assert(std::is_default_constructible_v<Params>, "Params must be default constructible");

diff --git a/src/plugins/apimon/apimon.cpp b/src/plugins/apimon/apimon.cpp
@@ -243,13 +243,12 @@ static event_response_t usermode_hook_cb(drakvuf_t drakvuf, drakvuf_trap_info* i
 
     uint64_t hookID = make_hook_id(info);
     auto hook = plugin->createReturnHook<ApimonReturnHookData>(info,
-            &apimon::usermode_return_hook_cb, drakvuf_get_limited_traps_ttl(drakvuf));
+            &apimon::usermode_return_hook_cb, target->target_name.data(), drakvuf_get_limited_traps_ttl(drakvuf));
     auto params = libhook::GetTrapParams<ApimonReturnHookData>(hook->trap_);
 
     params->arguments = std::move(arguments);
     params->target = target;
 
-    hook->trap_->name = target->target_name.c_str();
     plugin->ret_hooks[hookID] = std::move(hook);
 
     return VMI_EVENT_RESPONSE_NONE;

diff --git a/src/plugins/filetracer/linux.cpp b/src/plugins/filetracer/linux.cpp
@@ -302,13 +302,12 @@ event_response_t linux_filetracer::open_file_cb(drakvuf_t drakvuf, drakvuf_trap_
 
     // Create new trap for return callback
     uint64_t hookID = make_hook_id(info);
-    auto hook = this->createReturnHook<linux_data>(info, &linux_filetracer::open_file_ret_cb);
+    auto hook = this->createReturnHook<linux_data>(info, &linux_filetracer::open_file_ret_cb, info->trap->name);
     auto params = libhook::GetTrapParams<linux_data>(hook->trap_);
 
     // Save data
     params->setResultCallParams(info);
 
-    hook->trap_->name = info->trap->name;
     this->ret_hooks[hookID] = std::move(hook);
 
     return VMI_EVENT_RESPONSE_NONE;
@@ -470,7 +469,7 @@ event_response_t linux_filetracer::memfd_create_file_cb(drakvuf_t drakvuf, drakv
 
     // Create new trap for return callback
     uint64_t hookID = make_hook_id(info);
-    auto hook = this->createReturnHook<linux_data>(info, &linux_filetracer::memfd_create_file_ret_cb);
+    auto hook = this->createReturnHook<linux_data>(info, &linux_filetracer::memfd_create_file_ret_cb, info->trap->name);
     auto params = libhook::GetTrapParams<linux_data>(hook->trap_);
 
     // Save data
@@ -482,7 +481,6 @@ event_response_t linux_filetracer::memfd_create_file_cb(drakvuf_t drakvuf, drakv
 
     params->flags = parse_flags(flags, linux_memfd_flags, this->m_output_format);
 
-    hook->trap_->name = info->trap->name;
     this->ret_hooks[hookID] = std::move(hook);
 
     return VMI_EVENT_RESPONSE_NONE;

diff --git a/src/plugins/filetracer/win.cpp b/src/plugins/filetracer/win.cpp
@@ -740,7 +740,7 @@ event_response_t win_filetracer::create_file_cb(drakvuf_t drakvuf, drakvuf_trap_
 
     // Create new trap for return callback
     uint64_t hookID = make_hook_id(info);
-    auto hook = this->createReturnHook<win_data>(info, &win_filetracer::create_file_ret_cb);
+    auto hook = this->createReturnHook<win_data>(info, &win_filetracer::create_file_ret_cb, info->trap->name);
     auto params = libhook::GetTrapParams<win_data>(hook->trap_);
 
     // Save data
@@ -756,7 +756,6 @@ event_response_t win_filetracer::create_file_cb(drakvuf_t drakvuf, drakvuf_trap_
     params->desired_access = desired_access;
     params->rsp = ret_addr;
 
-    hook->trap_->name = info->trap->name;
     this->ret_hooks[hookID] = std::move(hook);
 
     return VMI_EVENT_RESPONSE_NONE;
@@ -831,7 +830,7 @@ event_response_t win_filetracer::open_file_cb(drakvuf_t drakvuf, drakvuf_trap_in
     }
 
     uint64_t hookID = make_hook_id(info);
-    auto hook = this->createReturnHook<win_data>(info, &win_filetracer::open_file_ret_cb);
+    auto hook = this->createReturnHook<win_data>(info, &win_filetracer::open_file_ret_cb, info->trap->name);
     auto params = libhook::GetTrapParams<win_data>(hook->trap_);
 
     // Save data
@@ -845,7 +844,6 @@ event_response_t win_filetracer::open_file_cb(drakvuf_t drakvuf, drakvuf_trap_in
     params->desired_access = desired_access;
     params->rsp = ret_addr;
 
-    hook->trap_->name = info->trap->name;
     this->ret_hooks[hookID] = std::move(hook);
 
     return VMI_EVENT_RESPONSE_NONE;
@@ -901,7 +899,7 @@ event_response_t win_filetracer::query_attributes_file_cb(drakvuf_t drakvuf, dra
     addr_t ret_addr = drakvuf_get_function_return_address(drakvuf, info);
 
     uint64_t hookID = make_hook_id(info);
-    auto hook = this->createReturnHook<win_data>(info, &win_filetracer::query_attributes_file_ret_cb);
+    auto hook = this->createReturnHook<win_data>(info, &win_filetracer::query_attributes_file_ret_cb, info->trap->name);
     auto params = libhook::GetTrapParams<win_data>(hook->trap_);
 
     // Save data
@@ -911,7 +909,6 @@ event_response_t win_filetracer::query_attributes_file_cb(drakvuf_t drakvuf, dra
     params->file_information = file_information;
     params->rsp = ret_addr;
 
-    hook->trap_->name = info->trap->name;
     this->ret_hooks[hookID] = std::move(hook);
 
     return VMI_EVENT_RESPONSE_NONE;
@@ -949,7 +946,7 @@ event_response_t win_filetracer::query_full_attributes_file_cb(drakvuf_t drakvuf
     addr_t ret_addr = drakvuf_get_function_return_address(drakvuf, info);
 
     uint64_t hookID = make_hook_id(info);
-    auto hook = this->createReturnHook<win_data>(info, &win_filetracer::query_attributes_file_ret_cb);
+    auto hook = this->createReturnHook<win_data>(info, &win_filetracer::query_attributes_file_ret_cb, info->trap->name);
     auto params = libhook::GetTrapParams<win_data>(hook->trap_);
 
     // Save data
@@ -959,7 +956,6 @@ event_response_t win_filetracer::query_full_attributes_file_cb(drakvuf_t drakvuf
     params->file_information = file_information;
     params->rsp = ret_addr;
 
-    hook->trap_->name = info->trap->name;
     this->ret_hooks[hookID] = std::move(hook);
 
     return VMI_EVENT_RESPONSE_NONE;
@@ -1122,7 +1118,7 @@ event_response_t win_filetracer::query_information_file_cb(drakvuf_t drakvuf, dr
 
         // Create new trap for return callback
         uint64_t hookID = make_hook_id(info);
-        auto hook = this->createReturnHook<win_data>(info, &win_filetracer::query_information_file_ret_cb);
+        auto hook = this->createReturnHook<win_data>(info, &win_filetracer::query_information_file_ret_cb, info->trap->name);
         auto params = libhook::GetTrapParams<win_data>(hook->trap_);
 
         // Save data
@@ -1133,7 +1129,6 @@ event_response_t win_filetracer::query_information_file_cb(drakvuf_t drakvuf, dr
         params->file_information = fileinfo;
         params->file_information_class = fileinfoclass;
 
-        hook->trap_->name = info->trap->name;
         this->ret_hooks[hookID] = std::move(hook);
     }
 

diff --git a/src/plugins/plugins_ex.h b/src/plugins/plugins_ex.h
@@ -476,7 +476,7 @@ class pluginex : public plugin
 
     template<typename Params = PluginResult, typename Callback>
     [[nodiscard]]
-    std::unique_ptr<libhook::ReturnHook> createReturnHook(drakvuf_trap_info* info, Callback cb, int ttl = UNLIMITED_TTL);
+    std::unique_ptr<libhook::ReturnHook> createReturnHook(drakvuf_trap_info* info, Callback cb, const char* display_name = nullptr, int ttl = UNLIMITED_TTL);
 
     template<typename Params = PluginResult, typename Callback>
     [[nodiscard]]
@@ -591,11 +591,11 @@ Plugin* get_trap_plugin(const drakvuf_trap_info_t* info)
 }
 
 template<typename Params, typename Callback>
-std::unique_ptr<libhook::ReturnHook> pluginex::createReturnHook(drakvuf_trap_info* info, Callback cb, int ttl)
+std::unique_ptr<libhook::ReturnHook> pluginex::createReturnHook(drakvuf_trap_info* info, Callback cb, const char* display_name, int ttl)
 {
     static_assert(std::is_base_of_v<PluginResult, Params>, "Params must derive from PluginResult");
 
-    auto hook = libhook::ReturnHook::create<Params>(this->drakvuf, info, this->wrap_plugin_cb(cb), ttl);
+    auto hook = libhook::ReturnHook::create<Params>(this->drakvuf, info, this->wrap_plugin_cb(cb), display_name, ttl);
     if (hook)
     {
         static_cast<Params*>(hook->trap_->data)->plugin_ = this;

diff --git a/src/plugins/procmon/linux.cpp b/src/plugins/procmon/linux.cpp
@@ -380,13 +380,12 @@ event_response_t linux_procmon::do_open_execat_cb(drakvuf_t drakvuf, drakvuf_tra
     // Register return trap
     auto hookID = make_hook_id(info);
 
-    auto ret_hook = createReturnHook<open_execat_data>(info, &linux_procmon::do_open_execat_ret_cb);
+    auto ret_hook = createReturnHook<open_execat_data>(info, &linux_procmon::do_open_execat_ret_cb, "do_open_execat_ret_trap");
     if (!ret_hook)
     {
         PRINT_DEBUG("[PROCMON] Failed to register return trap from do_open_execat.\n");
         return VMI_EVENT_RESPONSE_NONE;
     }
-    ret_hook->trap_->name = "do_open_execat_ret_trap";
     auto ret_params = libhook::GetTrapParams<open_execat_data>(ret_hook->trap_);
     ret_params->data = params_->data;
     this->internal_ret_traps[hookID] = std::move(ret_hook);
@@ -431,7 +430,7 @@ event_response_t linux_procmon::do_execveat_common_cb(drakvuf_t drakvuf, drakvuf
 
     // Create new trap for return callback
     auto hookID = make_hook_id(info);
-    auto hook = this->createReturnHook<execve_data>(info, &linux_procmon::do_execveat_common_ret_cb);
+    auto hook = this->createReturnHook<execve_data>(info, &linux_procmon::do_execveat_common_ret_cb, info->trap->name);
     auto params = libhook::GetTrapParams<execve_data>(hook->trap_);
 
     params->fd = drakvuf_get_function_argument(drakvuf, info, 1);
@@ -450,7 +449,6 @@ event_response_t linux_procmon::do_execveat_common_cb(drakvuf_t drakvuf, drakvuf
     params->thread_name = thread_name ?: "";
     g_free(thread_name);
 
-    hook->trap_->name = info->trap->name;
     this->ret_hooks[hookID] = std::move(hook);
 
     auto open_hook = createSyscallHook<open_execat_data>(this->do_open_execat_name, &linux_procmon::do_open_execat_cb, "do_open_execat_trap");
@@ -570,7 +568,7 @@ event_response_t linux_procmon::send_signal_cb(drakvuf_t drakvuf, drakvuf_trap_i
 
     // Create new trap for return callback
     uint64_t hookID = make_hook_id(info);
-    auto hook = this->createReturnHook<send_signal_data>(info, &linux_procmon::send_signal_ret_cb);
+    auto hook = this->createReturnHook<send_signal_data>(info, &linux_procmon::send_signal_ret_cb, info->trap->name);
     auto params = libhook::GetTrapParams<send_signal_data>(hook->trap_);
 
     // Save data about current process
@@ -584,7 +582,6 @@ event_response_t linux_procmon::send_signal_cb(drakvuf_t drakvuf, drakvuf_trap_i
     params->target_proc_ppid = target_proc_data.ppid;
     params->signal = signal;
 
-    hook->trap_->name = info->trap->name;
     this->ret_hooks[hookID] = std::move(hook);
 
     g_free(const_cast<char*>(target_proc_data.name));
@@ -654,13 +651,12 @@ event_response_t linux_procmon::kernel_clone_cb(drakvuf_t drakvuf, drakvuf_trap_
     }
 
     uint64_t hookID = make_hook_id(info);
-    auto hook = this->createReturnHook<kernel_clone_data>(info, &linux_procmon::kernel_clone_ret_cb);
+    auto hook = this->createReturnHook<kernel_clone_data>(info, &linux_procmon::kernel_clone_ret_cb, info->trap->name);
     auto params = libhook::GetTrapParams<kernel_clone_data>(hook->trap_);
 
     params->flags = flags;
     params->exit_signal = exit_signal;
 
-    hook->trap_->name = info->trap->name;
     this->ret_hooks[hookID] = std::move(hook);
     return VMI_EVENT_RESPONSE_NONE;
 }

diff --git a/src/plugins/rpcmon/rpcmon.cpp b/src/plugins/rpcmon/rpcmon.cpp
@@ -453,13 +453,12 @@ static event_response_t usermode_hook_cb(drakvuf_t drakvuf, drakvuf_trap_info* i
 
     uint64_t hookID = make_hook_id(info);
     auto hook = plugin->createReturnHook<RpcmonReturnHookData>(info,
-            &rpcmon::usermode_return_hook_cb, drakvuf_get_limited_traps_ttl(drakvuf));
+            &rpcmon::usermode_return_hook_cb, target->target_name.data(), drakvuf_get_limited_traps_ttl(drakvuf));
     auto params = libhook::GetTrapParams<RpcmonReturnHookData>(hook->trap_);
 
     params->arguments = std::move(arguments);
     params->target = target;
 
-    hook->trap_->name = target->target_name.c_str();
     plugin->ret_hooks[hookID] = std::move(hook);
 
     return VMI_EVENT_RESPONSE_NONE;

diff --git a/src/plugins/syscalls/linux.cpp b/src/plugins/syscalls/linux.cpp
@@ -312,12 +312,10 @@ event_response_t linux_syscalls::linux_cb(drakvuf_t drakvuf, drakvuf_trap_info_t
     if (this->disable_sysret)
         return VMI_EVENT_RESPONSE_NONE;
 
-    auto hook = this->createReturnHook<linux_syscall_data>(info, &linux_syscalls::linux_ret_cb);
+    auto hook = this->createReturnHook<linux_syscall_data>(info, &linux_syscalls::linux_ret_cb, info->trap->name);
     auto params = libhook::GetTrapParams<linux_syscall_data>(hook->trap_);
     params->setResultCallParams(info);
 
-    hook->trap_->name = info->trap->name;
-
     auto hookID = make_hook_id(info);
     this->ret_hooks[hookID] = std::move(hook);
 
@@ -394,4 +392,4 @@ linux_syscalls::linux_syscalls(drakvuf_t drakvuf, const syscalls_config* config,
 
     if (!this->trap_syscall_table_entries(drakvuf))
         PRINT_DEBUG("[SYSCALLS] Failed to set breakpoints on some syscalls.\n");
-}
+}