luc14n0/glib
1
0
Fork 0
mirror of https://gitlab.gnome.org/GNOME/glib.git synced 2024-12-25 15:06:14 +01:00
Code Issues Packages Projects Releases Wiki Activity

gobject: fix race in g_object_unref() related to toggle references

Browse Source 
The previous g_object_unref() was racy. There were three places where we
decremented the ref count, but still accessed the object afterwards
(while assuming that somebody else might still hold a reference). For
example:

      if (!g_atomic_int_compare_and_exchange_full ((int *) &object->ref_count,
                                                   old_ref, old_ref - 1,
                                                   &old_ref))
        continue;

      TRACE (GOBJECT_OBJECT_UNREF (object, G_TYPE_FROM_INSTANCE (object), old_ref));

      /* if we went from 2->1 we need to notify toggle refs if any */
      if (old_ref == 2 && OBJECT_HAS_TOGGLE_REF (object))
        {
          /* The last ref being held in this case is owned by the toggle_ref */
          toggle_refs_notify (object, TRUE);
        }

After we decrement the reference count (and gave up our reference), we
are only allowed to access object if we know we have the only possible
reference to it. In particular, if old_ref is larger than 1, then
somebody else holds references and races against destroying object.
The object might be a dangling pointer already.

This is slightly complicated due to toggle references and clearing of
weak-locations.

For toggle references, we must take a lock on the mutex. Luckily, that
is only necessary, when the current reference count is exactly 2.

Note that we emit the TRACE() after the ref count was already decreased.
If another thread unrefs the object, inside the TRACE() we might have a
dangling pointer. That would only be fixable, by emitting the TRACE()
before the actual unref (which has its own problems). This problem
already existed previously.

The change to the test is necessary and correct. Before this patch,
g_object_unref() would call dispose() and decrement the reference count
right after.
In the test case at gobject/tests/reference.c:1108, the reference count
after dispose and decrement is 1. Then it thaws the queue notification,
which emits a property changed signal. The test then proceeds to
reference the object again and notifying the toggle reference.
Previously, the toggle reference was notified 3 times.
After this change, the property changed signal is emitted before
decreasing the reference count. Taking a reference then does not cause
an additional toggle on+off, so in total only one toggle happens.
That accounts for the change in the test. The new behavior is
correct.
This commit is contained in:
Thomas Haller 2023-12-19 09:30:29 +01:00
parent 7292726931
commit 408dc69186
2 changed files with 195 additions and 137 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

315
gobject/gobject.c
View File

@ -3652,6 +3652,13 @@ toggle_refs_notify (GObject *object,
* this reason, you should only ever use a toggle reference if there
* is important state in the proxy object.
*
* Note that if you unref the object on another thread, then @notify might
* still be invoked after g_object_remove_toggle_ref(), and the object argument
* might be a dangling pointer. If the object is destroyed on other threads,
* you must take care of that yourself.
*
* A g_object_add_toggle_ref() must be released with g_object_remove_toggle_ref().
*
* Since: 2.8
*/
void
@ -3707,6 +3714,11 @@ g_object_add_toggle_ref (GObject *object,
* Removes a reference added with g_object_add_toggle_ref(). The
* reference count of the object is decreased by one.
*
* Note that if you unref the object on another thread, then @notify might
* still be invoked after g_object_remove_toggle_ref(), and the object argument
* might be a dangling pointer. If the object is destroyed on other threads,
* you must take care of that yourself.
*
* Since: 2.8
*/
void
@ -3803,165 +3815,212 @@ g_object_unref (gpointer _object)
{
GObject *object = _object;
gint old_ref;
GToggleNotify toggle_notify;
gpointer toggle_data;
GSList **weak_locations;
GObjectNotifyQueue *nqueue;
gboolean do_retry;
GType obj_gtype;
g_return_if_fail (G_IS_OBJECT (object));
/* here we want to atomically do: if (ref_count>1) { ref_count--; return; } */
/* obj_gtype will be needed for TRACE(GOBJECT_OBJECT_UNREF()) later. Note
* that we issue the TRACE() after decrementing the ref-counter. If at that
* point the reference counter does not reach zero, somebody else can race
* and destroy the object.
*
* This means, TRACE() can be called with a dangling object pointer. This
* could only be avoided, by emitting the TRACE before doing the actual
* unref, but at that point we wouldn't know the correct "old_ref" value.
* Maybe this should change.
*
* Anyway. At that later point we can also no longer safely get the GType for
* the TRACE(). Do it now.
*/
obj_gtype = G_TYPE_FROM_INSTANCE (object);
(void) obj_gtype;
old_ref = g_atomic_int_get (&object->ref_count);
retry_atomic_decrement1:
while (old_ref > 1)
retry_beginning:
if (old_ref > 2)
{
/* valid if last 2 refs are owned by this call to unref and the toggle_ref */
/* We have many references. If we can decrement the ref counter, we are done. */
if (!g_atomic_int_compare_and_exchange_full ((int *) &object->ref_count,
old_ref, old_ref - 1,
&old_ref))
continue;
TRACE (GOBJECT_OBJECT_UNREF (object, G_TYPE_FROM_INSTANCE (object), old_ref));
/* if we went from 2->1 we need to notify toggle refs if any */
if (old_ref == 2 && OBJECT_HAS_TOGGLE_REF (object))
{
/* The last ref being held in this case is owned by the toggle_ref */
toggle_refs_notify (object, TRUE);
}
old_ref, old_ref - 1, &old_ref))
goto retry_beginning;
/* Beware: object might be a dangling pointer. */
TRACE (GOBJECT_OBJECT_UNREF (object, obj_gtype, old_ref));
return;
}
{
GSList **weak_locations;
GObjectNotifyQueue *nqueue;
if (old_ref == 2)
{
/* We are about to return the second-to-last reference. In that case we
* might need to notify a toggle reference.
*
* Note that a g_object_add_toggle_ref() MUST always be released
* via g_object_remove_toggle_ref(). Thus, if we are here with
* an old_ref of 2, then at most one of the references can be
* a toggle reference.
*
* We need to take a lock, to avoid races. */
/* The only way that this object can live at this point is if
* there are outstanding weak references already established
* before we got here.
*
* If there were not already weak references then no more can be
* established at this time, because the other thread would have
* to hold a strong ref in order to call
* g_object_add_weak_pointer() and then we wouldn't be here.
*
* Other GWeakRef's (weak locations) instead may still be added
* before the object is finalized, but in such case we'll unset
* them as part of the qdata removal.
*/
weak_locations = g_datalist_id_get_data (&object->qdata, quark_weak_locations);
G_LOCK (toggle_refs_mutex);
if (weak_locations != NULL)
{
g_rw_lock_writer_lock (&weak_locations_lock);
toggle_notify = toggle_refs_get_notify_unlocked (object, &toggle_data);
/* It is possible that one of the weak references beat us to
* the lock. Make sure the refcount is still what we expected
* it to be.
*/
old_ref = g_atomic_int_get (&object->ref_count);
if (old_ref != 1)
{
g_rw_lock_writer_unlock (&weak_locations_lock);
goto retry_atomic_decrement1;
}
if (!g_atomic_int_compare_and_exchange_full ((int *) &object->ref_count,
old_ref, old_ref - 1, &old_ref))
{
G_UNLOCK (toggle_refs_mutex);
goto retry_beginning;
}
/* We got the lock first, so the object will definitely die
* now. Clear out all the weak references, if they're still set.
*/
weak_locations = g_datalist_id_remove_no_notify (&object->qdata,
quark_weak_locations);
g_clear_pointer (&weak_locations, weak_locations_free_unlocked);
G_UNLOCK (toggle_refs_mutex);
g_rw_lock_writer_unlock (&weak_locations_lock);
}
/* Beware: object might be a dangling pointer. */
TRACE (GOBJECT_OBJECT_UNREF (object, obj_gtype, old_ref));
if (toggle_notify)
toggle_notify (toggle_data, object, TRUE);
return;
}
/* freeze the notification queue, so we don't accidentally emit
* notifications during dispose() and finalize().
*
* The notification queue stays frozen unless the instance acquires
* a reference during dispose(), in which case we thaw it and
* dispatch all the notifications. If the instance gets through
* to finalize(), the notification queue gets automatically
* drained when g_object_finalize() is reached and
* the qdata is cleared.
*/
nqueue = g_object_notify_queue_freeze (object);
if (G_UNLIKELY (old_ref != 1))
{
gboolean object_already_finalized = TRUE;
/* we are about to remove the last reference */
TRACE (GOBJECT_OBJECT_DISPOSE (object, G_TYPE_FROM_INSTANCE (object), 1));
G_OBJECT_GET_CLASS (object)->dispose (object);
TRACE (GOBJECT_OBJECT_DISPOSE_END (object, G_TYPE_FROM_INSTANCE (object), 1));
g_return_if_fail (!object_already_finalized);
return;
}
/* may have been re-referenced meanwhile */
old_ref = g_atomic_int_get ((int *) &object->ref_count);
/* We only have one reference left. Proceed to (maybe) clear weak locations. */
weak_locations = g_datalist_id_get_data (&object->qdata, quark_weak_locations);
while (old_ref > 1)
{
/* valid if last 2 refs are owned by this call to unref and the toggle_ref */
if (weak_locations != NULL)
{
g_rw_lock_writer_lock (&weak_locations_lock);
if (!g_atomic_int_compare_and_exchange_full ((int *) &object->ref_count,
old_ref, old_ref - 1,
&old_ref))
continue;
/* After taking the lock, recheck whether we still have only one reference. */
old_ref = g_atomic_int_get (&object->ref_count);
if (old_ref != 1)
{
g_rw_lock_writer_unlock (&weak_locations_lock);
goto retry_beginning;
}
TRACE (GOBJECT_OBJECT_UNREF (object, G_TYPE_FROM_INSTANCE (object), old_ref));
/* We clear out weak locations. After this, the object cannot get a
* strong reference via a GWeakRef and as the ref count is 1, nobody else
* has a reference anymore.
*/
weak_locations = g_datalist_id_remove_no_notify (&object->qdata, quark_weak_locations);
g_clear_pointer (&weak_locations, weak_locations_free_unlocked);
/* emit all notifications that have been queued during dispose() */
g_object_notify_queue_thaw (object, nqueue, FALSE);
g_rw_lock_writer_unlock (&weak_locations_lock);
}
/* if we went from 2->1 we need to notify toggle refs if any */
if (old_ref == 2 && OBJECT_HAS_TOGGLE_REF (object) &&
g_atomic_int_get ((int *) &object->ref_count) == 1)
{
/* The last ref being held in this case is owned by the toggle_ref */
toggle_refs_notify (object, TRUE);
}
/* At this point, we checked with an atomic read that we only hold only one
* reference. Weak locations are cleared (and toggle references are not to
* be considered in this case). Proceed with dispose().
*
* First, freeze the notification queue, so we don't accidentally emit
* notifications during dispose() and finalize().
*
* The notification queue stays frozen unless the instance acquires a
* reference during dispose(), in which case we thaw it and dispatch all the
* notifications. If the instance gets through to finalize(), the
* notification queue gets automatically drained when g_object_finalize() is
* reached and the qdata is cleared.
*/
nqueue = g_object_notify_queue_freeze (object);
return;
}
TRACE (GOBJECT_OBJECT_DISPOSE (object, G_TYPE_FROM_INSTANCE (object), 1));
G_OBJECT_GET_CLASS (object)->dispose (object);
TRACE (GOBJECT_OBJECT_DISPOSE_END (object, G_TYPE_FROM_INSTANCE (object), 1));
/* we are still in the process of taking away the last ref */
g_datalist_id_set_data (&object->qdata, quark_closure_array, NULL);
g_signal_handlers_destroy (object);
g_datalist_id_set_data (&object->qdata, quark_weak_refs, NULL);
g_datalist_id_set_data (&object->qdata, quark_weak_locations, NULL);
g_datalist_id_set_data (&object->qdata, quark_weak_notifies, NULL);
retry_decrement:
/* Here, old_ref is 1 if we just come from dispose(). If the object was resurrected,
* we can hit `goto retry_decrement` and be here with a larger old_ref. */
/* decrement the last reference */
old_ref = g_atomic_int_add (&object->ref_count, -1);
g_return_if_fail (old_ref > 0);
if (old_ref > 1 && nqueue)
{
/* If the object was resurrected, we need to unfreeze the notify
* queue. */
g_object_notify_queue_thaw (object, nqueue, FALSE);
nqueue = NULL;
}
TRACE (GOBJECT_OBJECT_UNREF (object, G_TYPE_FROM_INSTANCE (object), old_ref));
if (old_ref > 2)
{
if (!g_atomic_int_compare_and_exchange_full ((int *) &object->ref_count,
old_ref, old_ref - 1,
&old_ref))
goto retry_decrement;
/* may have been re-referenced meanwhile */
if (G_LIKELY (old_ref == 1))
{
TRACE (GOBJECT_OBJECT_FINALIZE (object, G_TYPE_FROM_INSTANCE (object)));
G_OBJECT_GET_CLASS (object)->finalize (object);
TRACE (GOBJECT_OBJECT_FINALIZE_END (object, G_TYPE_FROM_INSTANCE (object)));
/* Beware: object might be a dangling pointer. */
TRACE (GOBJECT_OBJECT_UNREF (object, obj_gtype, old_ref));
return;
}
GOBJECT_IF_DEBUG (OBJECTS,
{
gboolean was_present;
if (old_ref == 2)
{
/* If the object was resurrected and the current ref-count is 2, then we
* are about to drop the ref-count to 1. We may need to emit a toggle
* notification. Take a lock and check for that.
*
* In that case, we need a lock to get the toggle notification. */
G_LOCK (toggle_refs_mutex);
toggle_notify = toggle_refs_get_notify_unlocked (object, &toggle_data);
do_retry = !g_atomic_int_compare_and_exchange_full ((int *) &object->ref_count,
old_ref, old_ref - 1,
&old_ref);
G_UNLOCK (toggle_refs_mutex);
/* catch objects not chaining finalize handlers */
G_LOCK (debug_objects);
was_present = g_hash_table_remove (debug_objects_ht, object);
G_UNLOCK (debug_objects);
if (do_retry)
goto retry_decrement;
if (was_present)
g_critical ("Object %p of type %s not finalized correctly.",
object, G_OBJECT_TYPE_NAME (object));
});
g_type_free_instance ((GTypeInstance *) object);
}
else
{
/* The instance acquired a reference between dispose() and
* finalize(), so we need to thaw the notification queue
*/
g_object_notify_queue_thaw (object, nqueue, FALSE);
}
}
/* Beware: object might be a dangling pointer. */
TRACE (GOBJECT_OBJECT_UNREF (object, obj_gtype, old_ref));
if (toggle_notify)
toggle_notify (toggle_data, object, TRUE);
return;
}
/* Drop ref count to zero. */
if (!g_atomic_int_compare_and_exchange_full ((int *) &object->ref_count,
old_ref, old_ref - 1, &old_ref))
goto retry_decrement;
TRACE (GOBJECT_OBJECT_UNREF (object, obj_gtype, old_ref));
/* The object is almost gone. Finalize. */
g_datalist_id_set_data (&object->qdata, quark_closure_array, NULL);
g_signal_handlers_destroy (object);
g_datalist_id_set_data (&object->qdata, quark_weak_refs, NULL);
g_datalist_id_set_data (&object->qdata, quark_weak_locations, NULL);
g_datalist_id_set_data (&object->qdata, quark_weak_notifies, NULL);
TRACE (GOBJECT_OBJECT_FINALIZE (object, G_TYPE_FROM_INSTANCE (object)));
G_OBJECT_GET_CLASS (object)->finalize (object);
TRACE (GOBJECT_OBJECT_FINALIZE_END (object, G_TYPE_FROM_INSTANCE (object)));
GOBJECT_IF_DEBUG (OBJECTS,
{
gboolean was_present;
/* catch objects not chaining finalize handlers */
G_LOCK (debug_objects);
was_present = g_hash_table_remove (debug_objects_ht, object);
G_UNLOCK (debug_objects);
if (was_present)
g_critical ("Object %p of type %s not finalized correctly.",
object, G_OBJECT_TYPE_NAME (object));
});
g_type_free_instance ((GTypeInstance *) object);
}
/**

17
gobject/tests/reference.c
View File

@ -1106,8 +1106,7 @@ test_toggle_ref_and_notify_on_dispose (void)
obj->expected.count = 1;
obj->notify_handler = G_CALLBACK (on_object_notify);
g_object_remove_toggle_ref (G_OBJECT (obj), obj->toggle_notify, NULL);
/* FIXME: adjust the count to 1 when !2377 is in */
g_assert_cmpint (obj->actual.count, ==, 4);
g_assert_cmpint (obj->actual.count, ==, 2);
g_assert_cmpuint (obj->notify_called, ==, 1);
disposed_checker = &obj;
@ -1117,10 +1116,10 @@ test_toggle_ref_and_notify_on_dispose (void)
* notification is happening if notify handler switches to normal reference
*/
obj->disposing_refs = 1;
obj->expected.count = 4;
obj->expected.count = 2;
obj->notify_handler = G_CALLBACK (on_object_notify_switch_to_normal_ref);
g_object_remove_toggle_ref (G_OBJECT (obj), obj->toggle_notify, NULL);
g_assert_cmpint (obj->actual.count, ==, 5);
g_assert_cmpint (obj->actual.count, ==, 2);
g_assert_cmpuint (obj->notify_called, ==, 2);
disposed_checker = &obj;
@ -1131,10 +1130,10 @@ test_toggle_ref_and_notify_on_dispose (void)
*/
obj->disposing_refs = 1;
obj->disposing_refs_all_normal = TRUE;
obj->expected.count = 5;
obj->expected.count = 2;
obj->notify_handler = G_CALLBACK (on_object_notify_switch_to_toggle_ref);
g_object_unref (obj);
g_assert_cmpint (obj->actual.count, ==, 7);
g_assert_cmpint (obj->actual.count, ==, 3);
g_assert_cmpuint (obj->notify_called, ==, 3);
disposed_checker = &obj;
@ -1145,10 +1144,10 @@ test_toggle_ref_and_notify_on_dispose (void)
*/
obj->disposing_refs = 1;
obj->disposing_refs_all_normal = FALSE;
obj->expected.count = 7;
obj->expected.count = 3;
obj->notify_handler = G_CALLBACK (on_object_notify_add_ref);
g_object_remove_toggle_ref (G_OBJECT (obj), obj->toggle_notify, NULL);
g_assert_cmpint (obj->actual.count, ==, 8);
g_assert_cmpint (obj->actual.count, ==, 3);
g_assert_cmpuint (obj->notify_called, ==, 4);
g_object_unref (obj);
@ -1156,7 +1155,7 @@ test_toggle_ref_and_notify_on_dispose (void)
g_object_add_weak_pointer (G_OBJECT (obj), &disposed_checker);
obj->disposing_refs = 0;
obj->expected.count = 9;
obj->expected.count = 4;
g_clear_object (&obj);
g_assert_null (disposed_checker);
}