GNOME Bugzilla – Bug 673409
g_resource_open_stream / g_resource_lookup_data may return stale data pointer
Last modified: 2012-04-16 20:40:37 UTC
Created attachment 211199 [details] [review] Copy out data instead of just referencing it The do_lookup helper function used by these two API calls uses g_variant_get_data to fill in the data pointer. However, the g_variant_get_data documentation says: 'The returned data must not be freed; it remains valid for as long as value exists.' The GVariant passed to g_variant_get_data isn't guaranteed to exist after do_lookup, and at least in the byte-swapped resource case, it indeed doesn't, leading to use-after-free (valgrind output below) and cascaded failure, e.g. breaking the CSS parsing for the Adwaita theme in GTK+ 3.4. The attached patch fixes the use-after-free, but I suspect it introduces leaks in other cases. But I hope this'll give the glib developers enough information to come up with a proper fix. ==10823== Command: gtk3-demo ==10823== ==10823== Invalid read of size 4 ==10823== at 0xFFBBA30: memcpy (in /usr/lib/valgrind/vgpreload_memcheck-ppc32-linux.so) ==10823== by 0xF4EC8F3: g_memory_input_stream_read (string3.h:52) ==10823== by 0xF4E58CF: g_input_stream_read (ginputstream.c:204) ==10823== by 0xF4FBB77: g_resource_file_input_stream_read (gresourcefile.c:801) ==10823== by 0xF4E58CF: g_input_stream_read (ginputstream.c:204) ==10823== by 0xF4CCF5F: g_file_load_contents (gfile.c:6234) ==10823== by 0xFBD6BA7: gtk_css_provider_load_internal (gtkcssprovider.c:2388) ==10823== by 0xFBD668F: gtk_css_provider_load_internal (gtkcssprovider.c:1876) ==10823== by 0xFBD776B: gtk_css_provider_load_from_path (gtkcssprovider.c:2541) ==10823== by 0xFBD7AD3: gtk_css_provider_get_named (gtkcssprovider.c:2718) ==10823== by 0xFD04E4B: settings_update_theme (gtksettings.c:2883) ==10823== by 0xFD0507F: gtk_settings_get_for_screen (gtksettings.c:1567) ==10823== Address 0x414c454 is 8,196 bytes inside a block of size 10,050 free'd ==10823== at 0xFFB7E44: free (in /usr/lib/valgrind/vgpreload_memcheck-ppc32-linux.so) ==10823== by 0xF128437: standard_free (gmem.c:98) ==10823== by 0xF128737: g_free (gmem.c:252) ==10823== by 0xF0F49A7: g_bytes_unref (gbytes.c:293) ==10823== by 0xF163CD3: g_variant_unref (gvariant-core.c:635) ==10823== by 0xF4FA3D7: do_lookup.isra.2 (gresource.c:322) ==10823== by 0xF4FA733: g_resource_open_stream (gresource.c:359) ==10823== by 0xF4FAD17: g_resources_open_stream (gresource.c:662) ==10823== by 0xF4FB8DF: g_resource_file_read (gresourcefile.c:545) ==10823== by 0xF4C5943: g_file_read (gfile.c:1481) ==10823== by 0xF4CCF13: g_file_load_contents (gfile.c:6226) ==10823== by 0xFBD6BA7: gtk_css_provider_load_internal (gtkcssprovider.c:2388)
Did you do anything special in testgtk3? I can't repro, with latest glib/gtk/adwaita master. Also, looking at the trace this should be impossible. g_variant_unref (value) in the trace shows it's the _final_ unref of the variant's data, but value = gvdb_table_get_value (resource->table, path) and therefore its data is owned by resource->table which lives as long as the GResource. Maybe there's a bogus extra unref somewhere?
(In reply to comment #1) I think you missed the 'byte-swapped resource' part. I'm running on a big endian architecture, but /usr/share/themes/Adwaita/gtk-3.0/gtk.gresource is little endian. In that case, gvdb_table_get_value creates a new GVariant with g_variant_byteswap.
I can confirm Michel Dänzer's patch fixes problems I have with gedit and nautilus in Ubuntu 12.04 (they don't show menus). It also fixes a problem with gnome-terminal (the backspace/arrow keys don't work).
Forgot to say this is using PowerPC. This is the bug I raised on launchpad https://bugs.launchpad.net/ubuntu/+source/glib2.0/+bug/961512 .
*** Bug 673697 has been marked as a duplicate of this bug. ***
Created attachment 211642 [details] [review] resources: compiler: Fix resources on big endian architectures
Michel, ojordan, can you test the latest patch ?
Yeah, seems to work, thanks. Looks like that makes gvdb_table_get_value unused; maybe it should be removed, to prevent similar bugs from creeping in again?
Review of attachment 211642 [details] [review]: thanks, please commit
Yep, all good! Many thanks Michel, Christian and Matthias for you work on this.
Just wanted to confirm as requested by ikke who pointed me to this thread as a result of Bug 673697 being a duplicate of this one. My iBook G4 keyboard was fully functional before Ubuntu 12.04 upgrade and trial. The delete key along with the arrow keys all forward the cursor and put gibberish on the screen and do not go backwards. I have tried an external keyboard with the same result. Other text editors don't have this problem. Only the Ubuntu OS Terminal. Thanks.
@waynesbox The patches fix your problem. If you want to solve the problem now then you'll have to compile glib2.0 with the patch Christian Persch added. Otherwise just wait for it to make its way into Ubuntu 12.04.
Is there anything left blocking this? Can it go into a 2.32.x release as well? Also any thoughts on comment #8?
gvdb is a shared git module - it is also used by e.g. dconf; so that function can't be removed without checking all other others.
But yes, the patch can land in master and 2.32; it is marked as accepted-commit-now. Sorry that I forgot to pick it up for 2.32.1, an oversight on my part
*** Bug 674220 has been marked as a duplicate of this bug. ***
Pushed to master and 2-32.