After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 636489 - [valgrind] invalid read in texture cache
[valgrind] invalid read in texture cache
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: general
unspecified
Other Linux
: Normal normal
: ---
Assigned To: gnome-shell-maint
gnome-shell-maint
Depends on:
Blocks:
 
 
Reported: 2010-12-04 23:03 UTC by Colin Walters
Modified: 2010-12-18 18:43 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
StTextureCache: Plug leaks in tiled image loading (2.65 KB, patch)
2010-12-16 22:59 UTC, Colin Walters
committed Details | Review

Description Colin Walters 2010-12-04 23:03:50 UTC
==25108== Invalid read of size 4
==25108==    at 0x4008429: memcpy (mc_replace_strmem.c:628)
==25108==    by 0x4A32375: g_memdup (gstrfuncs.c:131)
==25108==    by 0x4589A2B: _cogl_texture_prepare_for_upload (cogl-texture.c:194)
==25108==    by 0x4592610: _cogl_atlas_texture_new_from_bitmap (cogl-atlas-texture.c:570)
==25108==    by 0x4589FA2: cogl_texture_new_from_bitmap (cogl-texture.c:457)
==25108==    by 0x458A07A: cogl_texture_new_from_data (cogl-texture.c:442)
==25108==    by 0x649CDA8: pixbuf_to_cogl_handle (st-texture-cache.c:728)
==25108==    by 0x649F429: on_sliced_image_loaded (st-texture-cache.c:1162)
==25108==    by 0x48D6E85: g_simple_async_result_complete (gsimpleasyncresult.c:748)
==25108==    by 0x48D6F0D: complete_in_idle_cb_for_thread (gsimpleasyncresult.c:813)
==25108==    by 0x4A0C6D0: g_idle_dispatch (gmain.c:4532)
==25108==    by 0x4A11159: g_main_context_dispatch (gmain.c:2436)
==25108==  Address 0xe870c1c is 140 bytes inside a block of size 32,768 free'd
==25108==    at 0x4005EAD: free (vg_replace_malloc.c:366)
==25108==    by 0x4A18515: g_free (gmem.c:263)
==25108==    by 0xF1D556C: png_free_callback (io-png.c:243)
==25108==    by 0x4A9BD793: png_free (in /usr/lib/libpng12.so.0.44.0)
==25108==    by 0x4A9A5304: ??? (in /usr/lib/libpng12.so.0.44.0)
==25108==    by 0x4A453060: inflateEnd (in /lib/libz.so.1.2.5)
==25108==    by 0x4A9B1989: ??? (in /usr/lib/libpng12.so.0.44.0)
==25108==    by 0x4A9B1B4D: png_destroy_read_struct (in /usr/lib/libpng12.so.0.44.0)
==25108==    by 0xF1D6E78: gdk_pixbuf__png_image_load (io-png.c:353)
==25108==    by 0x4645BA8: _gdk_pixbuf_generic_image_load (gdk-pixbuf-io.c:901)
==25108==    by 0x4645E82: gdk_pixbuf_new_from_file (gdk-pixbuf-io.c:1013)
==25108==    by 0x649D16F: load_sliced_image (st-texture-cache.c:1216)
Comment 1 Colin Walters 2010-12-16 22:59:07 UTC
Created attachment 176565 [details] [review]
StTextureCache: Plug leaks in tiled image loading

We weren't freeing the whole pixbuf, nor were we the GList of
subpixbufs.  Plug both of these leaks in the error handling path
and in the default case.

All of the unreffing/cleanup should happen in the GDestroyNotify
for the result, not some in the handler.
Comment 2 Colin Walters 2010-12-17 17:19:33 UTC
The above doesn't fix the bug, but
http://bugzilla.clutter-project.org/show_bug.cgi?id=2491
probably does.
Comment 3 Owen Taylor 2010-12-17 19:58:05 UTC
Review of attachment 176565 [details] [review]:

Basically looks good.

::: src/st/st-texture-cache.c
@@ +1150,2 @@
 static ClutterActor *
+load_from_pixbuf (GdkPixbuf  *pixbuf)

Accidental whitespace change

@@ +1199,2 @@
 static void
+destroynotify_object_list (gpointer p)

you name this as if this was some naming convention we use... some descriptive name like free_list_and_unref_contents (GList *l) would be clearer.

@@ +1236,3 @@
             {
               g_simple_async_result_set_error (result, G_IO_ERROR, G_IO_ERROR_INVALID_ARGUMENT,
+                                               "Failed to create subpixbuf for sliced image");

OK, but gdk_pixbuf_new_subpixbuf() actually can't return NULL except in the Gdk-Pixbuf-Critical case of invalid input which we are never supposed to try and handle.
Comment 4 Colin Walters 2010-12-18 18:43:45 UTC
Attachment 176565 [details] pushed as bdfc516 - StTextureCache: Plug leaks in tiled image loading