857040 - PNG Image "contains errors" on Nightly, but renders fine in Safari

Status: VERIFIED FIXED

(Core :: Graphics, defect)

Description

[Tobias (:Tobbi) Markus]

I know the bug description is a little vague, but I don't know where this comes from.

The image in 

https://www.evernote.com/shard/s4/sh/72b0367c-ec06-4f01-8fed-db5ec8e01da9/3ac1ad0b336c65207dfcfc34bcece132 

which is 

https://www.evernote.com/shard/s4/sh/72b0367c-ec06-4f01-8fed-db5ec8e01da9/3ac1ad0b336c65207dfcfc34bcece132/res/a15766e8-a42a-4fa1-84c7-50ddc988b0b0/skitch.png 

doesn't render correctly in Firefox, but renders correctly in Safari. I get the not specific error message "cannot be displayed because it contains errors".

Comment 1

[Glenn Randers-Pehrson]

The PNG contains private chunks after the IDAT chunks with bad CRC.
Pngcrush -n -v skitch.png says:

Opening file skitch.png for length measurement
Allocating read structure
Allocating read_info,  end_info structures
Reading IHDR chunk, length = 13.
Reading pHYs chunk, length = 9.
Reading IDAT chunk, length = 16384.
Reading IDAT chunk, length = 16384.
Reading IDAT chunk, length = 16384.
Reading IDAT chunk, length = 16384.
Reading IDAT chunk, length = 4189.
Reading skMf chunk, length = 1756.
libpng warning: [00][00][00][00]: CRC error
Reading skRf chunk, length = 60307.
libpng warning: [00][00][00][00]: CRC error
Reading IEND chunk, length = 0.

Chrome also displays the file, but throwing an error message is the correct
behavior.  The bugfix is to fix the invalid PNG file upstream.

You can use "pngcrush -fix skitch.png skitch_fixed.png" to install
correct CRC values.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Why did bug 716140 affect our behavior here?

Comment 4

[Jesse Ruderman]

Another weird thing is that sometimes it says "The image [URL] cannot be displayed because it contains errors" and sometimes it just shows the URL.

Comment 5

[Jonathan Kew [:jfkthame]]

(In reply to Glenn Randers-Pehrson from comment #1)
> The PNG contains private chunks after the IDAT chunks with bad CRC.
> Pngcrush -n -v skitch.png says:
> 
> Opening file skitch.png for length measurement
> Allocating read structure
> Allocating read_info,  end_info structures
> Reading IHDR chunk, length = 13.
> Reading pHYs chunk, length = 9.
> Reading IDAT chunk, length = 16384.
> Reading IDAT chunk, length = 16384.
> Reading IDAT chunk, length = 16384.
> Reading IDAT chunk, length = 16384.
> Reading IDAT chunk, length = 4189.
> Reading skMf chunk, length = 1756.
> libpng warning: [00][00][00][00]: CRC error
> Reading skRf chunk, length = 60307.
> libpng warning: [00][00][00][00]: CRC error
> Reading IEND chunk, length = 0.
> 
> Chrome also displays the file, but throwing an error message is the correct
> behavior.

ISTM that as the skMf or skRf chunks are "ancillary" private chunks, it's legitimate to simply ignore them as unknown. (Decoders are not -required- by the spec to check CRC values.)

Rejecting the file because of the CRC errors is -also- a legitimate behavior, but it's not the only possible "correct" behavior.

Comment 6

[Glenn Randers-Pehrson]

> Rejecting the file because of the CRC errors is -also- a legitimate
> behavior, but it's not the only possible "correct" behavior.

True.  The PNG spec says in Clause 13.2, Error Handling, "... a CRC
mismatch ... indicates a corrupted datastream, and may be regarded as
a fatal error."  After we wrote the spec we found that it also can indicate
laziness on the part of the authors of private chunks, which seems to be
the case here.  If the skMf and skRf chunks had good CRC's, we would have
ignored them as unknown ancillary chunks.

See bug #397593, when we put in code to reject images with chunks after
IDAT that have a bad CRC.  At that time we were concerned about corrupt
APNG chunks following the main image.

Comment 7

[Glenn Randers-Pehrson]

(In reply to Boris Zbarsky (:bz) from comment #3)
> Why did bug 716140 affect our behavior here?

I don't know, but note that part 23 of that bug does change the error handling.
How did it affect the behavior?  Are we more lenient now or more strict?

Comment 8

[Joe Drew (not getting mail)]

More strict in some cases, more lenient in others, sadly. "Different." More correct in general, though.

Comment 9

[Glenn Randers-Pehrson]

(In reply to Glenn Randers-Pehrson from comment #6)

> it also can indicate
> laziness on the part of the authors of private chunks, which seems to be
> the case here.  If the skMf and skRf chunks had good CRC's, we would have
> ignored them as unknown ancillary chunks.

In fact it can also indicate a bug in libpng.    #:-(
libpng-1.6.x writes an incorrect length in uncompressed iTXt chunks which leads to reading the CRC from the wrong place.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Well, given that there is at least one "post a screenshot" web service where it seems like _all_ the images are hitting this, it seems like we need to either fix or evangelize for 21...

Comment 11

[Seth Fowler [:seth] [:s2h]]

I gave it a shot at fixing this in nsPNGDecoder.cpp. It's easy enough to fix this by changing the last argument of png_set_crc_action on line 660 to PNG_CRC_WARN_DISCARD. That will cause us to change our behavior for bug 397593, though, since AFAICT we'll now happily display PNGs that we wanted to reject in that bug. I couldn't figure out how to be more selective within the libpng API, since the callback for unknown chunks apparently is called after CRC issues have already been dealt with.

If we want to fix this bug rather than rely on evangelism, does anyone have a better approach? Let me know and I can post a patch, if this is what we want.

Comment 12

[Glenn Randers-Pehrson]

PNG_CRC_WARN_DISCARD is a bit dangerous.  It's OK if there is a corrupted bit in a chunk, but if there is a missing bit then the CRC mismatch is because the decoders is looking in the wrong place for the CRC.  Then, if the setting is WARN_DISCARD, it reads the next 4 bytes as the length of the next chunk.  If those are read from the wrong place, like as not the "length" will be a huge number and the chunk name will be garbage, and the IEND chunk will never be found.  Note that WARN_DISCARD does not actually issue a warning unless it's a debug build.  The decoder will just keep spinning until it reaches the end of the datastream.

Comment 13

[Seth Fowler [:seth] [:s2h]]

Yeah, I noticed (with some frustration) that it wasn't actually issuing a warning. Thanks for the clarification. Is there a safer alternative?

Comment 14

[Glenn Randers-Pehrson]

(In reply to Seth Fowler [:seth] from comment #13)
> Is there a safer alternative?

status quo rejects the file.  WARN_DISCARD will display the file if there is a noncritical corrupted chunk with correct length but might go into a tailspin if there is a corrupted chunk with wrong length.  There is no way for libpng to distinguish between those two cases.

Comment 15

[Alex Keybl [:akeybl]]

(In reply to Glenn Randers-Pehrson from comment #12)
> PNG_CRC_WARN_DISCARD is a bit dangerous.  It's OK if there is a corrupted
> bit in a chunk, but if there is a missing bit then the CRC mismatch is
> because the decoders is looking in the wrong place for the CRC.  Then, if
> the setting is WARN_DISCARD, it reads the next 4 bytes as the length of the
> next chunk.  If those are read from the wrong place, like as not the
> "length" will be a huge number and the chunk name will be garbage, and the
> IEND chunk will never be found.  Note that WARN_DISCARD does not actually
> issue a warning unless it's a debug build.  The decoder will just keep
> spinning until it reaches the end of the datastream.

What's the user impact if we make this change? Security?

An in-product behavior fix here is HIGHLY desirable over doing outreach to all (known) affected sites.

Comment 16

[Jesse Ruderman]

Are there known affected sites other than Evernote?

Comment 17

[Seth Fowler [:seth] [:s2h]]

Evernote itself isn't even affected, is it? It's just that that bad image happens to be hosted on Evernote.

Comment 18

[Jesse Ruderman]

We've seen 3 broken images, all hosted on Evernote.  So whatever software is generating them is probably associated with Evernote somehow.

Comment 19

[Joe Drew (not getting mail)]

Skitch, maybe?

Comment 20

[Seth Fowler [:seth] [:s2h]]

Heh, yeah, which Evernote apparently has bought. Might be worth talking to the Skitch folks.

Comment 21

[Trent Mick]

FWIW, I logged a bug with evernote and so far got this response:

```
Hello trentm,

We apologize for the inconvenience, however it appears there is currently a bug causing this.
We appreciate you taking the time to send us this error. We have filed an internal bug report with the QA team lead. She will classify and fix the bugs in the order determined by the project Lead.
Unfortunately this is not something I can fix for you at this time.
Once again, we apologize for the inconvenience.
Thanks for making Evernote a better product,

Vidal Vasquez
Evernote Support
```

Comment 23

[Alex Keybl [:akeybl]]

What are our in-product options (comment 15) especially if this ends up being a larger issue?

Comment 24

[Glenn Randers-Pehrson]

See discussion in bug #397593.  The alternative is to undo that bugfix (simply removing several lines in NSPNGDecoder.cpp) and ignore the APNG spec "strong recommendation":

Error Handling

APNG is designed to allow incremental display of frames before the entire image has been read. This implies that some errors may not be detected until partway through the animation. It is strongly recommended that when any error is encountered decoders should discard all subsequent frames, stop the animation, and revert to displaying the default image. A decoder which detects an error before the animation has started should display the default image. An error message may be displayed to the user if appropriate.

Regular PNG images won't be affected much because we don't use any chunks after the IDAT (image data) except for IEND (signals the end of the PNG datastream).  But I don't much like the potential for failing to detect the end of the datastream.

Comment 25

[Joe Drew (not getting mail)]

What Glenn said.

Comment 26

[Kadir Topal [:atopal]]

Not to add noise to the bug, but whatever we decide we should do it soon. This is going to hit a large number of users with the Beta release in 12 days. 

Did anyone already reach out to Skitch/Evernote and made them aware of the fact that in a few weeks they will be flooded with a bajillion support requests because Firefox users can't see the screenshots anymore?

Comment 27

[Glenn Randers-Pehrson]

Attachment: v00 warn on bad CRC

Revert to default libpng behavior of warning about bad CRC in ancillary chunks after IDAT instead of issuing an error.

Comment 28

[Glenn Randers-Pehrson]

I posted a patch to remove the special handling of bad CRC after IDAT.  It can't introduce any new vulnerability because we already handle bad CRC in chunks before IDAT that way.  Encountering such a bad chunk we'd be out of sync thereafter and never find the IDAT chunks or the IEND chunk anyhow.

It's a simple patch that just removes the png_set_crc_action() call and adds some commentary, but I guess it should be run through the try server anyway.  @Ryan would you do the honors?

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://tbpl.mozilla.org/?tree=Try&rev=ab5361d1d566

Comment 30

[Glenn Randers-Pehrson]

It looks like only 4 builds, on Fedora and Ubuntu, were tried.  They are all green.

Comment 31

[Joe Drew (not getting mail)]

Comment on attachment 744639 [details] [diff] [review]
v00 warn on bad CRC

Review of attachment 744639 [details] [diff] [review]:
-----------------------------------------------------------------

::: image/decoders/nsPNGDecoder.cpp
@@ +656,1 @@
>     */

Feel free to just remove this comment altogether.

Comment 32

[Glenn Randers-Pehrson]

Attachment: v01 warn on bad CRC

Removed historical commentary

Comment 33

[Joe Drew (not getting mail)]

Comment on attachment 745211 [details] [diff] [review]
v01 warn on bad CRC

Lovely!

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/06875b8eb65c

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/06875b8eb65c

Seems like we could reftest this?

Comment 36

[Glenn Randers-Pehrson]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #35)
> https://hg.mozilla.org/mozilla-central/rev/06875b8eb65c
> 
> Seems like we could reftest this?

I'll prepare one.

Comment 37

[Glenn Randers-Pehrson]

Attachment: junk_bad_CRC.png for ref test

Comment 38

[Glenn Randers-Pehrson]

Extract from the output of "pngcrush -n -v -fix junk_bad_CRC.png":

Opening file junk_bad_CRC.png for length measurement
Reading IHDR chunk, length = 13.
Reading PLTE chunk, length = 6.
Reading IDAT chunk, length = 10.
Reading jUNk chunk, length = 0.
libpng warning: [00][00][00][00]: CRC error
Reading IEND chunk, length = 0.

Comment 39

[Joe Drew (not getting mail)]

Comment on attachment 745211 [details] [diff] [review]
v01 warn on bad CRC

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 716140 
User impact if declined: Some invalid PNGs that we used to accept are rejected
Testing completed (on m-c, etc.): On m-c and aurora
Risk to taking this patch (and alternatives if risky): Should be very low risk.
String or IDL/UUID changes made by this patch: none

Comment 40

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/6925a549d2f0

Glenn, were you going to submit a reftest still?

Comment 41

[Glenn Randers-Pehrson]

Should it go in media/libpng/crashtests, image/test/reftest, or somewhere else?

Comment 42

[Ryan VanderMeulen [:RyanVM]]

image/test/reftest sounds right to me. Joe?

Comment 43

[Joe Drew (not getting mail)]

Yep!

Comment 44

[Paul Silaghi, QA [:pauly]]

Verified fixed using the testcase and the link in comment 0 in FF 22b3 Win 7, Ubuntu 13.04 and Mac OS X 10.8.3

Comment 45

[Mihai Morar, (:MihaiMorar)]

Verified Fixed on FF23b5 on Win 7x64, ubuntu 13.04 x86 and Mac OS 10.9:

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:24.0) Gecko/20130711 Firefox/24.0
Mozilla/5.0 (X11; Linux i686; rv:23.0) Gecko/20100101 Firefox/23.0
Build ID: 20130711004005

Comment 46

[Mihai Morar, (:MihaiMorar)]

(In reply to Mihai Morar, QA [:MarioMi] from comment #45)

Sorry for wrong UserAgent Win7 and Build ID.
The corect one is: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:23.0) Gecko/20100101 Firefox/23.0(20130711122148)