io-png: Clarify and fix the error handling

Neither png_create_read_struct* nor png_create_info_struct can trigger
a longjmp out of libpng. That's only documented as being possible with
png_set_progressive_read_fn, even though, currently, it does not.

The documentation [1] is being a bit simplistic when it says:
  "... you will need to update the jmpbuf field every time you enter a
   new routine that will call a png_*() function"

On closer look, the documentation for png_create_read_struct* [2] and
png_create_info_struct [3] clarify that they return NULL on error and
don't longjmp. This is borne out by the libpng code, comments and
examples.

In light of this, the assumption that the error callback would set the
GError when either png_create_read_struct* or png_create_info_struct
fails is wrong. These functions can only fail if there was a request
to allocate an absurdly large amount of memory that exceeds one of the
limits encoded in libpng; or if the memory allocator returns NULL; or
if there was a ABI mismatch caused by compiling and running against
incompatible libpng versions. All those are either programming or
integration errors or something extremely catastrophic - normally one
wouldn't bother handling them as run-time failures. However, since the
libpng documentation repeatedly mentions that the return value from
these functions should be checked, it's wise to do so.

The current location of the setjmp is confusing to the reader because
one can be misled into thinking that there's no need to check the
value returned by png_create_info_struct, or that a failure will lead
to the error handler and a longjmp. It's better to move it closer to
the fallible png_set_progressive_read_fn.

Fallout from e8d0d8ed3d33ee6cedb75a394c36af3312b310ff

[1] http://www.libpng.org/pub/png/libpng-1.2.5-manual.html#section-3
[2] http://refspecs.linuxbase.org/LSB_4.1.0/LSB-Desktop-generic/LSB-Desktop-generic/libpng12.png.create.read.struct.1.html
[3] http://refspecs.linuxbase.org/LSB_4.1.0/LSB-Desktop-generic/LSB-Desktop-generic/libpng12.png.create.info.struct.1.html

https://gitlab.gnome.org/GNOME/gdk-pixbuf/merge_requests/16

1 files changed, 28 insertions, 8 deletions

diff --git a/gdk-pixbuf/io-png.c b/gdk-pixbuf/io-png.c
index 06cf36595..e39b1ecb7 100644
--- a/gdk-pixbuf/io-png.c
+++ b/gdk-pixbuf/io-png.c
@@ -483,16 +483,19 @@ gdk_pixbuf__png_image_begin_load (GdkPixbufModuleSizeFunc size_func,
 #endif
         if (lc->png_read_ptr == NULL) {
                 g_free(lc);
-                /* error callback should have set the error */
+
+                /* A failure here isn't supposed to call the error
+                 * callback, but it doesn't hurt to be careful.
+                 */
+                if (error && *error == NULL) {
+                        g_set_error_literal (error,
+                                             GDK_PIXBUF_ERROR,
+                                             GDK_PIXBUF_ERROR_INSUFFICIENT_MEMORY,
+                                             _("Couldn’t allocate memory for loading PNG"));
+                }
+
                 return NULL;
         }
-        
-	if (setjmp (png_jmpbuf(lc->png_read_ptr))) {
-                png_destroy_read_struct(&lc->png_read_ptr, &lc->png_info_ptr, NULL);
-                g_free(lc);
-                /* error callback should have set the error */
-                return NULL;
-	}
 
         /* Create the auxiliary context struct */
 
@@ -501,6 +504,23 @@ gdk_pixbuf__png_image_begin_load (GdkPixbufModuleSizeFunc size_func,
         if (lc->png_info_ptr == NULL) {
                 png_destroy_read_struct(&lc->png_read_ptr, NULL, NULL);
                 g_free(lc);
+
+                /* A failure here isn't supposed to call the error
+                 * callback, but it doesn't hurt to be careful.
+                 */
+                if (error && *error == NULL) {
+                        g_set_error_literal (error,
+                                             GDK_PIXBUF_ERROR,
+                                             GDK_PIXBUF_ERROR_INSUFFICIENT_MEMORY,
+                                             _("Couldn’t allocate memory for loading PNG"));
+                }
+
+                return NULL;
+        }
+
+        if (setjmp (png_jmpbuf(lc->png_read_ptr))) {
+                png_destroy_read_struct(&lc->png_read_ptr, &lc->png_info_ptr, NULL);
+                g_free(lc);
                 /* error callback should have set the error */
                 return NULL;
         }