From 15e003a1f7bfc892b92a3058c256164c136ce1bf Mon Sep 17 00:00:00 2001 From: Nowaker Date: Sun, 11 Oct 2026 05:36:40 -0500 Subject: [PATCH 1/2] object: survive late toggle notifications and release outside the queue lock GObject calls a toggle notify after releasing its own locks, so another thread can deliver it after the toggle reference was removed and the wrapper freed, possibly with a freed GObject (see g_object_add_toggle_ref()). Passing the ObjectInstance as notify data let such a notification queue a dangling pointer, which the next queue drain at GC start dereferenced. Register each toggle reference in the ToggleQueue under a never-reused identity and pass that identity as notify data. The notify looks it up under the queue lock and drops the notification before touching the instance or the GObject once the identity is retired. Every teardown path retires the identity and cancels queued toggles before removing the toggle reference. The GC weak pointer sweep held the queue lock while releasing native objects. Releasing a GSettings runs its weak notify, which waits for the settings backend mutex, while the dconf worker holds that mutex and waits for the queue lock to deliver a toggle notify, deadlocking Cinnamon with one thread spinning in ToggleQueue::lock(). Disassociate under the lock and release only after unlocking. AI-Model: anthropic/claude-opus-5-5 AI-Variant: medium AI-Tool: opencode 1.18.34-vt-207-907b3bc518 AI-Platform: linux AI-Harness: Vibeterm e2a734d --- gi/object.cpp | 90 ++++++++++++++++++------- gi/object.h | 12 +++- gi/toggle.cpp | 20 ++++++ gi/toggle.h | 14 ++++ test/gjs-test-toggle-queue.cpp | 116 +++++++++++++++++++++++++++++++++ 5 files changed, 228 insertions(+), 24 deletions(-) diff --git a/gi/object.cpp b/gi/object.cpp index 5a4bfc66..29920d0d 100644 --- a/gi/object.cpp +++ b/gi/object.cpp @@ -2173,9 +2173,10 @@ void ObjectInstance::gobj_dispose_notify() { if (m_uses_toggle_ref) { g_object_ref(m_ptr.get()); - g_object_remove_toggle_ref(m_ptr, wrapped_gobj_toggle_notify, this); - ToggleQueue::get_default()->cancel(this); - wrapped_gobj_toggle_notify(this, m_ptr, TRUE); + retire_toggle_notify_data(); + g_object_remove_toggle_ref(m_ptr, wrapped_gobj_toggle_notify, + toggle_notify_data()); + handle_toggle_notify(true); m_uses_toggle_ref = false; } @@ -2295,10 +2296,39 @@ static void toggle_handler(ObjectInstance* self, } } -void ObjectInstance::wrapped_gobj_toggle_notify(void* instance, GObject*, +void* ObjectInstance::register_toggle_notify_data() { + m_toggle_id = ToggleQueue::get_default()->register_object(this); + return toggle_notify_data(); +} + +void* ObjectInstance::toggle_notify_data() const { + return reinterpret_cast(m_toggle_id); +} + +std::pair ObjectInstance::retire_toggle_notify_data() { + return ToggleQueue::get_default()->retire_object(m_toggle_id, this); +} + +void ObjectInstance::wrapped_gobj_toggle_notify(void* data, GObject*, gboolean is_last_ref) { + // GObject calls the notify after releasing its own locks, so another + // thread can deliver it after the toggle reference was removed and the + // instance freed, with a GObject that may be freed as well (see + // g_object_add_toggle_ref()). The notify data is therefore an identity that + // is never reused, and only the queue lock, held until the notification is + // handled, tells whether it still belongs to a live instance. + auto toggle_queue = ToggleQueue::get_default(); + ObjectInstance* self = toggle_queue->lookup_object( + reinterpret_cast(data)); + if (!self) + return; + + self->handle_toggle_notify(is_last_ref); +} + +void ObjectInstance::handle_toggle_notify(bool is_last_ref) { bool toggle_up_queued, toggle_down_queued; - auto* self = static_cast(instance); + auto* self = this; GjsContextPrivate* gjs = GjsContextPrivate::from_current_context(); if (gjs->destroying()) { @@ -2380,8 +2410,9 @@ void ObjectInstance::release_native_object() { ignore_gobject_finalization(); if (m_uses_toggle_ref && !m_gobj_disposed) { + retire_toggle_notify_data(); g_object_remove_toggle_ref(m_ptr.release(), wrapped_gobj_toggle_notify, - this); + toggle_notify_data()); return; } @@ -2483,15 +2514,28 @@ void ObjectInstance::update_heap_wrapper_weak_pointers(JSTracer* trc, "%zu wrapped GObject(s) to examine", ObjectInstance::num_wrapped_gobjects()); - // Take a lock on the queue till we're done with it, so that we don't - // risk that another thread will queue something else while sweeping - auto locked_queue = ToggleQueue::get_default(); + std::vector disassociated; + { + // Take a lock on the queue till we're done with it, so that we don't + // risk that another thread will queue something else while sweeping + auto locked_queue = ToggleQueue::get_default(); - ObjectInstance::remove_wrapped_gobjects_if( - [&trc](ObjectInstance* instance) -> bool { - return instance->weak_pointer_was_finalized(trc); - }, - std::mem_fn(&ObjectInstance::disassociate_js_gobject)); + ObjectInstance::remove_wrapped_gobjects_if( + [&trc](ObjectInstance* instance) -> bool { + return instance->weak_pointer_was_finalized(trc); + }, + [&disassociated](ObjectInstance* instance) { + instance->disassociate_js_gobject(); + disassociated.push_back(instance); + }); + } + + // Releasing a GObject can run dispose and finalize code that waits for a + // lock held by another thread, while that thread waits for the queue lock + // to deliver a toggle notification. So release them only after unlocking; + // the notifications for the disassociated instances are discarded anyway. + for (ObjectInstance* instance : disassociated) + instance->release_disassociated_js_gobject(); } bool ObjectInstance::weak_pointer_was_finalized(JSTracer* trc) { @@ -2575,7 +2619,8 @@ void ObjectInstance::ensure_uses_toggle_ref(JSContext* cx) { */ m_uses_toggle_ref = true; switch_to_rooted(cx); - g_object_add_toggle_ref(m_ptr, wrapped_gobj_toggle_notify, this); + g_object_add_toggle_ref(m_ptr, wrapped_gobj_toggle_notify, + register_toggle_notify_data()); /* We now have both a ref and a toggle ref, we only want the toggle ref. * This may immediately remove the GC root we just added, since refcount may @@ -2610,8 +2655,7 @@ static void invalidate_closure_collection(T* closures, void* data, void ObjectInstance::disassociate_js_gobject() { bool had_toggle_down, had_toggle_up; - std::tie(had_toggle_down, had_toggle_up) = - ToggleQueue::get_default()->cancel(this); + std::tie(had_toggle_down, had_toggle_up) = retire_toggle_notify_data(); if (had_toggle_up && !had_toggle_down) { g_error( "JS object wrapper for GObject %p (%s) is being released while " @@ -2626,7 +2670,9 @@ void ObjectInstance::disassociate_js_gobject() { // First, remove the wrapper pointer from the wrapped GObject unset_object_qdata(); } +} +void ObjectInstance::release_disassociated_js_gobject() { // Now release all the resources the current wrapper has invalidate_closures(); release_native_object(); @@ -2799,14 +2845,12 @@ ObjectInstance::~ObjectInstance() { invalidate_closures(); - // Do not keep the queue locked here, as we may want to leave the other - // threads to queue toggle events till we're owning the GObject so that - // eventually (once the toggle reference is finally removed) we can be sure - // that no other toggle event will target this (soon dead) wrapper. + // Do not keep the queue locked here, as releasing the GObject may need + // other threads to make progress. Retiring the toggle notify data first + // ensures that no toggle event will target this (soon dead) wrapper. bool had_toggle_up; bool had_toggle_down; - std::tie(had_toggle_down, had_toggle_up) = - ToggleQueue::get_default()->cancel(this); + std::tie(had_toggle_down, had_toggle_up) = retire_toggle_notify_data(); // GObject is not already freed if (m_ptr) { diff --git a/gi/object.h b/gi/object.h index 745a4b0b..0927ad7a 100644 --- a/gi/object.h +++ b/gi/object.h @@ -11,6 +11,7 @@ #include #include +#include // for pair #include #include @@ -28,6 +29,7 @@ #include #include "gi/info.h" +#include "gi/toggle.h" #include "gi/value.h" #include "gi/wrapperutils.h" #include "cjs/auto.h" @@ -331,6 +333,9 @@ class ObjectInstance : public GIWrapperInstance retire_toggle_notify_data(); public: static void context_dispose_notify(void* data, diff --git a/gi/toggle.cpp b/gi/toggle.cpp index e2b0e142..96062d1a 100644 --- a/gi/toggle.cpp +++ b/gi/toggle.cpp @@ -117,6 +117,26 @@ std::pair ToggleQueue::cancel(ObjectInstance* obj) { return {had_toggle_down, had_toggle_up}; } +ToggleQueue::ObjectId ToggleQueue::register_object(ObjectInstance* obj) { + g_assert(owns_lock() && "Unsafe access to queue"); + ObjectId id = ++m_last_object_id; + m_objects.emplace(id, obj); + return id; +} + +ObjectInstance* ToggleQueue::lookup_object(ObjectId id) const { + g_assert(owns_lock() && "Unsafe access to queue"); + auto it = m_objects.find(id); + return it == m_objects.end() ? nullptr : it->second; +} + +std::pair ToggleQueue::retire_object(ObjectId id, + ObjectInstance* obj) { + g_assert(owns_lock() && "Unsafe access to queue"); + m_objects.erase(id); + return cancel(obj); +} + bool ToggleQueue::handle_toggle(Handler handler) { g_assert(owns_lock() && "Unsafe access to queue"); diff --git a/gi/toggle.h b/gi/toggle.h index 25a90117..9dbb4731 100644 --- a/gi/toggle.h +++ b/gi/toggle.h @@ -15,6 +15,7 @@ #include #include #include +#include #include // for pair #include // for gboolean @@ -32,6 +33,7 @@ class ToggleQueue { enum Direction : uint8_t { DOWN, UP }; using Handler = void (*)(ObjectInstance*, Direction); + using ObjectId = uintptr_t; private: friend Gjs::Test::ToggleQueue; @@ -49,6 +51,8 @@ class ToggleQueue { }; std::deque q; + std::unordered_map m_objects; + ObjectId m_last_object_id = 0; std::atomic_bool m_shutdown = ATOMIC_VAR_INIT(false); unsigned m_idle_id = 0; @@ -91,6 +95,16 @@ class ToggleQueue { // Cancels pending toggles and returns whether any were queued. std::pair cancel(ObjectInstance*); + /* A toggle notification can arrive after its toggle reference was removed + * and its instance freed, so the notify data is an identity of the toggle + * reference rather than the instance. Identities are never reused. + * register_object() returns a new identity for @obj, lookup_object() + * returns its instance or nullptr once retired, and retire_object() retires + * it and cancels its pending toggles like cancel(). */ + [[nodiscard]] ObjectId register_object(ObjectInstance* obj); + [[nodiscard]] ObjectInstance* lookup_object(ObjectId id) const; + std::pair retire_object(ObjectId id, ObjectInstance* obj); + /* Pops a toggle from the queue and processes it. Call this if you don't * want to wait for it to be processed in idle time. Returns false if queue * is empty. */ diff --git a/test/gjs-test-toggle-queue.cpp b/test/gjs-test-toggle-queue.cpp index 77876ba8..2a373f9f 100644 --- a/test/gjs-test-toggle-queue.cpp +++ b/test/gjs-test-toggle-queue.cpp @@ -39,6 +39,10 @@ static std::deque> struct ObjectInstance : ::ObjectInstance { using ::ObjectInstance::ensure_uses_toggle_ref; using ::ObjectInstance::new_for_gobject; + using ::ObjectInstance::disassociate_js_gobject; + using ::ObjectInstance::release_disassociated_js_gobject; + using ::ObjectInstance::toggle_notify_data; + using ::ObjectInstance::wrapped_gobj_toggle_notify; using ::ObjectInstance::wrapper_is_rooted; }; @@ -633,6 +637,108 @@ static void test_toggle_queue_object_handle_many_up_and_down( g_assert_true(ToggleQueue::queue().empty()); } +static void* toggle_notify_data(::ObjectInstance* instance) { + return static_cast(instance)->toggle_notify_data(); +} + +static ::ToggleQueue::ObjectId toggle_id(::ObjectInstance* instance) { + return reinterpret_cast<::ToggleQueue::ObjectId>( + toggle_notify_data(instance)); +} + +// GObject reads the toggle notify data under its own lock and calls the notify +// only after releasing it, so the call can arrive late from another thread +static void late_toggle_notify(void* data, GObject* gobj, bool is_last_ref) { + std::thread([data, gobj, is_last_ref] { + ObjectInstance::wrapped_gobj_toggle_notify(data, gobj, is_last_ref); + }).join(); +} + +static void test_toggle_queue_object_notify_other_thread(GjsUnitTestFixture* fx, + const void*) { + auto* instance = new_test_gobject(fx); + + late_toggle_notify(toggle_notify_data(instance), instance->ptr(), false); + assert_equal(ToggleQueue::queue().size(), 1LU); + assert_equal(ToggleQueue::queue().at(0).object, instance); + assert_equal(ToggleQueue::queue().at(0).direction, + ::ToggleQueue::Direction::UP); + + ToggleQueue::get_default()->cancel(instance); +} + +static void test_toggle_queue_object_late_notify_retired(GjsUnitTestFixture* fx, + const void*) { + auto* instance = new_test_gobject(fx); + ToggleQueue::get_default()->retire_object(toggle_id(instance), instance); + + late_toggle_notify(toggle_notify_data(instance), instance->ptr(), false); + g_assert_true(ToggleQueue::queue().empty()); + + // A late DOWN may come with a GObject that was finalized meanwhile + late_toggle_notify(toggle_notify_data(instance), nullptr, true); + g_assert_true(ToggleQueue::queue().empty()); +} + +static void test_toggle_queue_object_late_notify_disassociated( + GjsUnitTestFixture* fx, const void*) { + auto* instance = new_test_gobject(fx); + auto* instance_test = static_cast(instance); + void* data = toggle_notify_data(instance); + AutoUnref reffed{instance->ptr(), TakeOwnership{}}; + + // As the GC sweep does: disassociate under the queue lock, release later + instance_test->disassociate_js_gobject(); + late_toggle_notify(data, reffed, false); + g_assert_true(ToggleQueue::queue().empty()); + + instance_test->release_disassociated_js_gobject(); + late_toggle_notify(data, reffed, false); + g_assert_true(ToggleQueue::queue().empty()); + late_toggle_notify(data, reffed, true); + g_assert_true(ToggleQueue::queue().empty()); +} + +static void test_toggle_queue_object_late_notify_finalized( + GjsUnitTestFixture* fx, const void*) { + auto* instance = new_test_gobject(fx); + void* data = toggle_notify_data(instance); + GObject* gobj = instance->ptr(); + + GWeakRef weak_ref; + g_weak_ref_init(&weak_ref, gobj); + TQ::wait_for_gc(fx); + g_assert_null(g_weak_ref_get(&weak_ref)); + g_weak_ref_clear(&weak_ref); + + // Both the instance and the GObject are freed; neither may be touched + late_toggle_notify(data, gobj, false); + g_assert_true(ToggleQueue::queue().empty()); + late_toggle_notify(data, gobj, true); + g_assert_true(ToggleQueue::queue().empty()); +} + +static void test_toggle_queue_object_late_notify_reused_address( + GjsUnitTestFixture* fx, const void*) { + auto* instance = new_test_gobject(fx); + void* old_data = toggle_notify_data(instance); + ToggleQueue::get_default()->retire_object(toggle_id(instance), instance); + + // A new instance allocated at the address of the retired one + auto new_id = ToggleQueue::get_default()->register_object(instance); + + late_toggle_notify(old_data, instance->ptr(), false); + g_assert_true(ToggleQueue::queue().empty()); + + late_toggle_notify(reinterpret_cast(new_id), instance->ptr(), + false); + assert_equal(ToggleQueue::queue().size(), 1LU); + assert_equal(ToggleQueue::queue().at(0).object, instance); + + ToggleQueue::get_default()->retire_object(new_id, instance); + g_assert_true(ToggleQueue::queue().empty()); +} + void add_tests_for_toggle_queue() { #define ADD_TOGGLE_QUEUE_TEST(path, f) \ g_test_add("/toggle-queue/" path, GjsUnitTestFixture, nullptr, TQ::setup, \ @@ -683,6 +789,16 @@ void add_tests_for_toggle_queue() { test_toggle_queue_object_handle_many_up); ADD_TOGGLE_QUEUE_TEST("object/handle_many_up_and_down", test_toggle_queue_object_handle_many_up_and_down); + ADD_TOGGLE_QUEUE_TEST("object/notify_other_thread", + test_toggle_queue_object_notify_other_thread); + ADD_TOGGLE_QUEUE_TEST("object/late_notify_retired", + test_toggle_queue_object_late_notify_retired); + ADD_TOGGLE_QUEUE_TEST("object/late_notify_disassociated", + test_toggle_queue_object_late_notify_disassociated); + ADD_TOGGLE_QUEUE_TEST("object/late_notify_finalized", + test_toggle_queue_object_late_notify_finalized); + ADD_TOGGLE_QUEUE_TEST("object/late_notify_reused_address", + test_toggle_queue_object_late_notify_reused_address); #undef ADD_TOGGLE_QUEUE_TEST } From 7610bfc9c512f58c7fc37321af278989f05ee92e Mon Sep 17 00:00:00 2001 From: Nowaker Date: Sun, 11 Oct 2026 06:06:56 -0500 Subject: [PATCH 2/2] toggle: refuse to wrap toggle reference identities A late toggle notification is matched to its instance by an identity that must never be reused, or a notification meant for a retired toggle reference would reach a newer instance. Abort instead of wrapping the counter, and test the boundary with the counter set just below its maximum. Describe the late notification tests accurately: the reused address test registers the same pointer again, and the finalized test proves the GObject was freed, not that the C++ instance was deleted separately. AI-Model: anthropic/claude-opus-5-5 AI-Variant: medium AI-Tool: opencode 1.18.34-vt-207-907b3bc518 AI-Platform: linux AI-Harness: Vibeterm 38f6148 --- gi/toggle.cpp | 3 +++ test/gjs-test-toggle-queue.cpp | 33 +++++++++++++++++++++++++++++++-- 2 files changed, 34 insertions(+), 2 deletions(-) diff --git a/gi/toggle.cpp b/gi/toggle.cpp index 96062d1a..bdcc6d50 100644 --- a/gi/toggle.cpp +++ b/gi/toggle.cpp @@ -11,6 +11,7 @@ #include // for find_if #include #include +#include #include // for pair #include "gi/object.h" @@ -119,6 +120,8 @@ std::pair ToggleQueue::cancel(ObjectInstance* obj) { ToggleQueue::ObjectId ToggleQueue::register_object(ObjectInstance* obj) { g_assert(owns_lock() && "Unsafe access to queue"); + if (G_UNLIKELY(m_last_object_id == std::numeric_limits::max())) + g_error("Toggle reference identities exhausted, refusing to reuse one"); ObjectId id = ++m_last_object_id; m_objects.emplace(id, obj); return id; diff --git a/test/gjs-test-toggle-queue.cpp b/test/gjs-test-toggle-queue.cpp index 2a373f9f..7501d77b 100644 --- a/test/gjs-test-toggle-queue.cpp +++ b/test/gjs-test-toggle-queue.cpp @@ -8,6 +8,7 @@ #include #include #include +#include #include #include #include // for tie @@ -60,6 +61,9 @@ struct ToggleQueue { static ::ToggleQueue::Handler handler() { return get_default()->m_toggle_handler; } + static void set_last_object_id(::ToggleQueue::ObjectId id) { + get_default()->m_last_object_id = id; + } }; namespace TQ { @@ -711,7 +715,8 @@ static void test_toggle_queue_object_late_notify_finalized( g_assert_null(g_weak_ref_get(&weak_ref)); g_weak_ref_clear(&weak_ref); - // Both the instance and the GObject are freed; neither may be touched + // The weak ref proves the GObject was freed and the GC retired the toggle + // identity; the late notifications must touch neither late_toggle_notify(data, gobj, false); g_assert_true(ToggleQueue::queue().empty()); late_toggle_notify(data, gobj, true); @@ -724,7 +729,8 @@ static void test_toggle_queue_object_late_notify_reused_address( void* old_data = toggle_notify_data(instance); ToggleQueue::get_default()->retire_object(toggle_id(instance), instance); - // A new instance allocated at the address of the retired one + // Simulates a new instance allocated at the address of the retired one by + // registering the same pointer again auto new_id = ToggleQueue::get_default()->register_object(instance); late_toggle_notify(old_data, instance->ptr(), false); @@ -739,6 +745,27 @@ static void test_toggle_queue_object_late_notify_reused_address( g_assert_true(ToggleQueue::queue().empty()); } +static void test_toggle_queue_object_ids_exhausted(GjsUnitTestFixture* fx, + const void*) { + if (!g_test_subprocess()) { + g_test_trap_subprocess(nullptr, 0, G_TEST_SUBPROCESS_DEFAULT); + g_test_trap_assert_failed(); + g_test_trap_assert_stderr("*identities exhausted*"); + return; + } + + auto* instance = new_test_gobject(fx); + auto max = std::numeric_limits<::ToggleQueue::ObjectId>::max(); + ToggleQueue::set_last_object_id(max - 1); + + auto last_id = ToggleQueue::get_default()->register_object(instance); + assert_equal(last_id, max); + ToggleQueue::get_default()->retire_object(last_id, instance); + + // Wrapping around would hand out identities of retired toggle references + std::ignore = ToggleQueue::get_default()->register_object(instance); +} + void add_tests_for_toggle_queue() { #define ADD_TOGGLE_QUEUE_TEST(path, f) \ g_test_add("/toggle-queue/" path, GjsUnitTestFixture, nullptr, TQ::setup, \ @@ -799,6 +826,8 @@ void add_tests_for_toggle_queue() { test_toggle_queue_object_late_notify_finalized); ADD_TOGGLE_QUEUE_TEST("object/late_notify_reused_address", test_toggle_queue_object_late_notify_reused_address); + ADD_TOGGLE_QUEUE_TEST("object/ids_exhausted", + test_toggle_queue_object_ids_exhausted); #undef ADD_TOGGLE_QUEUE_TEST }