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 405539 - gdk_pixbuf_save() for PNG saver can return FALSE without filling the GError
gdk_pixbuf_save() for PNG saver can return FALSE without filling the GError
Status: RESOLVED FIXED
Product: gdk-pixbuf
Classification: Platform
Component: general
git master
Other All
: Normal normal
: ---
Assigned To: gtk-bugs
gtk-bugs
Depends on:
Blocks:
 
 
Reported: 2007-02-07 22:01 UTC by Daniel Atallah
Modified: 2010-07-10 04:06 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Fill GError if returning FALSE and plug leak (1.16 KB, patch)
2007-02-07 22:03 UTC, Daniel Atallah
none Details | Review
Fill GError if returning FALSE and plug leak (2.38 KB, patch)
2007-02-08 04:06 UTC, Daniel Atallah
none Details | Review
Patch to warn and continue when unrecognized parameters are encountered during a pixbuf save. (Also a tiny JPEG saver leak fix) (2.06 KB, patch)
2007-02-16 17:18 UTC, Daniel Atallah
none Details | Review

Description Daniel Atallah 2007-02-07 22:01:39 UTC
Please describe the problem:
If gdk_pixbuf_save() is called for the PNG saver with unrecognized parameters (e.g. "compression" for versions prior to 2.8.0), it will return FALSE without filling the GError object.

I also noticed that cleanup was not happening if png_create_write_struct() failed, causing potential leakage if memory had been allocated for the png_text.

A patch fixing these issues is attached.

Steps to reproduce:


Actual results:


Expected results:


Does this happen every time?


Other information:
Comment 1 Daniel Atallah 2007-02-07 22:03:00 UTC
Created attachment 82121 [details] [review]
Fill GError if returning FALSE and plug leak
Comment 2 Daniel Atallah 2007-02-08 04:06:41 UTC
Created attachment 82129 [details] [review]
Fill GError if returning FALSE and plug leak

I noticed the same issue in the JPEG loader, so I updated the patch to include the fix for that too.
Comment 3 Matthias Clasen 2007-02-16 05:38:21 UTC
I have committed the leak fix. 

Wrt. the error handling, I believe that when we added the error handling in the
pixbuf saving code, we decided that invalid parameter values are recoverable runtime errors (reportable through GError) while invalid keys are programming
errors (reportable via g_error, g_warning, etc).
Comment 4 Daniel Atallah 2007-02-16 14:45:45 UTC
The problem with invalid keys being considered programming errors is that when new keys are added (e.g. "compression" being added to the png saver in 2.8.0), the code will compile on previous versions just fine, but will not work.

I know we can't change this retroactively, but it seems like it might be a good idea going forward to use g_warning() to indicate unrecognized keys, but to ignore them.  Otherwise there needs to be version checking when using new parameters.
Comment 5 Matthias Clasen 2007-02-16 14:59:38 UTC
Thats a valid point, although I don't expect new keys for existing loaders to appear at any rate that would be a cause for concern...

Can you cook a patch that ignores unknown keys with a warning ?
Comment 6 Daniel Atallah 2007-02-16 17:18:04 UTC
Created attachment 82688 [details] [review]
Patch to warn and continue when unrecognized parameters are encountered during a pixbuf save. (Also a tiny JPEG saver leak fix)
Comment 7 Matthias Clasen 2007-04-28 14:48:12 UTC
2007-04-28  Matthias Clasen <mclasen@redhat.com>

        * io-png.c:
        * io-jpeg.c: Accept unknown parameters with a warning
        when saving.  (#405539, Daniel Atallah)