Generate better error message when loading invalid PNGs

Submitted by Adrian Johnson on March 25, 2016, 9:11 p.m.

Details

Message ID 56F5A973.4070203@redneon.com
State New
Series "Generate better error message when loading invalid PNGs"
Headers show

Commit Message

Adrian Johnson March 25, 2016, 9:11 p.m.
On 26/03/16 03:53, Uli Schlachter wrote:
> Based on an idea from Cyril Roelandt, this patch makes cairo generate better
> error messages when cairo_image_surface_create_from_png{,_stream} is called on
> e.g. JPEG files. To do so, we don't let libpng check if the file starts with a
> PNG signature, but do so by hand. On mismatch, a surface with status
> CAIRO_STATUS_READ_ERROR is returned where previously the status was
> CAIRO_STATUS_NO_MEMORY (the status used for all errors in libpng).

What about if it is a PNG file but libpng is unable to decode it? It is
still going to return the unhelpful CAIRO_STATUS_NO_MEMORY. It is also
useful restrict CAIRO_STATUS_READ_ERROR to errors reading the from the
stream.

I think we need an error status for backends that can fail. The attached
patch adds CAIRO_STATUS_PNG_ERROR. I can also write a patch for win32 as
it is returning CAIRO_STATUS_NO_MEMORY for GDI errors.

Patch hide | download patch | download mbox

From ec854c8d55789a3891347679be22643b440335c3 Mon Sep 17 00:00:00 2001
From: Adrian Johnson <ajohnson@redneon.com>
Date: Sat, 26 Mar 2016 07:24:26 +1030
Subject: [PATCH] Add CAIRO_STATUS_PNG_ERROR for errors returned by libpng

---
 src/cairo-device.c        |  1 +
 src/cairo-error-private.h |  1 +
 src/cairo-misc.c          |  2 ++
 src/cairo-png.c           | 10 +++++++---
 src/cairo-region.c        |  1 +
 src/cairo-spans.c         |  1 +
 src/cairo-surface.c       |  1 +
 src/cairo.c               |  3 ++-
 src/cairo.h               |  2 ++
 9 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/src/cairo-device.c b/src/cairo-device.c
index 585a9c1..e270666 100644
--- a/src/cairo-device.c
+++ b/src/cairo-device.c
@@ -159,6 +159,7 @@  _cairo_device_create_in_error (cairo_status_t status)
     case CAIRO_STATUS_INVALID_MESH_CONSTRUCTION:
     case CAIRO_STATUS_DEVICE_FINISHED:
     case CAIRO_STATUS_JBIG2_GLOBAL_MISSING:
+    case CAIRO_STATUS_PNG_ERROR:
     default:
 	_cairo_error_throw (CAIRO_STATUS_NO_MEMORY);
 	return (cairo_device_t *) &_nil_device;
diff --git a/src/cairo-error-private.h b/src/cairo-error-private.h
index 178078a..f94a893 100644
--- a/src/cairo-error-private.h
+++ b/src/cairo-error-private.h
@@ -94,6 +94,7 @@  enum _cairo_int_status {
     CAIRO_INT_STATUS_INVALID_MESH_CONSTRUCTION,
     CAIRO_INT_STATUS_DEVICE_FINISHED,
     CAIRO_INT_STATUS_JBIG2_GLOBAL_MISSING,
+    CAIRO_INT_STATUS_PNG_ERROR,
 
     CAIRO_INT_STATUS_LAST_STATUS,
 
diff --git a/src/cairo-misc.c b/src/cairo-misc.c
index 3c7c959..d1ab7b3 100644
--- a/src/cairo-misc.c
+++ b/src/cairo-misc.c
@@ -158,6 +158,8 @@  cairo_status_to_string (cairo_status_t status)
 	return "the target device has been finished";
     case CAIRO_STATUS_JBIG2_GLOBAL_MISSING:
 	return "CAIRO_MIME_TYPE_JBIG2_GLOBAL_ID used but no CAIRO_MIME_TYPE_JBIG2_GLOBAL data provided";
+    case CAIRO_STATUS_PNG_ERROR:
+	return "error occurred in libpng while reading from or writing to a PNG file";
     default:
     case CAIRO_STATUS_LAST_STATUS:
 	return "<unknown error status>";
diff --git a/src/cairo-png.c b/src/cairo-png.c
index 068617d..b160cac 100644
--- a/src/cairo-png.c
+++ b/src/cairo-png.c
@@ -136,7 +136,7 @@  png_simple_error_callback (png_structp png,
 
     /* default to the most likely error */
     if (*error == CAIRO_STATUS_SUCCESS)
-	*error = _cairo_error (CAIRO_STATUS_NO_MEMORY);
+	*error = _cairo_error (CAIRO_STATUS_PNG_ERROR);
 
 #ifdef PNG_SETJMP_SUPPORTED
     longjmp (png_jmpbuf (png), 1);
@@ -349,7 +349,8 @@  stdio_write_func (png_structp png, png_bytep data, png_size_t size)
  * be allocated for the operation or
  * %CAIRO_STATUS_SURFACE_TYPE_MISMATCH if the surface does not have
  * pixel contents, or %CAIRO_STATUS_WRITE_ERROR if an I/O error occurs
- * while attempting to write the file.
+ * while attempting to write the file, or %CAIRO_STATUS_PNG_ERROR if libpng
+ * returned an error.
  *
  * Since: 1.0
  **/
@@ -417,7 +418,8 @@  stream_write_func (png_structp png, png_bytep data, png_size_t size)
  * successfully.  Otherwise, %CAIRO_STATUS_NO_MEMORY is returned if
  * memory could not be allocated for the operation,
  * %CAIRO_STATUS_SURFACE_TYPE_MISMATCH if the surface does not have
- * pixel contents.
+ * pixel contents, or %CAIRO_STATUS_PNG_ERROR if libpng
+ * returned an error.
  *
  * Since: 1.0
  **/
@@ -744,6 +746,7 @@  read_png (struct png_read_closure_t *png_closure)
  *	%CAIRO_STATUS_NO_MEMORY
  *	%CAIRO_STATUS_FILE_NOT_FOUND
  *	%CAIRO_STATUS_READ_ERROR
+ *      %CAIRO_STATUS_PNG_ERROR
  *
  * Alternatively, you can allow errors to propagate through the drawing
  * operations and check the status on the context upon completion
@@ -799,6 +802,7 @@  cairo_image_surface_create_from_png (const char *filename)
  *
  *	%CAIRO_STATUS_NO_MEMORY
  *	%CAIRO_STATUS_READ_ERROR
+ *      %CAIRO_STATUS_PNG_ERROR
  *
  * Alternatively, you can allow errors to propagate through the drawing
  * operations and check the status on the context upon completion
diff --git a/src/cairo-region.c b/src/cairo-region.c
index 6a51225..6def842 100644
--- a/src/cairo-region.c
+++ b/src/cairo-region.c
@@ -107,6 +107,7 @@  _cairo_region_create_in_error (cairo_status_t status)
     case CAIRO_STATUS_INVALID_MESH_CONSTRUCTION:
     case CAIRO_STATUS_DEVICE_FINISHED:
     case CAIRO_STATUS_JBIG2_GLOBAL_MISSING:
+    case CAIRO_STATUS_PNG_ERROR:
     default:
 	_cairo_error_throw (CAIRO_STATUS_NO_MEMORY);
 	return (cairo_region_t *) &_cairo_region_nil;
diff --git a/src/cairo-spans.c b/src/cairo-spans.c
index 182390c..e8fa7c1 100644
--- a/src/cairo-spans.c
+++ b/src/cairo-spans.c
@@ -128,6 +128,7 @@  _cairo_scan_converter_create_in_error (cairo_status_t status)
     case CAIRO_STATUS_INVALID_MESH_CONSTRUCTION: RETURN_NIL;
     case CAIRO_STATUS_DEVICE_FINISHED: RETURN_NIL;
     case CAIRO_STATUS_JBIG2_GLOBAL_MISSING:
+    case CAIRO_STATUS_PNG_ERROR:
     default:
 	break;
     }
diff --git a/src/cairo-surface.c b/src/cairo-surface.c
index 46f6894..9cf3f09 100644
--- a/src/cairo-surface.c
+++ b/src/cairo-surface.c
@@ -2725,6 +2725,7 @@  _cairo_surface_create_in_error (cairo_status_t status)
     case CAIRO_STATUS_INVALID_MESH_CONSTRUCTION:
     case CAIRO_STATUS_DEVICE_FINISHED:
     case CAIRO_STATUS_JBIG2_GLOBAL_MISSING:
+    case CAIRO_STATUS_PNG_ERROR:
     default:
 	_cairo_error_throw (CAIRO_STATUS_NO_MEMORY);
 	return (cairo_surface_t *) &_cairo_surface_nil;
diff --git a/src/cairo.c b/src/cairo.c
index e3acf4d..05fd86b 100644
--- a/src/cairo.c
+++ b/src/cairo.c
@@ -153,7 +153,8 @@  static const cairo_t _cairo_nil[] = {
     DEFINE_NIL_CONTEXT (CAIRO_STATUS_DEVICE_ERROR),
     DEFINE_NIL_CONTEXT (CAIRO_STATUS_INVALID_MESH_CONSTRUCTION),
     DEFINE_NIL_CONTEXT (CAIRO_STATUS_DEVICE_FINISHED),
-    DEFINE_NIL_CONTEXT (CAIRO_STATUS_JBIG2_GLOBAL_MISSING)
+    DEFINE_NIL_CONTEXT (CAIRO_STATUS_JBIG2_GLOBAL_MISSING),
+    DEFINE_NIL_CONTEXT (CAIRO_STATUS_PNG_ERROR)
 
 };
 COMPILE_TIME_ASSERT (ARRAY_LENGTH (_cairo_nil) == CAIRO_STATUS_LAST_STATUS - 1);
diff --git a/src/cairo.h b/src/cairo.h
index 3104d47..a1808bd 100644
--- a/src/cairo.h
+++ b/src/cairo.h
@@ -292,6 +292,7 @@  typedef struct _cairo_user_data_key {
  * @CAIRO_STATUS_DEVICE_FINISHED: target device has been finished (Since 1.12)
  * @CAIRO_STATUS_JBIG2_GLOBAL_MISSING: %CAIRO_MIME_TYPE_JBIG2_GLOBAL_ID has been used on at least one image
  *   but no image provided %CAIRO_MIME_TYPE_JBIG2_GLOBAL (Since 1.14)
+ * @CAIRO_STATUS_PNG_ERROR: error occurred in libpng while reading from or writing to a PNG file (Since 1.16)
  * @CAIRO_STATUS_LAST_STATUS: this is a special value indicating the number of
  *   status values defined in this enumeration.  When using this value, note
  *   that the version of cairo at run-time may have additional status values
@@ -348,6 +349,7 @@  typedef enum _cairo_status {
     CAIRO_STATUS_INVALID_MESH_CONSTRUCTION,
     CAIRO_STATUS_DEVICE_FINISHED,
     CAIRO_STATUS_JBIG2_GLOBAL_MISSING,
+    CAIRO_STATUS_PNG_ERROR,
 
     CAIRO_STATUS_LAST_STATUS
 } cairo_status_t;
-- 
2.1.4


Comments

Uli Schlachter March 26, 2016, 7:35 a.m.
Am 25.03.2016 um 22:11 schrieb Adrian Johnson:
> On 26/03/16 03:53, Uli Schlachter wrote:
>> Based on an idea from Cyril Roelandt, this patch makes cairo generate better
>> error messages when cairo_image_surface_create_from_png{,_stream} is called on
>> e.g. JPEG files. To do so, we don't let libpng check if the file starts with a
>> PNG signature, but do so by hand. On mismatch, a surface with status
>> CAIRO_STATUS_READ_ERROR is returned where previously the status was
>> CAIRO_STATUS_NO_MEMORY (the status used for all errors in libpng).
> 
> What about if it is a PNG file but libpng is unable to decode it? It is
> still going to return the unhelpful CAIRO_STATUS_NO_MEMORY. It is also
> useful restrict CAIRO_STATUS_READ_ERROR to errors reading the from the
> stream.
> 
> I think we need an error status for backends that can fail. The attached
> patch adds CAIRO_STATUS_PNG_ERROR. I can also write a patch for win32 as
> it is returning CAIRO_STATUS_NO_MEMORY for GDI errors.

Fine with me. Note that we could detect OOM reliably by making libpng use our
own malloc/free wrappers via png_create_{read,write}_struct_2(). However,
perhaps this is a bit excessive.

> @@ -744,6 +746,7 @@ read_png (struct png_read_closure_t *png_closure)
>   *	%CAIRO_STATUS_NO_MEMORY
>   *	%CAIRO_STATUS_FILE_NOT_FOUND
>   *	%CAIRO_STATUS_READ_ERROR
> + *      %CAIRO_STATUS_PNG_ERROR
>   *
>   * Alternatively, you can allow errors to propagate through the drawing
>   * operations and check the status on the context upon completion
> @@ -799,6 +802,7 @@ cairo_image_surface_create_from_png (const char *filename)
>   *
>   *	%CAIRO_STATUS_NO_MEMORY
>   *	%CAIRO_STATUS_READ_ERROR
> + *      %CAIRO_STATUS_PNG_ERROR
>   *
>   * Alternatively, you can allow errors to propagate through the drawing
>   * operations and check the status on the context upon completion

In both of the above hunks: The code around uses a tab while your patch adds spaces.

With the above fixed:

Reviewed-by: Uli Schlachter <psychon@znc.in>

(Should there also be matching unit test, similar to what my patch did?)

Cheers,
Uli