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

[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 at redneon.com>
To: Uli Schlachter <psychon at 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 at 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
> 




-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-Adding-missing-error-status-to-utils.patch
Type: text/x-patch
Size: 3027 bytes
Desc: not available
URL: <https://lists.cairographics.org/archives/cairo/attachments/20160326/48272fde/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-Add-CAIRO_STATUS_FREETYPE_ERROR-for-errors-returned-.patch
Type: text/x-patch
Size: 7715 bytes
Desc: not available
URL: <https://lists.cairographics.org/archives/cairo/attachments/20160326/48272fde/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0004-Add-CAIRO_STATUS_WIN32_GDI_ERROR-for-GDI-errors.patch
Type: text/x-patch
Size: 7818 bytes
Desc: not available
URL: <https://lists.cairographics.org/archives/cairo/attachments/20160326/48272fde/attachment-0002.bin>