GNOME Bugzilla – Bug 349998
GtkPathBar use after free
Last modified: 2011-02-04 16:10:42 UTC
==3228== Invalid read of size 4 ==3228== at 0x4667F19: set_button_image_get_info_cb (gtkpathbar.c:1027) ==3228== by 0x47763C5: execute_callbacks_idle (gtkfilesystemunix.c:415) ==3228== by 0x49B3250: g_idle_dispatch (gmain.c:3924) ==3228== by 0x49B4FC1: g_main_context_dispatch (gmain.c:2043) ==3228== by 0x49B7F8E: g_main_context_iterate (gmain.c:2675) ==3228== by 0x49B84F4: g_main_context_iteration (gmain.c:2734) ==3228== by 0x463A3C3: gtk_main_iteration (gtkmain.c:1083) ==3228== by 0x804BDD0: _shutdown (product_gui.c:520) ==3228== by 0x804CC84: main (product_gui.c:777) ==3228== Address 0x911A210 is 24 bytes inside a block of size 32 free'd ==3228== at 0x4004FEA: free (vg_replace_malloc.c:233) ==3228== by 0x49BC2C0: g_free (gmem.c:187) ==3228== by 0x4667CA5: button_data_free (gtkpathbar.c:1160) ==3228== by 0x49569B0: weak_refs_notify (gobject.c:1442) ==3228== by 0x49A3276: g_datalist_id_set_data_full (gdataset.c:242) ==3228== by 0x4957098: g_object_real_dispose (gobject.c:539) ==3228== by 0x4660750: gtk_object_dispose (gtkobject.c:423) ==3228== by 0x475B1F0: gtk_widget_dispose (gtkwidget.c:6881) ==3228== by 0x495793B: g_object_run_dispose (gobject.c:578) ==3228== by 0x466044D: gtk_object_destroy (gtkobject.c:403) ==3228== by 0x475B3D4: gtk_widget_destroy (gtkwidget.c:2157) ==3228== by 0x4667BE8: gtk_path_bar_forall (gtkpathbar.c:657) ==3228== The easiest fix would appear to be test for cancellation before accessing data->button_data, however this is complicated by gtk_file_system_unix_cancel_operation being a no-op.
Created attachment 70232 [details] [review] Reorder unrefs to avoid use after cancellation
Set the cancelled flag for user bookkeeping. Index: gtk/gtkfilesystemunix.c =================================================================== RCS file: /cvs/gnome/gtk+/gtk/gtkfilesystemunix.c,v retrieving revision 1.77 diff -u -p -r1.77 gtkfilesystemunix.c --- gtk/gtkfilesystemunix.c 6 Jul 2006 05:14:01 -0000 1.77 +++ gtk/gtkfilesystemunix.c 4 Aug 2006 23:14:53 -0000 @@ -988,11 +983,12 @@ gtk_file_system_unix_create_folder (GtkF static void gtk_file_system_unix_cancel_operation (GtkFileSystemHandle *handle) { - /* We don't set "cancelled" to TRUE here, since the actual operation + /* We set "cancelled" to TRUE here, even though the actual operation * is executed in the function itself and not in a callback. So * the operations can never be cancelled (since they will be already * completed at this point. */ + handle->cancelled = TRUE; }
We shouldn't change gtk_file_system_unix_cancel_operation() here, as it is using the semantics as discussed on the mailing list. From looking at the trace and code it feels like the button data structures are destroyed during dispose (because their destroy func is triggered by the dispose of the GtkButton inside the button data), but actually the button data structures should be destroyed during finalize. Do you have an easy way to reproduce this bug (didn't really manage with valgrind)? This way I can analyse it further and come up with a different fix.
Okay, the simplest test case... Use testfilechooserbutton, click on the file open button and change folder. Hit cancel and then close the test app. Then during the draining of the event queue [see patch] valgrind will spit out the warning. So regarding the semantics... http://mail.gnome.org/archives/gtk-devel-list/2005-December/msg00106.html > Yes, the callback is always called. In the callback the cancelled flag > in GtkFileSystemHandle will have to be checked. This flag will only be > TRUE if all of the following conditions are true: > - The user requested the operation to be cancelled. > - The operation was cancelled while being executed. > - The temporary results got rolled back. If the operation was already > completed, we cannot roll back the changes and the cancellation will > thus be ignored. > That means that if you cancel a create folder operation and the folder > was already created, the result cannot be rolled back and the cancelled > flag will be set to FALSE. (Having skimmed the two threads in Nov/Dec05 I didn't see a rationale, though that statement clearly shows you have one. For example, you could also have raised a g_set_error("The operation was cancelled") to represent the failure case.) As it stands, it looks like the data will simply have to hold a reference to the GtkPathBar . --- testfilechooserbutton.c 2006-08-17 17:56:29.000000000 +0100 +++ ,testfilechooserbutton.c 2006-08-17 17:56:10.000000000 +0100 @@ -360,5 +360,14 @@ gtk_widget_destroy (win); + do + { + while (gtk_events_pending ()) + gtk_main_iteration (); + + g_usleep (1000); + } + while (gtk_events_pending ()); + return 0; }
This should be fixed in the latest gtk+ release. Could you check?
Np. I was just waiting for GtkFileChooser to settle before checking, will test ASAP.
Chris, how did the testing go ?
No feedback after comment 6. Assuming this has been fixed by comment 5.