diff --git a/0001-object-Prevent-use-after-free-in-signal-connections.patch b/0001-object-Prevent-use-after-free-in-signal-connections.patch new file mode 100644 index 0000000..b7cb48f --- /dev/null +++ b/0001-object-Prevent-use-after-free-in-signal-connections.patch @@ -0,0 +1,236 @@ +From 476cc84aa177c7c73e3f5d9568140294a20aaaf1 Mon Sep 17 00:00:00 2001 +From: Philip Chimento +Date: Sun, 11 Jun 2017 12:29:27 -0700 +Subject: [PATCH 1/2] object: Prevent use-after-free in signal connections + +Objects trace their signal connections in order to keep the closures +alive during garbage collection. When invalidating a signal connection, +we must do so in an idle function, since it is illegal to stop tracing a +GC-thing in the middle of GC. + +However, this caused a possible use-after-free if the signal connection +was invalidated, and then the object itself was finalized before the idle +function could be run. + +This refactor avoids the use-after-free by cancelling any pending idle +invalidations in the object's finalizer, and invalidating any remaining +signal connections in such a way that no more idle functions are +scheduled. + +https://bugzilla.gnome.org/show_bug.cgi?id=781799 +--- + gi/object.cpp | 124 +++++++++++++++++++++++++++++++++------------------------- + 1 file changed, 71 insertions(+), 53 deletions(-) + +diff --git a/gi/object.cpp b/gi/object.cpp +index d98c126..db5338b 100644 +--- a/gi/object.cpp ++++ b/gi/object.cpp +@@ -55,6 +55,8 @@ + #include + #include + ++typedef struct _ConnectData ConnectData; ++ + struct ObjectInstance { + GIObjectInfo *info; + GObject *gobj; /* NULL if we are the prototype and not an instance */ +@@ -62,7 +64,7 @@ struct ObjectInstance { + GType gtype; + + /* a list of all signal connections, used when tracing */ +- GList *signals; ++ std::set signals; + + /* the GObjectClass wrapped by this JS Object (only used for + prototypes) */ +@@ -74,11 +76,11 @@ struct ObjectInstance { + unsigned js_object_finalized : 1; + }; + +-typedef struct { ++struct _ConnectData { + ObjectInstance *obj; +- GList *link; + GClosure *closure; +-} ConnectData; ++ unsigned idle_invalidate_id; ++}; + + static std::stack object_init_list; + static GHashTable *class_init_properties; +@@ -93,7 +95,7 @@ static std::set dissociate_list; + GJS_DEFINE_PRIV_FROM_JS(ObjectInstance, gjs_object_instance_class) + + static void disassociate_js_gobject (GObject *gobj); +-static void invalidate_all_signals (ObjectInstance *priv); ++ + typedef enum { + SOME_ERROR_OCCURRED = false, + NO_SUCH_G_PROPERTY, +@@ -1220,6 +1222,21 @@ associate_js_gobject (JSContext *context, + } + + static void ++invalidate_all_signals(ObjectInstance *priv) ++{ ++ /* Can't loop directly through the items, since invalidating an item's ++ * closure might have the effect of removing the item from the set in the ++ * invalidate notifier */ ++ while (!priv->signals.empty()) { ++ /* This will also free cd, through the closure invalidation mechanism */ ++ ConnectData *cd = *priv->signals.begin(); ++ g_closure_invalidate(cd->closure); ++ /* Erase element if not already erased */ ++ priv->signals.erase(cd); ++ } ++} ++ ++static void + disassociate_js_gobject(GObject *gobj) + { + ObjectInstance *priv = get_object_qdata(gobj); +@@ -1369,43 +1386,44 @@ GJS_NATIVE_CONSTRUCTOR_DECLARE(object_instance) + } + + static void +-invalidate_all_signals(ObjectInstance *priv) +-{ +- GList *iter, *next; +- +- for (iter = priv->signals; iter; ) { +- ConnectData *cd = (ConnectData*) iter->data; +- next = iter->next; +- +- /* This will also free cd and iter, through +- the closure invalidation mechanism */ +- g_closure_invalidate(cd->closure); +- +- iter = next; +- } +-} +- +-static void + object_instance_trace(JSTracer *tracer, + JSObject *obj) + { + ObjectInstance *priv; +- GList *iter; + + priv = (ObjectInstance *) JS_GetPrivate(obj); + if (priv == NULL) + return; + +- for (iter = priv->signals; iter; iter = iter->next) { +- ConnectData *cd = (ConnectData *) iter->data; +- ++ for (ConnectData *cd : priv->signals) + gjs_closure_trace(cd->closure, tracer); +- } + + for (auto vfunc : priv->vfuncs) + vfunc->js_function.trace(tracer, "ObjectInstance::vfunc"); + } + ++/* Removing the signal connection data from the list means that the object stops ++ * tracing the JS function objects belonging to the closures. Incremental GC ++ * does not allow that in the middle of a garbage collection. Therefore, we must ++ * do it in an idle handler. ++ */ ++static gboolean ++signal_connection_invalidate_idle(void *user_data) ++{ ++ auto cd = static_cast(user_data); ++ cd->obj->signals.erase(cd); ++ g_slice_free(ConnectData, cd); ++ return G_SOURCE_REMOVE; ++} ++ ++static void ++signal_connection_invalidated(void *data, ++ GClosure *closure) ++{ ++ auto cd = static_cast(data); ++ cd->idle_invalidate_id = g_idle_add(signal_connection_invalidate_idle, cd); ++} ++ + static void + object_instance_finalize(JSFreeOp *fop, + JSObject *obj) +@@ -1429,7 +1447,31 @@ object_instance_finalize(JSFreeOp *fop, + bool had_toggle_up; + bool had_toggle_down; + +- invalidate_all_signals (priv); ++ /* We must invalidate all signal connections now, instead of in an idle ++ * handler, because the object will not exist anymore when we get ++ * around to the idle function. We originally needed to defer these ++ * invalidations to an idle function since the object needs to continue ++ * tracing its signal connections while GC is going on. However, once ++ * the object is finalized, it will not be tracing them any longer ++ * anyway, so it's safe to do them now. ++ * ++ * This is basically the same as invalidate_all_signals(), but does not ++ * defer the invalidation to an idle handler. ++ */ ++ for (ConnectData *cd : priv->signals) { ++ /* First remove any pending invalidation, we are doing it now. */ ++ if (cd->idle_invalidate_id > 0) ++ g_source_remove(cd->idle_invalidate_id); ++ ++ /* We also have to remove the invalidate notifier, which would ++ * otherwise schedule a new pending invalidation. */ ++ g_closure_remove_invalidate_notifier(cd->closure, cd, ++ signal_connection_invalidated); ++ ++ g_closure_invalidate(cd->closure); ++ g_slice_free(ConnectData, cd); ++ } ++ priv->signals.clear(); + + if (G_UNLIKELY (priv->gobj->ref_count <= 0)) { + g_error("Finalizing proxy for an already freed object of type: %s.%s\n", +@@ -1563,29 +1605,6 @@ gjs_lookup_object_prototype(JSContext *context, + return proto; + } + +-/* Removing the signal connection data from the list means that the object stops +- * tracing the JS function objects belonging to the closures. Incremental GC +- * does not allow that in the middle of a garbage collection. Therefore, we must +- * do it in an idle handler. +- */ +-static gboolean +-signal_connection_invalidate_idle(void *user_data) +-{ +- ConnectData *connect_data = (ConnectData *) user_data; +- +- connect_data->obj->signals = g_list_delete_link(connect_data->obj->signals, +- connect_data->link); +- g_slice_free(ConnectData, connect_data); +- return G_SOURCE_REMOVE; +-} +- +-static void +-signal_connection_invalidated(void *data, +- GClosure *closure) +-{ +- g_idle_add(signal_connection_invalidate_idle, data); +-} +- + static bool + real_connect_func(JSContext *context, + unsigned argc, +@@ -1639,9 +1658,8 @@ real_connect_func(JSContext *context, + goto out; + + connect_data = g_slice_new(ConnectData); +- priv->signals = g_list_prepend(priv->signals, connect_data); ++ priv->signals.insert(connect_data); + connect_data->obj = priv; +- connect_data->link = priv->signals; + /* This is a weak reference, and will be cleared when the closure is invalidated */ + connect_data->closure = closure; + g_closure_add_invalidate_notifier(closure, connect_data, signal_connection_invalidated); +-- +2.13.0 + diff --git a/0002-util-root-Allow-GjsMaybeOwned-DestroyNotify-to-free.patch b/0002-util-root-Allow-GjsMaybeOwned-DestroyNotify-to-free.patch new file mode 100644 index 0000000..addc533 --- /dev/null +++ b/0002-util-root-Allow-GjsMaybeOwned-DestroyNotify-to-free.patch @@ -0,0 +1,164 @@ +From 834eede633dff16910ca69f3d0b4876a99c65ba2 Mon Sep 17 00:00:00 2001 +From: Philip Chimento +Date: Sun, 11 Jun 2017 12:59:12 -0700 +Subject: [PATCH 2/2] util-root: Allow GjsMaybeOwned::DestroyNotify to free + +In the case of a closure, the GjsMaybeOwned object is embedded as part of +struct Closure. The context destroy notify callback will invalidate the +closure, which frees the GjsMaybeOwned object, causing a use-after-free +when the callback returns. + +This patch gives the callback a boolean return value; it should return +true if it has freed the GjsMaybeOwned object and false if it does not. +If the callback returns true, then the GjsMaybeOwned object will be +considered invalid from then on. + +https://bugzilla.gnome.org/show_bug.cgi?id=781799 +--- + gi/closure.cpp | 12 +++++++----- + gi/object.cpp | 3 ++- + gjs/jsapi-util-root.h | 10 ++++++---- + test/gjs-test-rooting.cpp | 26 +++++++++++++++++++++++++- + 4 files changed, 40 insertions(+), 11 deletions(-) + +diff --git a/gi/closure.cpp b/gi/closure.cpp +index e3ef0ba..d3d5b2f 100644 +--- a/gi/closure.cpp ++++ b/gi/closure.cpp +@@ -98,7 +98,7 @@ invalidate_js_pointers(GjsClosure *gc) + g_closure_invalidate(&gc->base); + } + +-static void ++static bool + global_context_finalized(JS::HandleObject obj, + void *data) + { +@@ -109,11 +109,13 @@ global_context_finalized(JS::HandleObject obj, + "which calls object %p", + c, c->obj.get()); + +- if (c->obj != NULL) { +- g_assert(c->obj == obj); ++ if (!c->obj) ++ return false; + +- invalidate_js_pointers(gc); +- } ++ g_assert(c->obj == obj); ++ ++ invalidate_js_pointers(gc); ++ return true; + } + + /* Closures have to drop their references to their JS functions in an idle +diff --git a/gi/object.cpp b/gi/object.cpp +index db5338b..0e38bc3 100644 +--- a/gi/object.cpp ++++ b/gi/object.cpp +@@ -915,7 +915,7 @@ wrapped_gobj_dispose_notify(gpointer data, + #endif + } + +-static void ++static bool + gobj_no_longer_kept_alive_func(JS::HandleObject obj, + void *data) + { +@@ -930,6 +930,7 @@ gobj_no_longer_kept_alive_func(JS::HandleObject obj, + priv->keep_alive.reset(); + dissociate_list_remove(priv); + weak_pointer_list.erase(priv); ++ return false; + } + + static void +diff --git a/gjs/jsapi-util-root.h b/gjs/jsapi-util-root.h +index 1bdf029..5e13177 100644 +--- a/gjs/jsapi-util-root.h ++++ b/gjs/jsapi-util-root.h +@@ -110,7 +110,7 @@ struct GjsHeapOperation { + template + class GjsMaybeOwned { + public: +- typedef void (*DestroyNotify)(JS::Handle thing, void *data); ++ using DestroyNotify = bool (*)(JS::Handle thing, void *data); + + private: + bool m_rooted; /* wrapper is in rooted mode */ +@@ -170,9 +170,11 @@ private: + * to remove it. */ + m_has_weakref = false; + +- /* The object is still live across this callback. */ +- if (m_notify) +- m_notify(handle(), m_data); ++ /* The object is still live entering this callback. The callback may ++ * destroy this wrapper if it's part of a larger struct, in which case ++ * it should return true so that we don't touch it */ ++ if (m_notify && m_notify(handle(), m_data)) ++ return; + + reset(); + } +diff --git a/test/gjs-test-rooting.cpp b/test/gjs-test-rooting.cpp +index 240455f..2c2dbdb 100644 +--- a/test/gjs-test-rooting.cpp ++++ b/test/gjs-test-rooting.cpp +@@ -212,7 +212,7 @@ test_maybe_owned_switch_to_unrooted_allows_collection(GjsRootingFixture *fx, + delete obj; + } + +-static void ++static bool + context_destroyed(JS::HandleObject obj, + void *data) + { +@@ -220,6 +220,16 @@ context_destroyed(JS::HandleObject obj, + g_assert_false(fx->notify_called); + g_assert_false(fx->finalized); + fx->notify_called = true; ++ return false; ++} ++ ++static bool ++context_destroyed_trash_object(JS::HandleObject jsobj, ++ void *data) ++{ ++ auto obj = static_cast *>(data); ++ memset(obj, -1, sizeof(GjsMaybeOwned)); ++ return true; + } + + static void +@@ -253,6 +263,18 @@ test_maybe_owned_object_destroyed_after_notify(GjsRootingFixture *fx, + delete obj; + } + ++static void ++test_maybe_owned_notify_callback_trashes_object_and_returns_true(GjsRootingFixture *fx, ++ gconstpointer unused) ++{ ++ auto obj = new GjsMaybeOwned(); ++ obj->root(PARENT(fx)->cx, test_obj_new(fx), context_destroyed_trash_object, ++ obj); ++ ++ gjs_unit_test_destroy_context(PARENT(fx)); ++ /* No assertions, should crash if test fails */ ++} ++ + void + gjs_test_add_tests_for_rooting(void) + { +@@ -288,6 +310,8 @@ gjs_test_add_tests_for_rooting(void) + test_maybe_owned_notify_callback_called_on_context_destroy); + ADD_CONTEXT_DESTROY_TEST("maybe-owned/object-destroyed-after-notify", + test_maybe_owned_object_destroyed_after_notify); ++ ADD_CONTEXT_DESTROY_TEST("maybe-owned/notify-callback-trashes-object-and-returns-true", ++ test_maybe_owned_notify_callback_trashes_object_and_returns_true); + + #undef ADD_CONTEXT_DESTROY_TEST + } +-- +2.13.0 + diff --git a/gjs.spec b/gjs.spec index d3b7957..77d2363 100644 --- a/gjs.spec +++ b/gjs.spec @@ -5,7 +5,7 @@ Name: gjs Version: 1.49.2 -Release: 1%{?dist} +Release: 2%{?dist} Summary: Javascript Bindings for GNOME # The following files contain code from Mozilla which @@ -28,13 +28,17 @@ BuildRequires: intltool BuildRequires: mozjs38-devel >= %{mozjs38_version} BuildRequires: pkgconfig # Bootstrap requirements -BuildRequires: gtk-doc gnome-common +BuildRequires: gtk-doc gnome-common git Requires: glib2%{?_isa} >= %{glib2_version} Requires: gobject-introspection%{?_isa} >= %{gobject_introspection_version} Requires: gtk3%{?_isa} >= %{gtk3_version} Requires: mozjs38%{?_isa} >= %{mozjs38_version} +# https://bugzilla.gnome.org/show_bug.cgi?id=781799 +Patch0: 0001-object-Prevent-use-after-free-in-signal-connections.patch +Patch1: 0002-util-root-Allow-GjsMaybeOwned-DestroyNotify-to-free.patch + %description Gjs allows using GNOME libraries from Javascript. It's based on the Spidermonkey Javascript engine from Mozilla and the GObject introspection @@ -56,7 +60,7 @@ The gjs-tests package contains tests that can be used to verify the functionality of the installed gjs package. %prep -%setup -q +%autosetup -S git %build (if ! test -x configure; then NOCONFIGURE=1 ./autogen.sh; fi; @@ -100,6 +104,10 @@ find %{buildroot} -name '*.la' -exec rm -f {} ';' %{_datadir}/installed-tests %changelog +* Tue Jun 13 2017 Bastien Nocera - 1.49.2-2 ++ gjs-1.49.2-2 +- Add fix for possible use-after-free crasher (bgo #781799) + * Mon Jun 12 2017 Kalev Lember - 1.49.2-1 - Update to 1.49.2