pngimage: clean up on user/internal errors

pngimage: The code simply exited with a return code of 99 in the event
of a user error including giving pngimage invalid PNG files and an
internal error.  It now attempts to clean up the state before doing so,
matching the normal behaviour.

pngimage: Non-ISO use of setjmp(3) corrected.

pngerror.c: Failure to call png_image_free on a false result from a
png_safe_execute function call fixed.  This was a regression caused by
the 'volatile' clean-up.  Not normally detectable because png_image_free
will often be called by the application.

Reviewed-by: Cosmin Truta <ctruta@gmail.com>
Signed-off-by: John Bowler <jbowler@acm.org>
Signed-off-by: Cosmin Truta <ctruta@gmail.com>

contrib/libtests/pngimage.c

@@ -542,6 +542,7 @@ typedef enum
 struct display
 {
    jmp_buf        error_return;      /* Where to go to on error */
+   error_level    error_code;        /* Set before longjmp */
 
    const char    *filename;          /* The name of the original file */
    const char    *operation;         /* Operation being performed */
@@ -762,7 +763,10 @@ display_log(struct display *dp, error_level level, const char *fmt, ...)
 
    /* Errors cause this routine to exit to the fail code */
    if (level > APP_FAIL || (level > ERRORS && !(dp->options & CONTINUE)))
+   {
+      dp->error_code = level;
       longjmp(dp->error_return, level);
+    }
 }
 
 /* error handler callbacks for libpng */
@@ -1570,18 +1574,19 @@ static int
 do_test(struct display *dp, const char *file)
    /* Exists solely to isolate the setjmp clobbers */
 {
-   int ret = setjmp(dp->error_return);
+   dp->error_code = VERBOSE; /* The "lowest" level */
 
-   if (ret == 0)
+   if (setjmp(dp->error_return) == 0)
    {
       test_one_file(dp, file);
       return 0;
    }
 
-   else if (ret < ERRORS) /* shouldn't longjmp on warnings */
+   else if (dp->error_code < ERRORS) /* shouldn't longjmp on warnings */
-      display_log(dp, INTERNAL_ERROR, "unexpected return code %d", ret);
+      display_log(dp, INTERNAL_ERROR, "unexpected return code %d",
+                  dp->error_code);
 
-   return ret;
+   return dp->error_code;
 }
 
 int
@@ -1681,7 +1686,11 @@ main(int argc, char **argv)
             int ret = do_test(&d, argv[i]);
 
             if (ret > QUIET) /* abort on user or internal error */
+            {
+               display_clean(&d);
+               display_destroy(&d);
                return 99;
+            }
          }
 
          /* Here on any return, including failures, except user/internal issues

pngerror.c

@@ -935,23 +935,37 @@ png_safe_warning(png_structp png_nonconst_ptr, png_const_charp warning_message)
 int /* PRIVATE */
 png_safe_execute(png_imagep image, int (*function)(png_voidp), png_voidp arg)
 {
-   png_voidp saved_error_buf = image->opaque->error_buf;
+   const png_voidp saved_error_buf = image->opaque->error_buf;
    jmp_buf safe_jmpbuf;
-   int result;
 
    /* Safely execute function(arg), with png_error returning back here. */
    if (setjmp(safe_jmpbuf) == 0)
    {
+      int result;
+
       image->opaque->error_buf = safe_jmpbuf;
       result = function(arg);
       image->opaque->error_buf = saved_error_buf;
-      return result;
+
+      if (result)
+         return 1; /* success */
    }
 
-   /* On png_error, return via longjmp, pop the jmpbuf, and free the image. */
+   /* The function failed either because of a caught png_error and a regular
+    * return of false above or because of an uncaught png_error from the
+    * function itself.  Ensure that the error_buf is always set back to the
+    * value saved above:
+    */
    image->opaque->error_buf = saved_error_buf;
-   png_image_free(image);
+
-   return 0;
+   /* On the final false return, when about to return control to the caller, the
+    * image is freed (png_image_free does this check but it is duplicated here
+    * for clarity:
+    */
+   if (saved_error_buf == NULL)
+      png_image_free(image);
+
+   return 0; /* failure */
 }
 #endif /* SIMPLIFIED READ || SIMPLIFIED_WRITE */
 #endif /* READ || WRITE */