[gdk-pixbuf/wip/rishi/png: 3/3] io-png: Clarify and fix the error handling

[Debarshi Ray <debarshir src gnome org>]

commit c2d50afb5eef8d97c92b1af5182d1894160a47a2
Author: Debarshi Ray <debarshir gnome org>
Date:   Sun Sep 9 20:26:22 2018 +0200

    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

 gdk-pixbuf/io-png.c | 36 ++++++++++++++++++++++++++++--------
 1 file 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;
         }