From 9d928576e52c0895328b7fd0f0ec1c2f825b00c8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ole=20Andr=C3=A9=20Vadla=20Ravn=C3=A5s?= Date: Mon, 13 Jan 2025 16:33:55 +0100 Subject: [PATCH 1/2] gumjs: Fix crash in Module finalizers Our QuickJS suspend/resume patch isn't elaborate enough to support usage from finalizers. Given that modules are created and destroyed in waves, however, it's probably a bit on the expensive side to suspend/resume JS execution during the destruction of each value. So to avoid both issues we defer the unref using an idle source. The better longer term solution will probably be to introduce a ModuleObserver of sorts that integrates with the platform's runtime loader and manages the lifetime of each Module. This will also allow us to emit signals anytime a Module is added/removed, which is a feature that a lot of agents would benefit from. Kudos to @mrmacete for reporting. --- bindings/gumjs/gumquickmodule.c | 98 ++++++++++++++++++++++++---- bindings/gumjs/gumquickmodule.h | 3 + bindings/gumjs/gumquickscript-priv.h | 13 ++++ bindings/gumjs/gumquickscript.c | 16 ++--- bindings/gumjs/gumv8module.cpp | 85 +++++++++++++++++++++--- bindings/gumjs/gumv8module.h | 3 + 6 files changed, 184 insertions(+), 34 deletions(-) diff --git a/bindings/gumjs/gumquickmodule.c b/bindings/gumjs/gumquickmodule.c index ad050e150..1acfdbe8a 100644 --- a/bindings/gumjs/gumquickmodule.c +++ b/bindings/gumjs/gumquickmodule.c @@ -7,6 +7,7 @@ #include "gumquickmodule.h" #include "gumquickmacros.h" +#include "gumquickscript-priv.h" typedef struct _GumQuickMatchContext GumQuickMatchContext; typedef struct _GumQuickModuleFilter GumQuickModuleFilter; @@ -28,6 +29,8 @@ struct _GumQuickModuleFilter GumQuickModule * parent; }; +static gboolean gum_quick_module_do_unrefs (gpointer user_data); + GUMJS_DECLARE_FUNCTION (gumjs_module_load) GUMJS_DECLARE_FUNCTION (gumjs_module_find_global_export_by_name) GUMJS_DECLARE_CONSTRUCTOR (gumjs_module_construct) @@ -130,6 +133,9 @@ _gum_quick_module_init (GumQuickModule * self, self->core = core; + self->pending_unrefs = NULL; + self->unref_source = NULL; + _gum_quick_core_store_module_data (core, "module", self); _gum_quick_create_class (ctx, &gumjs_module_def, core, &self->module_class, @@ -158,6 +164,7 @@ _gum_quick_module_init (GumQuickModule * self, void _gum_quick_module_dispose (GumQuickModule * self) { + g_assert (self->pending_unrefs == NULL); } void @@ -165,6 +172,73 @@ _gum_quick_module_finalize (GumQuickModule * self) { } +static void +gum_quick_module_schedule_unref (GumQuickModule * self, + GObject * object) +{ + GumQuickCore * core = self->core; + + if (_gum_quick_script_get_state (core->script) == GUM_SCRIPT_STATE_UNLOADING) + { + g_object_unref (object); + return; + } + + if (self->pending_unrefs == NULL) + self->pending_unrefs = g_ptr_array_new_with_free_func (g_object_unref); + + g_ptr_array_add (self->pending_unrefs, object); + + if (self->unref_source == NULL) + { + GSource * source; + + source = g_idle_source_new (); + g_source_set_callback (source, gum_quick_module_do_unrefs, self, NULL); + g_source_attach (source, + gum_script_scheduler_get_js_context (core->scheduler)); + self->unref_source = source; + + _gum_quick_core_pin (core); + } +} + +static gboolean +gum_quick_module_do_unrefs (gpointer user_data) +{ + GumQuickModule * self = user_data; + GumQuickCore * core = self->core; + GumQuickScope scope; + + while (TRUE) + { + GPtrArray * pending; + + g_rec_mutex_lock (core->mutex); + + pending = g_steal_pointer (&self->pending_unrefs); + + if (pending == NULL) + { + g_source_unref (self->unref_source); + self->unref_source = NULL; + } + + g_rec_mutex_unlock (core->mutex); + + if (pending == NULL) + break; + + g_ptr_array_unref (pending); + } + + _gum_quick_scope_enter (&scope, core); + _gum_quick_core_unpin (core); + _gum_quick_scope_leave (&scope); + + return G_SOURCE_REMOVE; +} + JSValue _gum_quick_module_new_from_handle (JSContext * ctx, GumModule * module, @@ -253,18 +327,16 @@ GUMJS_DEFINE_CONSTRUCTOR (gumjs_module_construct) GUMJS_DEFINE_FINALIZER (gumjs_module_finalize) { + GumQuickModule * parent; GumModule * m; - GumQuickScope scope = GUM_QUICK_SCOPE_INIT (core); - m = JS_GetOpaque (val, gumjs_get_parent_module (core)->module_class); + parent = gumjs_get_parent_module (core); + + m = JS_GetOpaque (val, parent->module_class); if (m == NULL) return; - _gum_quick_scope_suspend (&scope); - - g_object_unref (m); - - _gum_quick_scope_resume (&scope); + gum_quick_module_schedule_unref (parent, G_OBJECT (m)); } GUMJS_DEFINE_GETTER (gumjs_module_get_name) @@ -786,18 +858,16 @@ GUMJS_DEFINE_CONSTRUCTOR (gumjs_module_map_construct) GUMJS_DEFINE_FINALIZER (gumjs_module_map_finalize) { + GumQuickModule * parent; GumModuleMap * m; - GumQuickScope scope = GUM_QUICK_SCOPE_INIT (core); - m = JS_GetOpaque (val, gumjs_get_parent_module (core)->module_map_class); + parent = gumjs_get_parent_module (core); + + m = JS_GetOpaque (val, parent->module_map_class); if (m == NULL) return; - _gum_quick_scope_suspend (&scope); - - g_object_unref (m); - - _gum_quick_scope_resume (&scope); + gum_quick_module_schedule_unref (parent, G_OBJECT (m)); } GUMJS_DEFINE_GETTER (gumjs_module_map_get_handle) diff --git a/bindings/gumjs/gumquickmodule.h b/bindings/gumjs/gumquickmodule.h index c19a1cc51..54428c4a2 100644 --- a/bindings/gumjs/gumquickmodule.h +++ b/bindings/gumjs/gumquickmodule.h @@ -17,6 +17,9 @@ struct _GumQuickModule { GumQuickCore * core; + GPtrArray * pending_unrefs; + GSource * unref_source; + JSClassID module_class; JSClassID module_map_class; }; diff --git a/bindings/gumjs/gumquickscript-priv.h b/bindings/gumjs/gumquickscript-priv.h index af2b12b3c..4b5839fc6 100644 --- a/bindings/gumjs/gumquickscript-priv.h +++ b/bindings/gumjs/gumquickscript-priv.h @@ -13,8 +13,21 @@ G_BEGIN_DECLS +typedef guint GumScriptState; typedef struct _GumQuickWorker GumQuickWorker; +enum _GumScriptState +{ + GUM_SCRIPT_STATE_CREATED, + GUM_SCRIPT_STATE_LOADING, + GUM_SCRIPT_STATE_LOADED, + GUM_SCRIPT_STATE_UNLOADING, + GUM_SCRIPT_STATE_UNLOADED +}; + +G_GNUC_INTERNAL GumScriptState _gum_quick_script_get_state ( + GumQuickScript * self); + G_GNUC_INTERNAL GumQuickWorker * _gum_quick_script_make_worker ( GumQuickScript * self, const gchar * url, JSValue on_message); G_GNUC_INTERNAL GumQuickWorker * _gum_quick_worker_ref ( diff --git a/bindings/gumjs/gumquickscript.c b/bindings/gumjs/gumquickscript.c index 9941c413c..c9d991384 100644 --- a/bindings/gumjs/gumquickscript.c +++ b/bindings/gumjs/gumquickscript.c @@ -34,7 +34,6 @@ # include "gumquickdatabase.h" #endif -typedef guint GumScriptState; typedef struct _GumUnloadNotifyCallback GumUnloadNotifyCallback; typedef void (* GumUnloadNotifyFunc) (GumQuickScript * self, gpointer user_data); @@ -96,15 +95,6 @@ enum PROP_BACKEND }; -enum _GumScriptState -{ - GUM_SCRIPT_STATE_CREATED, - GUM_SCRIPT_STATE_LOADING, - GUM_SCRIPT_STATE_LOADED, - GUM_SCRIPT_STATE_UNLOADING, - GUM_SCRIPT_STATE_UNLOADED -}; - struct _GumUnloadNotifyCallback { GumUnloadNotifyFunc func; @@ -1092,6 +1082,12 @@ gum_quick_emit_data_free (GumEmitData * d) g_slice_free (GumEmitData, d); } +GumScriptState +_gum_quick_script_get_state (GumQuickScript * self) +{ + return self->state; +} + GumQuickWorker * _gum_quick_script_make_worker (GumQuickScript * self, const gchar * url, diff --git a/bindings/gumjs/gumv8module.cpp b/bindings/gumjs/gumv8module.cpp index 08600e9a7..64fa39789 100644 --- a/bindings/gumjs/gumv8module.cpp +++ b/bindings/gumjs/gumv8module.cpp @@ -8,6 +8,7 @@ #include "gumv8macros.h" #include "gumv8matchcontext.h" +#include "gumv8script-priv.h" #include #include @@ -71,6 +72,8 @@ struct GumV8ModuleFilter GumV8Module * module; }; +static gboolean gum_v8_module_do_unrefs (gpointer user_data); + GUMJS_DECLARE_FUNCTION (gumjs_module_load) GUMJS_DECLARE_FUNCTION (gumjs_module_find_global_export_by_name) GUMJS_DECLARE_GETTER (gumjs_module_get_name) @@ -183,6 +186,9 @@ _gum_v8_module_init (GumV8Module * self, self->core = core; + self->pending_unrefs = NULL; + self->unref_source = NULL; + auto module = External::New (isolate, self); auto klass = _gum_v8_create_class ("Module", nullptr, scope, module, isolate); @@ -245,6 +251,8 @@ _gum_v8_module_realize (GumV8Module * self) void _gum_v8_module_dispose (GumV8Module * self) { + g_assert (self->pending_unrefs == NULL); + g_hash_table_unref (self->values); self->values = NULL; @@ -278,6 +286,71 @@ _gum_v8_module_finalize (GumV8Module * self) { } +static void +gum_v8_module_schedule_unref (GumV8Module * self, + GObject * object) +{ + auto core = self->core; + + if (core->script->state == GUM_SCRIPT_STATE_UNLOADING) + { + g_object_unref (object); + return; + } + + if (self->pending_unrefs == NULL) + self->pending_unrefs = g_ptr_array_new_with_free_func (g_object_unref); + + g_ptr_array_add (self->pending_unrefs, object); + + if (self->unref_source == NULL) + { + auto source = g_idle_source_new (); + g_source_set_callback (source, gum_v8_module_do_unrefs, self, NULL); + g_source_attach (source, + gum_script_scheduler_get_js_context (core->scheduler)); + self->unref_source = source; + + _gum_v8_core_pin (core); + } +} + +static gboolean +gum_v8_module_do_unrefs (gpointer user_data) +{ + auto self = (GumV8Module *) user_data; + auto core = self->core; + + while (TRUE) + { + GPtrArray * pending; + { + Locker locker (core->isolate); + + pending = (GPtrArray *) g_steal_pointer (&self->pending_unrefs); + + if (pending == NULL) + { + g_source_unref (self->unref_source); + self->unref_source = NULL; + } + } + + if (pending == NULL) + break; + + g_ptr_array_unref (pending); + } + + { + ScriptScope scope (core->script); + + _gum_v8_core_unpin (core); + } + + return G_SOURCE_REMOVE; +} + GUMJS_DEFINE_FUNCTION (gumjs_module_load) { gchar * name; @@ -723,11 +796,7 @@ _gum_v8_module_new_take_handle (GumModule * handle, static void gum_v8_module_value_free (GumV8ModuleValue * value) { - { - ScriptUnlocker unlocker (value->module->core); - - g_object_unref (value->handle); - } + gum_v8_module_schedule_unref (value->module, G_OBJECT (value->handle)); delete value->wrapper; @@ -888,11 +957,7 @@ gum_v8_module_map_new (Local wrapper, static void gum_v8_module_map_free (GumV8ModuleMap * map) { - { - ScriptUnlocker unlocker (map->module->core); - - g_object_unref (map->handle); - } + gum_v8_module_schedule_unref (map->module, G_OBJECT (map->handle)); delete map->wrapper; diff --git a/bindings/gumjs/gumv8module.h b/bindings/gumjs/gumv8module.h index 368a0415f..6399c105a 100644 --- a/bindings/gumjs/gumv8module.h +++ b/bindings/gumjs/gumv8module.h @@ -16,6 +16,9 @@ struct GumV8Module GHashTable * values; GHashTable * maps; + GPtrArray * pending_unrefs; + GSource * unref_source; + v8::Global * klass; v8::Global * import_value; From a68a566fee3412f59b8c315cfec485dc5a7fc5b2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ole=20Andr=C3=A9=20Vadla=20Ravn=C3=A5s?= Date: Mon, 13 Jan 2025 20:27:55 +0100 Subject: [PATCH 2/2] module: Speed up NativeModule lifecycle By using a single lock for all Module objects. We should be able to revert this once we've improved our GLib static allocation cleanup patch to use a more suitable data structure for keeping track of mutexes. Kudos to @mrmacete for profiling. --- gum/backend-darwin/gummodule-darwin.c | 22 ++++++++-------------- gum/backend-elf/gummodule-elf.c | 20 ++++++++------------ gum/backend-elf/gummodule-elf.h | 2 -- 3 files changed, 16 insertions(+), 28 deletions(-) diff --git a/gum/backend-darwin/gummodule-darwin.c b/gum/backend-darwin/gummodule-darwin.c index a85aaa387..6a989d9a3 100644 --- a/gum/backend-darwin/gummodule-darwin.c +++ b/gum/backend-darwin/gummodule-darwin.c @@ -15,9 +15,6 @@ #include #include -#define GUM_NATIVE_MODULE_LOCK(o) g_mutex_lock (&(o)->mutex) -#define GUM_NATIVE_MODULE_UNLOCK(o) g_mutex_unlock (&(o)->mutex) - typedef struct _GumEnumerateImportsContext GumEnumerateImportsContext; typedef struct _GumEnumerateExportsContext GumEnumerateExportsContext; typedef struct _GumEnumerateSymbolsContext GumEnumerateSymbolsContext; @@ -32,8 +29,6 @@ struct _GumNativeModule GumMemoryRange range; GumDarwinModuleResolver * resolver; - GMutex mutex; - gpointer cached_handle; gboolean attempted_handle_creation; @@ -118,6 +113,8 @@ G_DEFINE_TYPE_EXTENDED (GumNativeModule, G_IMPLEMENT_INTERFACE (GUM_TYPE_MODULE, gum_native_module_iface_init)) +G_LOCK_DEFINE_STATIC (gum_native_module); + static void gum_native_module_class_init (GumNativeModuleClass * klass) { @@ -149,7 +146,6 @@ gum_native_module_iface_init (gpointer g_iface, static void gum_native_module_init (GumNativeModule * self) { - g_mutex_init (&self->mutex); } static void @@ -157,12 +153,12 @@ gum_native_module_dispose (GObject * object) { GumNativeModule * self = GUM_NATIVE_MODULE (object); - GUM_NATIVE_MODULE_LOCK (self); + G_LOCK (gum_native_module); g_clear_object (&self->cached_darwin_module); g_clear_pointer (&self->cached_handle, dlclose); - GUM_NATIVE_MODULE_UNLOCK (self); + G_UNLOCK (gum_native_module); G_OBJECT_CLASS (gum_native_module_parent_class)->dispose (object); } @@ -172,8 +168,6 @@ gum_native_module_finalize (GObject * object) { GumNativeModule * self = GUM_NATIVE_MODULE (object); - g_mutex_clear (&self->mutex); - g_free (self->path); G_OBJECT_CLASS (gum_native_module_parent_class)->finalize (object); @@ -237,7 +231,7 @@ _gum_native_module_detach_resolver (GumNativeModule * self) gpointer _gum_native_module_get_handle (GumNativeModule * self) { - GUM_NATIVE_MODULE_LOCK (self); + G_LOCK (gum_native_module); if (!self->attempted_handle_creation) { @@ -247,7 +241,7 @@ _gum_native_module_get_handle (GumNativeModule * self) self->cached_handle = dlopen (self->path, RTLD_LAZY); } - GUM_NATIVE_MODULE_UNLOCK (self); + G_UNLOCK (gum_native_module); return self->cached_handle; } @@ -255,7 +249,7 @@ _gum_native_module_get_handle (GumNativeModule * self) GumDarwinModule * _gum_native_module_get_darwin_module (GumNativeModule * self) { - GUM_NATIVE_MODULE_LOCK (self); + G_LOCK (gum_native_module); if (!self->attempted_darwin_module_creation) { @@ -267,7 +261,7 @@ _gum_native_module_get_darwin_module (GumNativeModule * self) gum_darwin_module_ensure_image_loaded (self->cached_darwin_module, NULL); } - GUM_NATIVE_MODULE_UNLOCK (self); + G_UNLOCK (gum_native_module); return self->cached_darwin_module; } diff --git a/gum/backend-elf/gummodule-elf.c b/gum/backend-elf/gummodule-elf.c index 299b3c8dc..3aff5f5a9 100644 --- a/gum/backend-elf/gummodule-elf.c +++ b/gum/backend-elf/gummodule-elf.c @@ -12,9 +12,6 @@ #include -#define GUM_NATIVE_MODULE_LOCK(o) g_mutex_lock (&(o)->mutex) -#define GUM_NATIVE_MODULE_UNLOCK(o) g_mutex_unlock (&(o)->mutex) - typedef struct _GumEnumerateImportsContext GumEnumerateImportsContext; typedef struct _GumEnumerateSymbolsContext GumEnumerateSymbolsContext; typedef struct _GumEnumerateRangesContext GumEnumerateRangesContext; @@ -85,6 +82,8 @@ G_DEFINE_TYPE_EXTENDED (GumNativeModule, G_IMPLEMENT_INTERFACE (GUM_TYPE_MODULE, gum_native_module_iface_init)) +G_LOCK_DEFINE_STATIC (gum_native_module); + static void gum_native_module_class_init (GumNativeModuleClass * klass) { @@ -116,7 +115,6 @@ gum_native_module_iface_init (gpointer g_iface, static void gum_native_module_init (GumNativeModule * self) { - g_mutex_init (&self->mutex); } static void @@ -124,7 +122,7 @@ gum_native_module_dispose (GObject * object) { GumNativeModule * self = GUM_NATIVE_MODULE (object); - GUM_NATIVE_MODULE_LOCK (self); + G_LOCK (gum_native_module); g_clear_object (&self->cached_elf_module); @@ -133,7 +131,7 @@ gum_native_module_dispose (GObject * object) else self->cached_handle = NULL; - GUM_NATIVE_MODULE_UNLOCK (self); + G_UNLOCK (gum_native_module); G_OBJECT_CLASS (gum_native_module_parent_class)->dispose (object); } @@ -143,8 +141,6 @@ gum_native_module_finalize (GObject * object) { GumNativeModule * self = GUM_NATIVE_MODULE (object); - g_mutex_clear (&self->mutex); - g_free (self->path); G_OBJECT_CLASS (gum_native_module_parent_class)->finalize (object); @@ -190,7 +186,7 @@ _gum_native_module_make_handleless (const gchar * path, gpointer _gum_native_module_get_handle (GumNativeModule * self) { - GUM_NATIVE_MODULE_LOCK (self); + G_LOCK (gum_native_module); if (!self->attempted_handle_creation) { @@ -203,7 +199,7 @@ _gum_native_module_get_handle (GumNativeModule * self) } } - GUM_NATIVE_MODULE_UNLOCK (self); + G_UNLOCK (gum_native_module); return self->cached_handle; } @@ -211,7 +207,7 @@ _gum_native_module_get_handle (GumNativeModule * self) GumElfModule * _gum_native_module_get_elf_module (GumNativeModule * self) { - GUM_NATIVE_MODULE_LOCK (self); + G_LOCK (gum_native_module); if (!self->attempted_elf_module_creation) { @@ -221,7 +217,7 @@ _gum_native_module_get_elf_module (GumNativeModule * self) self->range.base_address, NULL); } - GUM_NATIVE_MODULE_UNLOCK (self); + G_UNLOCK (gum_native_module); return self->cached_elf_module; } diff --git a/gum/backend-elf/gummodule-elf.h b/gum/backend-elf/gummodule-elf.h index 52adfb830..e8f95a3da 100644 --- a/gum/backend-elf/gummodule-elf.h +++ b/gum/backend-elf/gummodule-elf.h @@ -31,8 +31,6 @@ struct _GumNativeModule GDestroyNotify create_handle_data_destroy; GDestroyNotify destroy_handle; - GMutex mutex; - gpointer cached_handle; gboolean attempted_handle_creation;