GNOME Bugzilla – Bug 318589
crash reading certain PNG images
Last modified: 2010-07-10 04:04:11 UTC
Steps to reproduce: Stack trace: Other information: Between gtk+ 2.8.4 and 2.8.5 you changed the define LOADER_HEADER_SIZE in gdk-pixbuf/gdk-pixbuf- loader.c from 128 to 1024. This causes crashes in one of my application while loading certain PNG images. Reverting to the old value and recompiling the newest gtk+ (2.8.6 at this moment) makes the crash go away. Please review the code again for all the consequences of this change.
Can you attach an image which causes a crash ?
Created attachment 53336 [details] PNG image that leads to crashes Here is one of the images. Note it is totally black, so its filesize (347 bytes) is smaller than the new value of LOADER_HEADER_SIZE. The former value (128) was lower, so this may a latent bug uncovered by the change.
Doesn't crash for me. Can you show a stacktrace or a simple testcase which reproduces the crash ?
Cannot reproduce it with pixbuf-read from tests. I will try tomorrow to create a testcase. This is Windows, it's hard for me to get a stacktrace. Anyway, for additional weirdness, if I set LOADER_HEADER_SIZE to 347 it doesn't crash, while setting it to 348 it crashes. The coincidence is too big. I hope I don't waste your time.
Ok, found out what it happens, there are several bugs in 'gdk_pixbuf_loader_write' all around/related to the 'eaten ' variable: 1. You return FALSE without setting the error. This is contrary to the documentation. In my code I test for FALSE and dereference 'error'. I'm gullible :-) 2. The test (eaten <= 0) itself is wrong and determined my code to suddenly follow this code-path with the new value of LOADER_HEADER_SIZE. Let's split the explanation in two: - the test for equality with 0 is triggered when one calls 'gdk_pixbuf_loader_write' with count=0. This should work, it's legitimate and harmless. - the test for negativity is anyway too late: it is returned from 'gdk_pixbuf_loader_eat_header_write' where its name is 'n_bytes'. Note that there it is by now already used as size argument to 'memcpy'. Also note it's signed and the result of a comparison in the MIN macro with the unsigned 'count'. 'LOADER_HEADER_SIZE - priv->header_buf_offset' is the one that should be tested for negativity. My suggestion is: in 'gdk_pixbuf_loader_write' remove the (eaten <= 0) test and the return path. Please change the title of bug to something else, because this affects any image type. Also the crash happens in my application (by dereferencing 'error') not inside 'gdk_pixbuf', so the bug can be downgraded. Thank you for your time.
Created attachment 53366 [details] [review] Proposed solution to the problems I raised in my previous comment (#5)
Created attachment 53368 [details] [review] a simple patch Can you test if the simple patch above fixes your problem ?
Yes, it solves the problem indeed. But the issue of lack of error setting on the 'eaten <= 0' code-path remains. The documentation of 'gdk_pixbuf_loader_write' states: "If FALSE is returned, error will be set to an error from the GDK_PIXBUF_ERROR or G_FILE_ERROR domains." Maybe this code-path is impossible now, but then, maybe the code could be clarified/simplified. Anyway, I learned my lesson and from now on I will check all the error pointers even if the functions return error codes. Thank you for all your great work on GTK+ et al.
With the patch, gdk_pixbuf_loader_eat_write() is not called with count == 0 anymore, and the only way it can return 0 is if gdk_pixbuf_loader_load_module() returns FALSE, in which case it should have set the error.
You're right, of course. Please commit your patch.
2005-10-12 Matthias Clasen <mclasen@redhat.com> * gdk-pixbuf-loader.c (gdk_pixbuf_loader_write): Only call gdk_pixbuf_loader_eat_header_write() when count > 0. (#318589, Bogdan Nicula)