Providing an invalid PNG gives confusing error message "out of memory"

[winny-]

I saw in another issue the rationale for no JPG support or what have you, but it seems a little cryptic to give an out of memory error message when the the file is not a valid PNG.

How to reproduce

➜  ~  wget -q https://upload.wikimedia.org/wikipedia/commons/d/d6/Bromo-Tengger-Semeru_banner_2.jpg
➜  ~  i3lock -i Bromo-Tengger-Semeru_banner_2.jpg
Could not load image "Bromo-Tengger-Semeru_banner_2.jpg": out of memory
➜  ~  i3lock -i /dev/zero                        
Could not load image "/dev/zero": out of memory

[eplanet]

You're totally right! That error message comes from cairo_status_to_string (see error codes from Cairo).

It seems that the call to cairo_image_surface_create_from_png function doesn't give a lot of options: it returns either CAIRO_STATUS_NO_MEMORY, CAIRO_STATUS_FILE_NOT_FOUND or CAIRO_STATUS_READ_ERROR.

The only obvious way to improve this is checking manually the conditions of error. I'd be happy to provide a PR if agreed by @stapelberg and @Airblader.

[Airblader]

We once had this in #64 already, but back then cairo didn't have proper return codes. The patch for it in cairo actually originated there. :-) Improving the error message sounds good, provided it doesn't add a new dependency or requires us to guess or do checks on our own like it would have to back then.

We also shouldn't have to update the minimum required cairo version for this.

[eplanet]

Looks interesting. Without introducing a dependency to libmagic, checking for file existence and the first 8 bytes to be the PNG signature would cover virtually 99,9% of the error cases. I'm preparing a patch for that.

[Airblader]

No, let's please not do that. Handling the file is cairo's job. We should simply deal with the proper response it gives us in newer versions.

[crypdick]

@Airblader so is this a wontfix? I fell into this trap and Google shows that many others have encountered this issue.

[Airblader]

No, my answer above remains. We are happy to improve the message as long as we get the proper response from cairo and don't have to parse png ourselves.

[psychon]

Handling the file is cairo's job.

And cairo's position is "this is just a toy API". :-)

I just want to mention that there is a proposal for an API addition in cairo to provide some text in addition to the error codes: https://lists.cairographics.org/archives/cairo/2017-November/028442.html (No idea how the resulting texts would look like)

[Airblader]

And cairo's position is "this is just a toy API". :-)

But frankly I don't see the point in both parties parsing the file. Either cairo loads and renders the file and we don't, or we load it and pass it on to cairo. But not we load it to validate it and let cairo load it again so it can be drawn. :-)

[eplanet]

Allow me to insist since the discussion continues: reading the first 8 bytes of the file doesn't really mean "parsing" but rather like a CRC check. I understand you don't like it, but it's 100% effective, easy to implement and would virtually never change...

[Airblader]

Maybe so, but I still consider it a dirty hack. The validation should be the responsibility of the component doing something with the file.

[stapelberg]

I agree that the validation should happen in cairo. For the time being, I’d be okay with a PR which examines the first 8 bytes, though.

[Airblader]

For the time being

AKA "Forever", since the way I understand it this won't change in cairo.

[eplanet]

Great! Let me propose a PR for that then :-)

[psychon]

What do you think about using GdkPixbuf for loading images? That one provides "good" error messages and can even load SVGs (although it requires some magic).

[stapelberg]

AFAICT, GdkPixbuf is not yet in our transitive closure of dependencies, and is actually somewhat sizeable. I’d prefer not to use it.

[winny-]

Out of curiousity - what if i3lock simply checked that the first few bytes are 89 50 4e 47 0d 0a 1a 0a for PNG and ff d8 ff for JPEG (if the support gets merged in)?

This seems like the best way to do it, as the library i3lock uses doesn't have such functionality.

[stapelberg]

That’s what https://github.com/i3/i3lock/issues/114#issuecomment-356929698 already suggested.

[sdhand]

If you're not prepared to actually do any parsing of the image , why not just add some warning text to the error itself explaining that "only png is supported and cairo error messages are sometimes inaccurate" or words to that affect?

[winny-]

I think that would lead to users opening issues entitled only png is supported and cairo error messages are sometimes inaccurate and not actually help users realize their mistake.