Closed Bug 857040 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: Graphics, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla23
Tracking Status
firefox21 --- unaffected
firefox22 + verified
firefox23 + verified

People

(Reporter: Tobbi, Assigned: glennrp+bmo)

References

(Depends on 1 open bug)

Details

(Keywords: regression)

Attachments

(2 files, 1 obsolete file)

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".
Summary: Image "contains errors" on Nightly, but renders fine in Safari → PNG Image "contains errors" on Nightly, but renders fine in Safari
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.
Assignee: nobody → glennrp+bmo
Status: NEW → ASSIGNED
OS: Mac OS X → All
Hardware: x86 → All
Duplicate of this bug: 860020
Why did bug 716140 affect our behavior here?
Flags: needinfo?(glennrp+bmo)
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.
(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.
> 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.
Flags: needinfo?(glennrp+bmo)
(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?
More strict in some cases, more lenient in others, sadly. "Different." More correct in general, though.
(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.
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...
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.
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.
Yeah, I noticed (with some frustration) that it wasn't actually issuing a warning. Thanks for the clarification. Is there a safer alternative?
(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.
(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.
Are there known affected sites other than Evernote?
Evernote itself isn't even affected, is it? It's just that that bad image happens to be hosted on Evernote.
We've seen 3 broken images, all hosted on Evernote.  So whatever software is generating them is probably associated with Evernote somehow.
Skitch, maybe?
Heh, yeah, which Evernote apparently has bought. Might be worth talking to the Skitch folks.
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
```
Duplicate of this bug: 862944
What are our in-product options (comment 15) especially if this ends up being a larger issue?
Flags: needinfo?(joe)
Flags: needinfo?(glennrp+bmo)
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.
Flags: needinfo?(glennrp+bmo)
What Glenn said.
Flags: needinfo?(joe)
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?
Attached patch v00 warn on bad CRC (obsolete) — Splinter Review
Revert to default libpng behavior of warning about bad CRC in ancillary chunks after IDAT instead of issuing an error.
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?
Flags: needinfo?(ryanvm)
Depends on: 397593
It looks like only 4 builds, on Fedora and Ubuntu, were tried.  They are all green.
Attachment #744639 - Flags: review?(joe)
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.
Attachment #744639 - Flags: review?(joe) → review+
Removed historical commentary
Attachment #744639 - Attachment is obsolete: true
Attachment #745211 - Flags: review?(joe)
Comment on attachment 745211 [details] [diff] [review]
v01 warn on bad CRC

Lovely!
Attachment #745211 - Flags: review?(joe) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/06875b8eb65c

Seems like we could reftest this?
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
(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.
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.
Blocks: 872626
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
Attachment #745211 - Flags: approval-mozilla-beta?
Attachment #745211 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Should it go in media/libpng/crashtests, image/test/reftest, or somewhere else?
image/test/reftest sounds right to me. Joe?
Flags: needinfo?(joe)
Yep!
Flags: needinfo?(joe)
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
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
Status: RESOLVED → VERIFIED
(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)
No longer blocks: 872626
Depends on: 872626
No longer depends on: 397593
You need to log in before you can comment on or make changes to this bug.