Improve error message when using cairo_image_surface_create_from_png with an invalid file.

Submitted by Cyril Roelandt on March 22, 2016, 10:19 p.m.

Details

Message ID 56F1C509.8040801@gmail.com
State New
Headers show
Series "Improve error message when using cairo_image_surface_create_from_png with an invalid file." ( rev: 1 ) in Cairo

Not browsing as part of any series.

Commit Message

Cyril Roelandt March 22, 2016, 10:19 p.m.
Hello!

Consider the following example program:

#include <stdio.h>
#include <cairo.h>

int
main(int argc, char *argv[])
{
	cairo_surface_t *img = NULL;
	const char *path = "/tmp/foo.jpg";
	int i;

	img = cairo_image_surface_create_from_png(path);
	if (cairo_surface_status(img) != CAIRO_STATUS_SUCCESS) {
		fprintf(stderr, "Status: %s\n",
			cairo_status_to_string(cairo_surface_status(img)));
	}
	return 0;
}

With /tmp/foo.jpg being a valid, readable JPEG file. The output of the
program is:

"Status: out of memory"

which is not really a description of the error that occurred (the file
should have been a PNG image). This is an obvious issue when writing a
program using cairo, but it is far less obvious when running an
application that uses cairo behind the scenes. For instance, i3lock, a
screen locker, allows users to display an image of their choosing on the
locked screen, but only accepts PNGs. When mistakenly providing a JPEG
file, end users are usually a bit confused by the error message[1].

The error status is set to CAIRO_STATUS_NO_MEMORY as a "default" error
in png_simple_error_callback(), since png_get_error_ptr() returns
CAIRO_STATUS_SUCCESS. However, I do not think we are really unable to
find out what happened: a dirty but working heuristic would be to look
at the value of the error message passed to the error callback. In our
case, it clearly states "Not a PNG file".

The following patch, or something along those lines, might help users
get a better error message:



Maybe we could even have a CAIRO_STATUS_NOT_A_PNG error status. What do
you think?

Cyril Roelandt.

[1] https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=776682

Patch hide | download patch | download mbox

diff --git a/src/cairo-png.c b/src/cairo-png.c
index 068617d..e87c465 100644
--- a/src/cairo-png.c
+++ b/src/cairo-png.c
@@ -134,9 +134,15 @@  png_simple_error_callback (png_structp png,
 {
     cairo_status_t *error = png_get_error_ptr (png);

-    /* default to the most likely error */
-    if (*error == CAIRO_STATUS_SUCCESS)
-       *error = _cairo_error (CAIRO_STATUS_NO_MEMORY);
+    if (*error == CAIRO_STATUS_SUCCESS) {
+        /* Try do determine the error using the error message.
+           If not possible, default to the most likely error. */
+        if (!strcmp(error_msg, "Not a PNG file"))
+            *error = CAIRO_STATUS_READ_ERROR;
+        else
+           *error = CAIRO_STATUS_NO_MEMORY;
+        *error = _cairo_error(*error);
+    }

 #ifdef PNG_SETJMP_SUPPORTED
     longjmp (png_jmpbuf (png), 1);

Comments

Hi,

On 22.03.2016 23:19, Cyril Roelandt wrote:
[...]
> The following patch, or something along those lines, might help users
> get a better error message:
>
> diff --git a/src/cairo-png.c b/src/cairo-png.c
> index 068617d..e87c465 100644
> --- a/src/cairo-png.c
> +++ b/src/cairo-png.c
> @@ -134,9 +134,15 @@ png_simple_error_callback (png_structp png,
>   {
>       cairo_status_t *error = png_get_error_ptr (png);
>
> -    /* default to the most likely error */
> -    if (*error == CAIRO_STATUS_SUCCESS)
> -       *error = _cairo_error (CAIRO_STATUS_NO_MEMORY);
> +    if (*error == CAIRO_STATUS_SUCCESS) {
> +        /* Try do determine the error using the error message.
> +           If not possible, default to the most likely error. */
> +        if (!strcmp(error_msg, "Not a PNG file"))
> +            *error = CAIRO_STATUS_READ_ERROR;
> +        else
> +           *error = CAIRO_STATUS_NO_MEMORY;
> +        *error = _cairo_error(*error);
> +    }
>
>   #ifdef PNG_SETJMP_SUPPORTED
>       longjmp (png_jmpbuf (png), 1);

I don't like "grepping" through error messages like this (what if libpng changes 
the exact wording?) and wondered if there is no better API for this. For the 
first time in my life I looked at the libpng API (and the API docs) and I hope 
that I will never do that again...
So second try was to search for "Not a PNG file" in debian codesearch where I 
found the following:

http://sources.debian.net/src/vtk/5.10.1+dfsg-2/IO/vtkPNGReader.cxx/?hl=58#L58

This reads the first 8 bytes of the file (handles short reads correctly!) and 
uses png_sig_cmp() to check that these bytes are correct (and then it uses 
png_set_sig_bytes() in line 101 to compensate for these missing bytes).

Having seen this code, I like libpng's API even less, but apparently this would 
be the "official" way to handle this.

Perhabs checking the error message isn't so bad after all... I'm unsure.

 > Maybe we could even have a CAIRO_STATUS_NOT_A_PNG error status. What do
 > you think?

The PNG-API in cairo is part of the toy interfaces, which exist to simplify 
writing examples and tests. They aren't meant to offer all possibilities (use 
something like gdk-pixbuf for that).

Thus, I don't think we should have special error codes for the toy API (but I 
don't mind if someone thinks otherwise and adds them anyway).


Cheers,
Uli
On 03/23/2016 02:37 PM, Uli Schlachter wrote:

> 
> I don't like "grepping" through error messages like this (what if libpng changes 
> the exact wording?) and wondered if there is no better API for this. For the 
> first time in my life I looked at the libpng API (and the API docs) and I hope 
> that I will never do that again...

I don't like grepping either, that's why I called this "dirty" :)

> So second try was to search for "Not a PNG file" in debian codesearch where I 
> found the following:
> 
> http://sources.debian.net/src/vtk/5.10.1+dfsg-2/IO/vtkPNGReader.cxx/?hl=58#L58
> 
> This reads the first 8 bytes of the file (handles short reads correctly!) and 
> uses png_sig_cmp() to check that these bytes are correct (and then it uses 
> png_set_sig_bytes() in line 101 to compensate for these missing bytes).
> 

Couldn't we read the first 8 bytes of the file in
cairo_image_surface_create_from_png() before calling read_png() ? If the
file is not a PNG, then we could return an error code
(CAIRO_STATUS_INVALID_FILE or whatever); if it is a PNG, then we'd pass
it to libpng, which would not fail.

> Having seen this code, I like libpng's API even less, but apparently this would 
> be the "official" way to handle this.
> 
> Perhabs checking the error message isn't so bad after all... I'm unsure.
> 
>  > Maybe we could even have a CAIRO_STATUS_NOT_A_PNG error status. What do
>  > you think?
> 
> The PNG-API in cairo is part of the toy interfaces, which exist to simplify 
> writing examples and tests. They aren't meant to offer all possibilities (use 
> something like gdk-pixbuf for that).
> 
> Thus, I don't think we should have special error codes for the toy API (but I 
> don't mind if someone thinks otherwise and adds them anyway).
> 

I'll trust you on that since I do not really know cairo :)


Cheers,
Cyril.