GNOME Bugzilla – Bug 759273
[PATCH] Improve TIFF ops error handling
Last modified: 2016-02-23 21:55:24 UTC
Created attachment 317078 [details] [review] [PATCH] Improve error handling [PATCH] tiff: Improve error handling for loader and saver Do not let libtiff write to stdout and correct the on-error cleanup code (double free / unref).
Created attachment 317760 [details] [review] Second version, avoid printing empty strings...
commit 54865cb38471971217fedfc8232dfc482a0c03c7 Author: Martin Blanchard <tchaik@gmx.com> Date: Mon Dec 21 23:12:08 2015 +0100 tiff: Improve error handling for loader and saver Do not let libtiff write to stdout and correct the on-error cleanup code (double free / unref). https://bugzilla.gnome.org/show_bug.cgi?id=759273
(In reply to Øyvind Kolås from comment #2) > commit 54865cb38471971217fedfc8232dfc482a0c03c7 > Author: Martin Blanchard <tchaik@gmx.com> > Date: Mon Dec 21 23:12:08 2015 +0100 > > tiff: Improve error handling for loader and saver > > Do not let libtiff write to stdout and correct the on-error cleanup > code (double free / unref). > > https://bugzilla.gnome.org/show_bug.cgi?id=759273 This commit introduced new warnings. It seems that 'g_vasprintf' is not declared unless <glib/gprintf.h> is included. GLib doumentation on this particular function is probably not yet updated, but Gcc warns. BTW opening tiff-load.c I noticed that the function 'lseek_to_seek_type' is inconsistent (vim syntax highlighting looks weird) I'd say the SEEK_END case is inverted: https://git.gnome.org/browse/gegl/tree/operations/external/tiff-load.c#n99 and it seems this 'switch' has been copy/pasted in 'seek_in_stream' and also in tiff-save.c
(In reply to Massimo from comment #3) > (In reply to Øyvind Kolås from comment #2) > > commit 54865cb38471971217fedfc8232dfc482a0c03c7 > > Author: Martin Blanchard <tchaik@gmx.com> > > Date: Mon Dec 21 23:12:08 2015 +0100 > > > > tiff: Improve error handling for loader and saver > > > > Do not let libtiff write to stdout and correct the on-error cleanup > > code (double free / unref). > > > > https://bugzilla.gnome.org/show_bug.cgi?id=759273 > > This commit introduced new warnings. It seems > that 'g_vasprintf' is not declared unless <glib/gprintf.h> > is included. GLib doumentation on this particular function > is probably not yet updated, but Gcc warns. > https://build.gimp.org/job/gegl-master/879/parsed_console/job/gegl-master/879/parsed_console/log_content.html#WARNING297 https://build.gimp.org/job/gegl-master/879/parsed_console/job/gegl-master/879/parsed_console/log_content.html#WARNING298
(In reply to Massimo from comment #3) > This commit introduced new warnings. It seems > that 'g_vasprintf' is not declared unless <glib/gprintf.h> > is included. GLib doumentation on this particular function > is probably not yet updated, but Gcc warns. GLib documentation is actually pretty clear about this, my bad... > BTW opening tiff-load.c I noticed that the function > 'lseek_to_seek_type' is inconsistent (vim syntax highlighting > looks weird) I'd say the SEEK_END case is inverted: > > https://git.gnome.org/browse/gegl/tree/operations/external/tiff-load.c#n99 > > and it seems this 'switch' has been copy/pasted in > 'seek_in_stream' and also in tiff-save.c This is obviously another stupid error... Hopefully, libtiff always seek using SEEK_CUR, as far as I could notice.
Created attachment 319054 [details] [review] Add missing include
Created attachment 319055 [details] [review] Fix switch case and return
commit e7e926115f16080a952d578901124e59d23cb2e3 Author: Martin Blanchard <tchaik@gmx.com> Date: Thu Jan 14 22:13:43 2016 +0100 tiff: Fix includes for g_vasprintf and keep gcc quiet https://bugzilla.gnome.org/show_bug.cgi?id=759273
(In reply to Martin Blanchard from comment #5) > (In reply to Massimo from comment #3) > > This commit introduced new warnings. It seems > > that 'g_vasprintf' is not declared unless <glib/gprintf.h> > > is included. GLib doumentation on this particular function > > is probably not yet updated, but Gcc warns. > > GLib documentation is actually pretty clear about this, my bad... > Well, under Description it says: <quote> Note that the functions g_printf(), g_fprintf(), g_sprintf(), g_snprintf(), g_vprintf(), g_vfprintf(), g_vsprintf() and g_vsnprintf() are declared in the header gprintf.h which is not included in glib.h (otherwise using glib.h would drag in stdio.h), so you'll have to explicitly include <glib/gprintf.h> in order to use the GLib printf() functions. </quote> It doesn't mention g_vasprintf > > BTW opening tiff-load.c I noticed that the function > > 'lseek_to_seek_type' is inconsistent (vim syntax highlighting > > looks weird) I'd say the SEEK_END case is inverted: > > > > https://git.gnome.org/browse/gegl/tree/operations/external/tiff-load.c#n99 > > > > and it seems this 'switch' has been copy/pasted in > > 'seek_in_stream' and also in tiff-save.c > > This is obviously another stupid error... Hopefully, libtiff always seek > using SEEK_CUR, as far as I could notice. Well, I was thinking to a compilation failure: that is G_SEEK_END is an enum whose value is independent of SEEK_END, so it is possible that a libc declares SEEK_CUR or SEEK_SET to that value, in that case you'd have a switch with different case's for the same values
Opened a bug for the GLib documentation omission: https://bugzilla.gnome.org/show_bug.cgi?id=760716
(In reply to Martin Blanchard from comment #7) > Created attachment 319055 [details] [review] [review] > Fix switch case and return Anything wrong with the second patch or it has just been forgot?
missed it when applying pending patches, pushed now.