From 66d18594c1d0072937ec357910d10d4ed8e46fe6 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] 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 | 95 ++++++++++++++++++++++++---- bindings/gumjs/gumquickmodule.h | 3 + bindings/gumjs/gumquickscript-priv.h | 13 ++++ bindings/gumjs/gumquickscript.c | 16 ++--- bindings/gumjs/gumv8module.cpp | 81 +++++++++++++++++++++--- bindings/gumjs/gumv8module.h | 3 + 6 files changed, 177 insertions(+), 34 deletions(-) diff --git a/bindings/gumjs/gumquickmodule.c b/bindings/gumjs/gumquickmodule.c index ad050e150..7aec927f2 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; + g_queue_init (&self->pending_unrefs); + 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 (g_queue_is_empty (&self->pending_unrefs)); } void @@ -165,6 +172,70 @@ _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; + } + + g_queue_push_tail (&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) + { + GObject * object; + + g_rec_mutex_lock (core->mutex); + + object = g_queue_pop_head (&self->pending_unrefs); + + if (object == NULL) + { + g_source_unref (self->unref_source); + self->unref_source = NULL; + } + + g_rec_mutex_unlock (core->mutex); + + if (object == NULL) + break; + + g_object_unref (object); + } + + _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 +324,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 +855,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..bcb631d7b 100644 --- a/bindings/gumjs/gumquickmodule.h +++ b/bindings/gumjs/gumquickmodule.h @@ -17,6 +17,9 @@ struct _GumQuickModule { GumQuickCore * core; + GQueue 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..567af75a2 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; + g_queue_init (&self->pending_unrefs); + 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 (g_queue_is_empty (&self->pending_unrefs)); + g_hash_table_unref (self->values); self->values = NULL; @@ -278,6 +286,67 @@ _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; + } + + g_queue_push_tail (&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) + { + GObject * object; + { + Locker locker (core->isolate); + + object = G_OBJECT (g_queue_pop_head (&self->pending_unrefs)); + + if (object == NULL) + { + g_source_unref (self->unref_source); + self->unref_source = NULL; + } + } + if (object == NULL) + break; + + g_object_unref (object); + } + + { + ScriptScope scope (core->script); + + _gum_v8_core_unpin (core); + } + + return G_SOURCE_REMOVE; +} + GUMJS_DEFINE_FUNCTION (gumjs_module_load) { gchar * name; @@ -723,11 +792,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 +953,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..43ab31fe5 100644 --- a/bindings/gumjs/gumv8module.h +++ b/bindings/gumjs/gumv8module.h @@ -16,6 +16,9 @@ struct GumV8Module GHashTable * values; GHashTable * maps; + GQueue pending_unrefs; + GSource * unref_source; + v8::Global * klass; v8::Global * import_value;