GNOME Bugzilla – Bug 551706
g_object_unref racy condition can lead to crash
Last modified: 2009-10-06 16:32:47 UTC
Steps to reproduce: 1. A lot of threads doing g_oject_unref on the same object 2. 3. Stack trace:
+ Trace 206603
Other information: This happens if : The object has a refcount of 2 and 2 threads call g_object_unref at the same time : Thread 1 : 1. Executes : if (old_ref > 1) { if (!g_atomic_int_compare_and_exchange (&object->ref_count, old_ref, old_ref - 1)) goto retry_atomic_decrement1; Now let's say there's a context switch and thread 2 executes : Thread 2 : else { /* we are about tp remove the last reference */ G_OBJECT_GET_CLASS (object)->dispose (object) etc.... Thread 2 finishes the call and destroys the object : g_type_free_instance ((GTypeInstance*) object); Now Thread 1 resumes : if (old_ref == 2 && OBJECT_HAS_TOGGLE_REF (object) toggle_refs_notify (object, TRUE); OBJECT_HAS_TOGGLE_REF reads invalid memory as object is not destroyed! In my case this invalid read returns >= 1 and the if returns true... thus calling the toggle_ref_notify and crashes (not that there never was a add_ref_notify in the app) See the following patch to fix the issue...
Created attachment 118459 [details] [review] Patch to fix the crash
Error in my comment :( Read : "OBJECT_HAS_TOGGLE_REF reads invalid memory as object IS destroyed!" sorry
Patch applys to 2.18.0 too
Any comment on this ? After a about a year.. it's a small patch and it prevents crashes...
Comment on attachment 118459 [details] [review] Patch to fix the crash Thanks a lot, the patch indeed fixes the issue where a context switch after the first unref CAS leads to another thread destroying the object before the first can check for TOGGLE refs, in case of an object that has no toggle refs installed. This particular setup is quite complicated though, so I'd like to have a comment next to your patch explaining the issue: >--- glib-2.16.5.orig/gobject/gobject.c 2008-09-10 14:55:33.000000000 -0400 >+++ glib-2.16.5/gobject/gobject.c 2008-09-10 14:58:00.000000000 -0400 >@@ -1752,11 +1752,12 @@ > old_ref = g_atomic_int_get (&object->ref_count); > if (old_ref > 1) > { >+ gboolean toggle_ref = OBJECT_HAS_TOGGLE_REF (object); Make this: gboolean toggle_ref = OBJECT_HAS_TOGGLE_REF (object); // valid if last 2 refs are owned by this call to unref and the toggle_ref > if (!g_atomic_int_compare_and_exchange (&object->ref_count, old_ref, old_ref - 1)) > goto retry_atomic_decrement1; > > /* if we went from 2->1 we need to notify toggle refs if any */ >- if (old_ref == 2 && OBJECT_HAS_TOGGLE_REF (object)) >+ if (old_ref == 2 && toggle_ref) Make this: if (old_ref == 2 && toggle_ref) // the last ref being held in this case is owned by the toggle_ref > toggle_refs_notify (object, TRUE); > } > else >@@ -1769,11 +1770,12 @@ > old_ref = g_atomic_int_get (&object->ref_count); > if (old_ref > 1) > { >+ gboolean toggle_ref = OBJECT_HAS_TOGGLE_REF (object); See above. > if (!g_atomic_int_compare_and_exchange (&object->ref_count, old_ref, old_ref - 1)) > goto retry_atomic_decrement2; > > /* if we went from 2->1 we need to notify toggle refs if any */ >- if (old_ref == 2 && OBJECT_HAS_TOGGLE_REF (object)) >+ if (old_ref == 2 && toggle_ref) See above. > toggle_refs_notify (object, TRUE); > > return;
Cody, can you please take care of applying (and testing) this patch together with my comments please?
Tested this and it worked fine. Committed and pushed to master.
*** Bug 556515 has been marked as a duplicate of this bug. ***