Fwd: Re: [PATCH] Generate better error message when loading invalid PNGs

Submitted by Adrian Johnson on March 26, 2016, 12:17 p.m.

Details

Message ID 56F67DC9.7010101@redneon.com
State New
Headers show
Series "Fwd: Re: [PATCH] Generate better error message when loading invalid PNGs" ( rev: 1 ) in Cairo

Not browsing as part of any series.

Commit Message

Adrian Johnson March 26, 2016, 12:17 p.m.
I forgot to CC the list.

-------- Forwarded Message --------
Subject: Re: [cairo] [PATCH] Generate better error message when loading
invalid PNGs
Date: Sat, 26 Mar 2016 22:35:02 +1030
From: Adrian Johnson <ajohnson@redneon.com>
To: Uli Schlachter <psychon@znc.in>

On 26/03/16 18:05, Uli Schlachter wrote:
> 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.

I also noticed I missed a case statement in cairo-spans.c. I will fix
both issues before pushing.

> With the above fixed:
> 
> Reviewed-by: Uli Schlachter <psychon@znc.in>
> 
> (Should there also be matching unit test, similar to what my patch did?)

Your patch should work with the expected error code changed to
CAIRO_STATUS_PNG_ERROR. I'll leave it for you to push your test.

I've attached three more patches:

- Update the errors in the utils/* files. They have not been updated for
a while.

- Add CAIRO_STATUS_FREETYPE_ERROR for libfreetype errors. It now returns
CAIRO_STATUS_NO_MEMORY for out of memory and CAIRO_STATUS_FREETYPE_ERROR
for any other error.

- Add CAIRO_STATUS_WIN32_GDI_ERROR for GDI errors. I didn't try to
isolate out of memory errors from other errors as it is not clear to me
if a GDI out of memory error means "out of heap memory" or "out of some
internal resource in Windows". If the latter it is more helpful to
return a GDI error than to return NO_MEMORY.

> 
> Cheers,
> Uli
>

Patch hide | download patch | download mbox

From 747cab741cf63f30aa6bc2b787fc13e7e7a2b859 Mon Sep 17 00:00:00 2001
From: Adrian Johnson <ajohnson@redneon.com>
Date: Sat, 26 Mar 2016 22:18:05 +1030
Subject: [PATCH 4/4] Add CAIRO_STATUS_WIN32_GDI_ERROR for GDI errors

---
 src/cairo-device.c                       | 1 +
 src/cairo-error-private.h                | 1 +
 src/cairo-misc.c                         | 2 ++
 src/cairo-region.c                       | 1 +
 src/cairo-spans.c                        | 2 ++
 src/cairo-surface.c                      | 1 +
 src/cairo.c                              | 3 ++-
 src/cairo.h                              | 2 ++
 src/win32/cairo-win32-surface.c          | 6 +-----
 util/cairo-gobject/cairo-gobject-enums.c | 1 +
 util/cairo-script/cairo-script-private.h | 2 +-
 util/cairo-trace/trace.c                 | 1 +
 12 files changed, 16 insertions(+), 7 deletions(-)

diff --git a/src/cairo-device.c b/src/cairo-device.c
index 23f654f..bacf93b 100644
--- a/src/cairo-device.c
+++ b/src/cairo-device.c
@@ -161,6 +161,7 @@  _cairo_device_create_in_error (cairo_status_t status)
     case CAIRO_STATUS_JBIG2_GLOBAL_MISSING:
     case CAIRO_STATUS_PNG_ERROR:
     case CAIRO_STATUS_FREETYPE_ERROR:
+    case CAIRO_STATUS_WIN32_GDI_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 f9dea70..25dac7d 100644
--- a/src/cairo-error-private.h
+++ b/src/cairo-error-private.h
@@ -96,6 +96,7 @@  enum _cairo_int_status {
     CAIRO_INT_STATUS_JBIG2_GLOBAL_MISSING,
     CAIRO_INT_STATUS_PNG_ERROR,
     CAIRO_INT_STATUS_FREETYPE_ERROR,
+    CAIRO_INT_STATUS_WIN32_GDI_ERROR,
 
     CAIRO_INT_STATUS_LAST_STATUS,
 
diff --git a/src/cairo-misc.c b/src/cairo-misc.c
index c1e0971..f4db372 100644
--- a/src/cairo-misc.c
+++ b/src/cairo-misc.c
@@ -162,6 +162,8 @@  cairo_status_to_string (cairo_status_t status)
 	return "error occurred in libpng while reading from or writing to a PNG file";
     case CAIRO_STATUS_FREETYPE_ERROR:
 	return "error occurred in libfreetype";
+    case CAIRO_STATUS_WIN32_GDI_ERROR:
+	return "error occurred in the Windows Graphics Device Interface";
     default:
     case CAIRO_STATUS_LAST_STATUS:
 	return "<unknown error status>";
diff --git a/src/cairo-region.c b/src/cairo-region.c
index daf1f96..b738c44 100644
--- a/src/cairo-region.c
+++ b/src/cairo-region.c
@@ -109,6 +109,7 @@  _cairo_region_create_in_error (cairo_status_t status)
     case CAIRO_STATUS_JBIG2_GLOBAL_MISSING:
     case CAIRO_STATUS_PNG_ERROR:
     case CAIRO_STATUS_FREETYPE_ERROR:
+    case CAIRO_STATUS_WIN32_GDI_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 d2f9407..d20cd5a 100644
--- a/src/cairo-spans.c
+++ b/src/cairo-spans.c
@@ -130,6 +130,7 @@  _cairo_scan_converter_create_in_error (cairo_status_t status)
     case CAIRO_STATUS_JBIG2_GLOBAL_MISSING:
     case CAIRO_STATUS_PNG_ERROR:
     case CAIRO_STATUS_FREETYPE_ERROR:
+    case CAIRO_STATUS_WIN32_GDI_ERROR:
     default:
 	break;
     }
@@ -245,6 +246,7 @@  _cairo_span_renderer_create_in_error (cairo_status_t status)
     case CAIRO_STATUS_JBIG2_GLOBAL_MISSING: RETURN_NIL;
     case CAIRO_STATUS_PNG_ERROR: RETURN_NIL;
     case CAIRO_STATUS_FREETYPE_ERROR: RETURN_NIL;
+    case CAIRO_STATUS_WIN32_GDI_ERROR: RETURN_NIL;
     default:
 	break;
     }
diff --git a/src/cairo-surface.c b/src/cairo-surface.c
index 057e1de..ded146d 100644
--- a/src/cairo-surface.c
+++ b/src/cairo-surface.c
@@ -2727,6 +2727,7 @@  _cairo_surface_create_in_error (cairo_status_t status)
     case CAIRO_STATUS_JBIG2_GLOBAL_MISSING:
     case CAIRO_STATUS_PNG_ERROR:
     case CAIRO_STATUS_FREETYPE_ERROR:
+    case CAIRO_STATUS_WIN32_GDI_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 078c5e2..ec27fe7 100644
--- a/src/cairo.c
+++ b/src/cairo.c
@@ -155,7 +155,8 @@  static const cairo_t _cairo_nil[] = {
     DEFINE_NIL_CONTEXT (CAIRO_STATUS_DEVICE_FINISHED),
     DEFINE_NIL_CONTEXT (CAIRO_STATUS_JBIG2_GLOBAL_MISSING),
     DEFINE_NIL_CONTEXT (CAIRO_STATUS_PNG_ERROR),
-    DEFINE_NIL_CONTEXT (CAIRO_STATUS_FREETYPE_ERROR)
+    DEFINE_NIL_CONTEXT (CAIRO_STATUS_FREETYPE_ERROR),
+    DEFINE_NIL_CONTEXT (CAIRO_STATUS_WIN32_GDI_ERROR)
 
 };
 COMPILE_TIME_ASSERT (ARRAY_LENGTH (_cairo_nil) == CAIRO_STATUS_LAST_STATUS - 1);
diff --git a/src/cairo.h b/src/cairo.h
index 1d2f418..a09d839 100644
--- a/src/cairo.h
+++ b/src/cairo.h
@@ -294,6 +294,7 @@  typedef struct _cairo_user_data_key {
  *   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_FREETYPE_ERROR: error occurred in libfreetype (Since 1.16)
+ * @CAIRO_STATUS_WIN32_GDI_ERROR: error occurred in the Windows Graphics Device Interface (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
@@ -352,6 +353,7 @@  typedef enum _cairo_status {
     CAIRO_STATUS_JBIG2_GLOBAL_MISSING,
     CAIRO_STATUS_PNG_ERROR,
     CAIRO_STATUS_FREETYPE_ERROR,
+    CAIRO_STATUS_WIN32_GDI_ERROR,
 
     CAIRO_STATUS_LAST_STATUS
 } cairo_status_t;
diff --git a/src/win32/cairo-win32-surface.c b/src/win32/cairo-win32-surface.c
index e6862bd..f7285b9 100644
--- a/src/win32/cairo-win32-surface.c
+++ b/src/win32/cairo-win32-surface.c
@@ -122,11 +122,7 @@  _cairo_win32_print_gdi_error (const char *context)
 
     fflush (stderr);
 
-    /* We should switch off of last_status, but we'd either return
-     * CAIRO_STATUS_NO_MEMORY or CAIRO_STATUS_UNKNOWN_ERROR and there
-     * is no CAIRO_STATUS_UNKNOWN_ERROR.
-     */
-    return _cairo_error (CAIRO_STATUS_NO_MEMORY);
+    return _cairo_error (CAIRO_STATUS_WIN32_GDI_ERROR);
 }
 
 cairo_bool_t
diff --git a/util/cairo-gobject/cairo-gobject-enums.c b/util/cairo-gobject/cairo-gobject-enums.c
index cd4a4be..0c50694 100644
--- a/util/cairo-gobject/cairo-gobject-enums.c
+++ b/util/cairo-gobject/cairo-gobject-enums.c
@@ -55,6 +55,7 @@  cairo_gobject_status_get_type (void)
 	  { CAIRO_STATUS_PNG_ERROR, "CAIRO_STATUS_PNG_ERROR", "png-error" },
 	  { CAIRO_STATUS_FREETYPE_ERROR, "CAIRO_STATUS_FREETYPE_ERROR", "freetype-error" },
 	  { CAIRO_STATUS_LAST_STATUS, "CAIRO_STATUS_LAST_STATUS", "last-status" },
+	  { CAIRO_STATUS_WIN32_GDI_ERROR, "CAIRO_STATUS_WIN32_GDI_ERROR", "win32-gdi-error" },
           { 0, NULL, NULL }
       };
       GType type = g_enum_register_static (g_intern_static_string ("cairo_status_t"), values);
diff --git a/util/cairo-script/cairo-script-private.h b/util/cairo-script/cairo-script-private.h
index a8e203b..da846dc 100644
--- a/util/cairo-script/cairo-script-private.h
+++ b/util/cairo-script/cairo-script-private.h
@@ -228,7 +228,7 @@  typedef enum _csi_status {
     CSI_STATUS_JBIG2_GLOBAL_MISSING = CAIRO_STATUS_JBIG2_GLOBAL_MISSING,
     CSI_STATUS_PNG_ERROR = CAIRO_STATUS_PNG_ERROR,
     CSI_STATUS_FREETYPE_ERROR = CAIRO_STATUS_FREETYPE_ERROR,
-
+    CSI_STATUS_WIN32_GDI_ERROR = CAIRO_STATUS_WIN32_GDI_ERROR,
 
     /* cairo-script-interpreter specific errors */
 
diff --git a/util/cairo-trace/trace.c b/util/cairo-trace/trace.c
index 652e04a..e3c9933 100644
--- a/util/cairo-trace/trace.c
+++ b/util/cairo-trace/trace.c
@@ -1584,6 +1584,7 @@  _status_to_string (cairo_status_t status)
 	f(JBIG2_GLOBAL_MISSING);
 	f(PNG_ERROR);
 	f(FREETYPE_ERROR);
+	f(WIN32_GDI_ERROR);
     case CAIRO_STATUS_LAST_STATUS:
 	break;
     }
-- 
2.1.4



Comments

Am 26.03.2016 um 13:17 schrieb Adrian Johnson:
> I forgot to CC the list.
> 
> -------- Forwarded Message --------
> Subject: Re: [cairo] [PATCH] Generate better error message when loading
> invalid PNGs
> Date: Sat, 26 Mar 2016 22:35:02 +1030
> From: Adrian Johnson <ajohnson@redneon.com>
> To: Uli Schlachter <psychon@znc.in>
> 
> On 26/03/16 18:05, Uli Schlachter wrote:
>> 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.
> 
> I also noticed I missed a case statement in cairo-spans.c. I will fix
> both issues before pushing.
> 
>> With the above fixed:
>>
>> Reviewed-by: Uli Schlachter <psychon@znc.in>
>>
>> (Should there also be matching unit test, similar to what my patch did?)
> 
> Your patch should work with the expected error code changed to
> CAIRO_STATUS_PNG_ERROR. I'll leave it for you to push your test.

Let's just skip the tests then.
/me hides

> I've attached three more patches:
> 
> - Update the errors in the utils/* files. They have not been updated for
> a while.
> 
> - Add CAIRO_STATUS_FREETYPE_ERROR for libfreetype errors. It now returns
> CAIRO_STATUS_NO_MEMORY for out of memory and CAIRO_STATUS_FREETYPE_ERROR
> for any other error.
> 
> - Add CAIRO_STATUS_WIN32_GDI_ERROR for GDI errors. I didn't try to
> isolate out of memory errors from other errors as it is not clear to me
> if a GDI out of memory error means "out of heap memory" or "out of some
> internal resource in Windows". If the latter it is more helpful to
> return a GDI error than to return NO_MEMORY.

All look good to me as well. Feel free to push with my R-b.

Cheers,
Uli