397593 - Bad fcTL CRC in APNG files ignored

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Andrew Smith]

It looks like an APNG file with a bad CRC for the fcTL chunk but otherwise valid displays without complaining. This is not ok. Best of all if it didn't disply at all.

I'll look into it when I get a chance.

Comment 1

[Glenn Randers-Pehrson]

The spec says:

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.

Libpng discards the fcTL chunk with bad CRC, and continues. The APNG decoder should notice that the sequence number for the fcTL chunk is missing.

Comment 2

[Glenn Randers-Pehrson]

Attachment: Reject fcTL or fdAT chunk with bad CRC

Comment 3

[Glenn Randers-Pehrson]

The patch will reject *any* ancillary chunk after IDAT that has a bad CRC.  This is not much of a problem because we don't process any other ancillary chunks that come after IDAT anyhow.

Comment 4

[Glenn Randers-Pehrson]

The spec said "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" but the patch discards all subsequent frames, stops the animation, but simply bails out rather than reverting to the default image.  That would add more complexity than it's worth.

Comment 5

[Joe Drew (not getting mail)]

Comment on attachment 380651 [details] [diff] [review]
Reject fcTL or fdAT chunk with bad CRC

Yeah, this is probably the right way.

Comment 6

[Glenn Randers-Pehrson]

This patch is *not* incorporated in the libpng-1.2.37 patch (bug #492200)
since it is a libpr0n bug not a libpng bug.  It needs to be sr'ed and checked
in separately.

Comment 7

[Glenn Randers-Pehrson]

Note that the patch does not reject an fcTL chunk with bad CRC appearing ahead of the first IDAT.  Instead the fcTL chunk will be ignored and the IDAT will be displayed.  If there are subsequent good fcTL/fdAT chunks, they will be rejected because the sequence numbers are wrong.  I think this behavior is OK.

Comment 8

[Glenn Randers-Pehrson]

Requesting checkin on trunk.  This is probably not a serious-enough problem to bother with checking in to the branch.

Comment 9

[Serge Gautherie (:sgautherie)]

Comment on attachment 380651 [details] [diff] [review]
Reject fcTL or fdAT chunk with bad CRC

>+  /* Reject any ancillary chunk after IDAT with a bad CRC (bug #397593).
>+   * It would be better to show the default frame, if one has already
>+   * been successfully decoded, before bailing but it's simpler to just

Nit, shouldn't that be "decoded before bailing, but"?

>+   * bail out with an error message.
>+   */

Comment 10

[Glenn Randers-Pehrson]

How about:

  * It would be better to show the default frame (if one has already been
  * successfully decoded) before bailing, but it's simpler to just bail
  * out with an error message.
  */

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Checked in with that comment fix:
http://hg.mozilla.org/mozilla-central/rev/43a9b9b923f7

Thanks!

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Glenn, have you got an example PNG that shows the problem you fixed here? We should turn it into a reftest.

Comment 13

[Glenn Randers-Pehrson]

Fixing bug #857040 has deliberately caused regression of this bug.