Generate better error message when loading invalid PNGs

Submitted by Uli Schlachter on March 25, 2016, 5:23 p.m.

Details

Message ID 56F5742E.5040905@znc.in
State New
Series "Generate better error message when loading invalid PNGs"
Headers show

Commit Message

Uli Schlachter March 25, 2016, 5:23 p.m.
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).

Additionally, a unit test is added for testing this, but only for
cairo_image_surface_create_from_png_stream().

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

Hi Cyril, what do you think about this patch?

 src/cairo-png.c        |  8 ++++++++
 test/create-from-png.c | 20 ++++++++++++++++++++
 2 files changed, 28 insertions(+)

Patch hide | download patch | download mbox

diff --git a/src/cairo-png.c b/src/cairo-png.c
index 068617d..e4356aa 100644
--- a/src/cairo-png.c
+++ b/src/cairo-png.c
@@ -549,6 +549,7 @@  read_png (struct png_read_closure_t *png_closure)
     cairo_status_t status;
     unsigned char *mime_data;
     unsigned long mime_data_length;
+    png_byte png_magic[8];
 
     png_closure->png_data = _cairo_memory_stream_create ();
 
@@ -578,6 +579,13 @@  read_png (struct png_read_closure_t *png_closure)
     }
 #endif
 
+    stream_read_func (png, png_magic, ARRAY_LENGTH (png_magic));
+    if (png_sig_cmp (png_magic, 0, ARRAY_LENGTH (png_magic))) {
+	surface = _cairo_surface_create_in_error (CAIRO_STATUS_READ_ERROR);
+	goto BAIL;
+    }
+    png_set_sig_bytes (png, ARRAY_LENGTH (png_magic));
+
     png_read_info (png, info);
 
     png_get_IHDR (png, info,
diff --git a/test/create-from-png.c b/test/create-from-png.c
index 2ca1fa2..1f18c47 100644
--- a/test/create-from-png.c
+++ b/test/create-from-png.c
@@ -42,6 +42,13 @@  read_error (void *closure, unsigned char *data, unsigned int size)
     return CAIRO_STATUS_READ_ERROR;
 }
 
+static cairo_status_t
+not_a_png (void *closure, unsigned char *data, unsigned int size)
+{
+    memset (data, 42, size);
+    return CAIRO_STATUS_SUCCESS;
+}
+
 static cairo_test_status_t
 draw (cairo_t *cr, int width, int height)
 {
@@ -130,6 +137,19 @@  preamble (cairo_test_context_t *ctx)
     if (result != CAIRO_TEST_SUCCESS)
 	return result;
 
+    surface = cairo_image_surface_create_from_png_stream (not_a_png, NULL);
+    if (cairo_surface_status (surface) != CAIRO_STATUS_READ_ERROR) {
+	result = cairo_test_status_from_status (ctx,
+						cairo_surface_status (surface));
+	if (result == CAIRO_TEST_FAILURE) {
+	    cairo_test_log (ctx, "Error: expected \"read error\", but got: %s\n",
+			    cairo_status_to_string (cairo_surface_status (surface)));
+	}
+    }
+    cairo_surface_destroy (surface);
+    if (result != CAIRO_TEST_SUCCESS)
+	return result;
+
     /* cheekily test error propagation from the user write funcs as well ... */
     xasprintf (&path, "%s/reference", ctx->srcdir);
     xasprintf (&filename, "%s/%s", path, "create-from-png.ref.png");

Comments

Bill Spitzak March 25, 2016, 7:24 p.m.
I really don't see this as better, because now you are relying on local
code arriving at exactly the same "is this a png file" conclusion as
libpng. Also there are lots of other errors that are not no-memory.

Grepping libpng for png_error, I think perhaps this code will pick the most
appropriate error:

    if (strstr(message, "alloc") || strstr(message, "OOM") ||
strstr(message, "memory")
         return CAIRO_STATUS_NO_MEMORY;
    else
         return CAIRO_STATUS_READ_ERROR;

Grep was from here:
https://github.com/glennrp/libpng/search?utf8=%E2%9C%93&q=png_error


On Fri, Mar 25, 2016 at 10:23 AM, Uli Schlachter <psychon@znc.in> 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).
>
> Additionally, a unit test is added for testing this, but only for
> cairo_image_surface_create_from_png_stream().
>
> Signed-off-by: Uli Schlachter <psychon@znc.in>
> ---
>
> Hi Cyril, what do you think about this patch?
>
>  src/cairo-png.c        |  8 ++++++++
>  test/create-from-png.c | 20 ++++++++++++++++++++
>  2 files changed, 28 insertions(+)
>
> diff --git a/src/cairo-png.c b/src/cairo-png.c
> index 068617d..e4356aa 100644
> --- a/src/cairo-png.c
> +++ b/src/cairo-png.c
> @@ -549,6 +549,7 @@ read_png (struct png_read_closure_t *png_closure)
>      cairo_status_t status;
>      unsigned char *mime_data;
>      unsigned long mime_data_length;
> +    png_byte png_magic[8];
>
>      png_closure->png_data = _cairo_memory_stream_create ();
>
> @@ -578,6 +579,13 @@ read_png (struct png_read_closure_t *png_closure)
>      }
>  #endif
>
> +    stream_read_func (png, png_magic, ARRAY_LENGTH (png_magic));
> +    if (png_sig_cmp (png_magic, 0, ARRAY_LENGTH (png_magic))) {
> +       surface = _cairo_surface_create_in_error (CAIRO_STATUS_READ_ERROR);
> +       goto BAIL;
> +    }
> +    png_set_sig_bytes (png, ARRAY_LENGTH (png_magic));
> +
>      png_read_info (png, info);
>
>      png_get_IHDR (png, info,
> diff --git a/test/create-from-png.c b/test/create-from-png.c
> index 2ca1fa2..1f18c47 100644
> --- a/test/create-from-png.c
> +++ b/test/create-from-png.c
> @@ -42,6 +42,13 @@ read_error (void *closure, unsigned char *data,
> unsigned int size)
>      return CAIRO_STATUS_READ_ERROR;
>  }
>
> +static cairo_status_t
> +not_a_png (void *closure, unsigned char *data, unsigned int size)
> +{
> +    memset (data, 42, size);
> +    return CAIRO_STATUS_SUCCESS;
> +}
> +
>  static cairo_test_status_t
>  draw (cairo_t *cr, int width, int height)
>  {
> @@ -130,6 +137,19 @@ preamble (cairo_test_context_t *ctx)
>      if (result != CAIRO_TEST_SUCCESS)
>         return result;
>
> +    surface = cairo_image_surface_create_from_png_stream (not_a_png,
> NULL);
> +    if (cairo_surface_status (surface) != CAIRO_STATUS_READ_ERROR) {
> +       result = cairo_test_status_from_status (ctx,
> +                                               cairo_surface_status
> (surface));
> +       if (result == CAIRO_TEST_FAILURE) {
> +           cairo_test_log (ctx, "Error: expected \"read error\", but got:
> %s\n",
> +                           cairo_status_to_string (cairo_surface_status
> (surface)));
> +       }
> +    }
> +    cairo_surface_destroy (surface);
> +    if (result != CAIRO_TEST_SUCCESS)
> +       return result;
> +
>      /* cheekily test error propagation from the user write funcs as well
> ... */
>      xasprintf (&path, "%s/reference", ctx->srcdir);
>      xasprintf (&filename, "%s/%s", path, "create-from-png.ref.png");
> --
> 2.8.0.rc3
>
> --
> cairo mailing list
> cairo@cairographics.org
> https://lists.cairographics.org/mailman/listinfo/cairo
Cyril Roelandt March 25, 2016, 8:24 p.m.
On 03/25/2016 08:24 PM, Bill Spitzak wrote:
> I really don't see this as better, because now you are relying on local
> code arriving at exactly the same "is this a png file" conclusion as
> libpng. Also there are lots of other errors that are not no-memory.
> 
> Grepping libpng for png_error, I think perhaps this code will pick the most
> appropriate error:
> 
>     if (strstr(message, "alloc") || strstr(message, "OOM") ||
> strstr(message, "memory")
>          return CAIRO_STATUS_NO_MEMORY;
>     else
>          return CAIRO_STATUS_READ_ERROR;
> 

Why wouldn't we grep for "Not a PNG" as well? Also, Uli stated that it
might not be the best idea to rely on the error messages from libpng,
since they might change. WDYT?

Using png_sig_cmp allow us to be perfectly sure that the issue is with
the file/stream, and to return an appropriate error code to the user,
without gambling.

Cyril.
Cyril Roelandt March 25, 2016, 8:26 p.m.
On 03/25/2016 06:23 PM, Uli Schlachter wrote:
> Hi Cyril, what do you think about this patch?

This looks good to me, thanks!

Cyril.
Bill Spitzak March 26, 2016, 2:35 a.m.
On 03/25/2016 01:24 PM, Cyril Roelandt wrote:
> On 03/25/2016 08:24 PM, Bill Spitzak wrote:
>> I really don't see this as better, because now you are relying on local
>> code arriving at exactly the same "is this a png file" conclusion as
>> libpng. Also there are lots of other errors that are not no-memory.
>>
>> Grepping libpng for png_error, I think perhaps this code will pick the most
>> appropriate error:
>>
>>      if (strstr(message, "alloc") || strstr(message, "OOM") ||
>> strstr(message, "memory")
>>           return CAIRO_STATUS_NO_MEMORY;
>>      else
>>           return CAIRO_STATUS_READ_ERROR;
>>
>
> Why wouldn't we grep for "Not a PNG" as well? Also, Uli stated that it
> might not be the best idea to rely on the error messages from libpng,
> since they might change. WDYT?
>
> Using png_sig_cmp allow us to be perfectly sure that the issue is with
> the file/stream, and to return an appropriate error code to the user,
> without gambling.

Because looking at the source code I can see a lot more errors that are 
not "out of memory" but they would be tagged this way. To me it looks 
easier to detect the out of memory errors because they use some words in 
common.