1422694 - Cannot display uncompressed PNG

Status: NEW

(Core :: Graphics: ImageLib, defect, P3)

Description

[nicolas]

Attachment: my.png

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0
Build ID: 20171112125346

Steps to reproduce:

Open the attached my.png in Firefox.


Actual results:

 It briefly displays it, then goes blank, showing only yhe URL). In Firefox v56.0 it also added "cannot be displayed because it contains errors".

When opening Tools -> Browser console, it says "image corrupt or truncated" (both in v56.0 and v57.0)



Expected results:

It should have displayed it correctly.
The file opens correctly in all other web browsers that I could test, and successfully passes the pngcheck test


pngcheck.exe -vvt my.png

File: my.png (21758 bytes)
  chunk IHDR at offset 0x0000c, length 13
    80 x 90 image, 24-bit RGB, non-interlaced
  chunk IDAT at offset 0x00025, length 21701
    zlib: deflated, 256-byte window, superfast compression
    row filters (0 none, 1 sub, 2 up, 3 avg, 4 paeth):
      0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0
      0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0
      0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0
      0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 (90 out of 90)
  chunk IEND at offset 0x054f6, length 0
No errors detected in my.png (3 chunks, -0.7% compression).

Comment 1

[Flore Allemandou [:flore]]

I reproduced the bug in latest Nightly. The image is not displayed, although, when downloaded it can be displayed (even by Windows preview feature)

Comment 2

[Timothy Nikkel (:tnikkel)]

We hit the "Not enough compressed data" error in libpng here

https://dxr.mozilla.org/mozilla-central/rev/574f4f58fe09dd590ea892406e237318c31705b4/media/libpng/pngpread.c#688

If I prevent us from making the decoder as being in error and display what we were able to decode then we display the image the same as Chrome.

Comment 3

[Timothy Nikkel (:tnikkel)]

Attachment: patch?

What do you think of this? Since we can hit this case after decoding almost the whole image it seems like it would make sense to not make this a fatal error and instead display what we were able to decode.

Comment 4

[Timothy Nikkel (:tnikkel)]

So you'll integrate this patch into libpng?

Comment 5

[Glenn Randers-Pehrson]

I've raised this for discussion on [e-mail]

Comment 6

[Cosmin Truta]

This is a correct PNG container wrapped around an incorrect zlib data stream, which means that the whole package is an incorrect PNG image file.

You can fix the image file with "optipng -fix".

You cannot see the error using pngcheck, because pngcheck, apparently, doesn't check for this sort of error. But you can see the error if you extract the raw zlib datastream and feed it into a generic zlib decompressor. (I tried zlib.decompress() from Python, and it confirmed the error.)

What exactly is the error?

It's this: the zlib stream contains all the PNG pixel data, but it is not terminated correctly. The last-block marker isn't present.

Therefore, if you stop decoding when you have all pixels (like pngcheck does, I suppose), you don't notice the error. But if you continue decoding until the zlib decoder tells you that it's done (with the expectation that it will tell this to you eventually), with no end-of-stream in sight, but running out of bytes, then you're unpleasantly surprised with an "incomplete zlib stream" kind of error.

Here is what a correct PNG decoder (including pngcheck) should do:

if (all pixels are decoded)
{
    /* check the end of the deflate stream */
    if (not(deflate stream is terminated correctly))
    {
        benign_error("bad or missing end of deflate stream "
                     "after pixel data")
        return
    }

    /* check the end of the zlib stream */
    if (not(adler32 checksum is present))
    {
        benign_error("missing Adler-32 checksum in zlib stream")
        return
    }
    if (not(adler32 checksum is verified))
    {
        benign_error("incorrect Adler-32 checksum in zlib stream")
        return
    }
}

Notes:

(1) The case of spurious trailing data in IDAT (which may happen, although that's not the case with this particular test image) is handled in the first benign_error().

(2) There is a difference between a CRC-32 error in IDAT (which indicates corruption, and which should not be a benign error), versus an Adler-32 error (which indicates faulty encoding, possibly recoverable, hence a benign error).

Comment 7

[Glenn Randers-Pehrson]

  It would
be useful to know if Firefox built with the "system libpng" (and "System zlib")
exhibits the behaviour.

Comment 8

[Glenn Randers-Pehrson]

It is possible to build libpng16 to ignore trailing errors (CRC and ADLER32)
but I'm not sure what happens if the trailing data is  entirely missing. grep
the libpng source or manpage for IGNORE to find out how.

Comment 9

[Cosmin Truta]

(In reply to Glenn Randers-Pehrson from comment #8)
> It is possible to build libpng16 to ignore trailing errors (CRC and ADLER32)
> but I'm not sure what happens if the trailing data is  entirely missing. grep
> the libpng source or manpage for IGNORE to find out how.

I'm afraid it won't help with this test case. Checking for the end of deflate stream should come before checking for Adler-32 (as in my pseudocode above), and if the first check fails, ignoring the second check won't make a difference.

If the png reader does not refrain from calling the zlib reader after reading all pixels, which in turn does not refrain from  expecting more deflate-encoded data, then the error occurs due to running out of compressed input data (i.e. running out of IDAT content). This is where the "Not enough image data" error comes from.

It won't even get to the Adler32/CRC32 checking stage, because the subsequent Adler32 and CRC32 checksums are mistaken for compressed input data.

Comment 10

[Cosmin Truta]

I stepped with a debugger through the Chromium's embedded libpng code, and I noticed that it raises the same png_error "not enough compressed data", just like Mozilla's. The difference is that they (Chromium) choose to display the image anyway, and not propagate the error further down the stack.

Comment 11

[BugBot [:suhaib / :marco/ :calixte]]

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:tnikkel, could you have a look please?

Comment 12

[Timothy Nikkel (:tnikkel)]

Nah, if you read the comments the patch was pretty much rejected.

Comment 13

[BugBot [:suhaib / :marco/ :calixte]]

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:tnikkel, could you have a look please?