New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 827754 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Apr 2018
Cc:
Components:
EstimatedDays: ----
NextAction: 2018-04-16
OS: Linux , Windows , Mac
Pri: 1
Type: Bug-Regression



Sign in to add a comment

Regression in M64: failed to upload PNG file with huge XMP content

Project Member Reported by marchuk@chromium.org, Mar 31 2018

Issue description

Chrome version: 64, 65
OS version: Windows, MacOS
Case#:15334733 (premium customer, Netflix)

Description:
Sometimes Adobe products are creating PNG files with huge number <photoshop:DocumentAncestors> tags ,which are not violating XMP standard.

Unable to upload such files e.g. using Javascript window.URL.createObjectURL(file);
Sample usage script is located at https://jsfiddle.net/t4d5seht/
Sample file and XMP file properties export attached.

Steps to reproduce: 
Navigate to https://jsfiddle.net/t4d5seht/ (attached as well) on Windows or MacOS on version 64+
Upload attached PARTNER DELIVERED FAIL.png using button

Current Behavior / Reproduction: 
as per actual.png

File(4829903) {name: "PARTNER DELIVERED FAIL.png", lastModified: 1522454687126, lastModifiedDate: Fri Mar 30 2018 17:04:47 GMT-0700 (Pacific Daylight Time), webkitRelativePath: "", size: 4829903, …}
a.html:33 Event {isTrusted: true, type: "error", target: img, currentTarget: img, eventPhase: 2, …}
(anonymous) @ a.html:33
error (async)
onchange @ a.html:31

Expected Behavior: 
as per expected.png

Working with no issues in version 63 and less.
No issues on any Safari, IE, Edge, Firefox
 
a.html
1.1 KB View Download
x-export.xml
4.7 MB View Download
expected.png
199 KB View Download
actual.png
98.6 KB View Download
Labels: -Type-Bug Needs-Bisect Needs-Feedback Type-Bug-Regression
NextAction: 2018-04-16
Requesting bisect. I suspect this is a decoder issue.
Cc: abu...@netflix.com
Cc: -abu...@netflix.com
Played with stable versions, so if it helps with bissecting: 

64.0.3245.0 -- good
64.0.3245.2 (clang) -- good
64.0.3246.0 --bad
64.0.3246.2 (clang)--bad

Suspecting "Update libpng to 1.6.34"
https://chromium.googlesource.com/chromium/src/+/f82653a473f8de5fc86d0f2ecc75f6237e61946b

which was initially landed in 64.0.3246.0

Comment 5 by noel@chromium.org, Apr 3 2018

Cc: scroggo@chromium.org
Cc: nyerramilli@google.com
Labels: -Needs-Feedback -Needs-Bisect Target-67 Target-66 Target-65 M-65 hasbisect OS-Linux
Owner: khushals...@chromium.org
Status: Assigned (was: Untriaged)
Able to reproduce the issue on latest stable-65.0.3325.181 & Canary-67.0.3386.0 as per C#0 on mac 10.13.3,Windows 7 & Debian Rodate OS.

Good build: 64.0.3245.0  
Bad build: 64.0.3246.0  

Providing bisect info from Omahaproxy as we are not seeing revision numbers for the above builds,hence unable to provide tool bisect info.

Change Log:
----------
https://chromium.googlesource.com/chromium/src/+log/64.0.3245.0..64.0.3246.0?pretty=fuller&n=10000

From the above changelog suspecting khushalsagar@ to take a look into this issue and reassign to the right owner if it is not related to your change.

Thanks in advance..!

Owner: scroggo@chromium.org
No, this is almost certainly the libpng roll.
Description: Show this description
Cc: abu...@netflix.com
Labels: -Restrict-View-Google
--RVG, as fixed description.
Status: Started (was: Assigned)
Yes, this is due to the libpng roll.

In 1.6.32 (specifically the commit at [1]), a check was added to see if the size of a chunk was greater than the limit specified by PNG_USER_CHUNK_MALLOC_MAX. If it is, libpng will report an error and exit. It does this for all chunks, including chunks it will ignore. In this case, the chunk size is 4591191, which is greater than the 4000000L we set to fix  issue 117369 .

We ignore the iTXt chunk that holds this XMP data, but it looks like libpng allocates memory to hold it anyway, which is why it (now) checks to see whether it's over the limit before handling it (even if it won't be handled).

[1] https://github.com/glennrp/libpng/commit/347538efbdc21b8df684ebd92d37400b3ce85d55
What is stored in the XMP data? They don't affect how Chromium renders, but still have to be sent over the wire and read. We're actually exploring the idea of not sending such data to save bandwidth (see issue 823313).

But I guess the issue is that users are expected to be able to upload their own images? And their images could have arbitrarily sized iTXt chunks? That seems to indicate that we're not safe setting any limit. Some possible solutions:

- revert the update. this would bring back the regression ( issue 808665 ) from the last revert of the update. We could fix that by reapplying the optimizations to the 1.6.22 version of libpng
- patch upstream libpng. I'm not sure what the exact fix would be here. Maybe libpng should skip allocating memory for the chunks that are going to be ignored anyway. It is exiting out to prevent allocating more than our somewhat arbitrary limit, but since we're not going to use the chunk it seems like that's unnecessary.
- use a higher value for PNG_USER_CHUNK_MALLOC_MAX. I tried 5000000, and it happens to support the image that happens to be attached here, and it would be an easy fix without much downside (potentially allocate blocks for a meg larger chunks). It doesn't regress  issue 117369  either. (I tried no limit, which does regress that issue.)
- parse the individual chunks of the file in PNGImageReader, and never pass iTXt to libpng. We already do our own parsing until we reach the IDAT in order to look for APNG chunks, which we also do not pass to libpng. This could go a couple of levels. I think it is recommended to use such a chunk before IDAT (I'll see if I can figure out where I read that). If we only consider them then, that would be simpler, because it fits into the code we already parse. But if such a large iTXt chunk is used in an APNG, we may later pass it to libpng in StartFrameDecoding. This is for simplicity; we create a new png_struct and use the data from the beginning. We could mark any unneeded chunks and not pass them to libpng in that case, too. (This could also be used to fix a bug where Chromium doesn't work with a patched version of libpng with added apng support.) For text chunks that come after the (first) IDAT, we'd have to add more parsing code (we never parse beyond IDAT for non-animated PNGs; we just pass the data blindly to libpng, which knows what to do with it).
- figure out why Mozilla works. As far as I can tell, they use the same value for PNG_USER_CHUNK_MALLOC_MAX (see https://dxr.mozilla.org/mozilla-central/source/media/libpng/pnglibconf.h), and they are using libpng 1.6.34 (https://dxr.mozilla.org/mozilla-central/source/media/libpng/README). At least my version (52.6.0 (64-bit)) is working; I don't know how recent that is, or whether it's using 1.6.34
I've built a more recent version of FireFox (61.0a1) to try and figure out how the file works there, and it no longer does. I get the message "The image "..." cannot be displayed because it contains errors." I suspect this is for the same reason it fails to decode in Chromium. I filed https://bugzilla.mozilla.org/show_bug.cgi?id=1452735 for FireFox. If it should work in Chromium, it seems that it should also work there.

I filed https://github.com/glennrp/libpng/issues/217 to see if libpng might be interested in making it possible to really skip the chunk.
The NextAction date has arrived: 2018-04-16
Another option: change the error to a benign error, as suggested in [1]. Uploaded https://chromium-review.googlesource.com/c/chromium/src/+/1014027

Is this image copyright free so that we can use it in a regression test? Or is it easy to create one?

[1] https://github.com/glennrp/libpng/issues/217#issuecomment-381256881
We got approval for using image below (RVG)

https://drive.google.com/open?id=1LIWe1kUWrRfN4pWvyosC7vPqq-A_xpGL
Project Member

Comment 16 by bugdroid1@chromium.org, Apr 18 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e87a02987101e2dbe319a4aba6b52470f7624b4a

commit e87a02987101e2dbe319a4aba6b52470f7624b4a
Author: Leon Scroggins III <scroggo@google.com>
Date: Wed Apr 18 21:45:46 2018

Turn large PNG chunks into benign errors

Bug:  827754 

A recent change to libpng [1] (included in Chromium with the recent
libpng update [2]) turns chunks that are bigger than
PNG_USER_CHUNK_MALLOC_MAX into failures. Although this matches the
intent of PNG_USER_CHUNK_MALLOC_MAX, it also causes images which used to
be viewable in Chromium to fail. Changing to a benign error allows us to
display these images once again. Though it means we do allow libpng to
allocate more than PNG_USER_CHUNK_MALLOC_MAX, it matches the behavior
prior to [2] (when we were using 1.6.22), and it does not regress
 crbug.com/117369 

Add a regression test. The image is supplied in the bug, and has approval
to be checked in [3].

[1] https://github.com/glennrp/libpng/commit/347538efbdc21b8df684ebd92d37400b3ce85d55
[2] https://chromium.googlesource.com/chromium/src/+/f82653a473f8de5fc86d0f2ecc75f6237e61946b
[3] https://bugs.chromium.org/p/chromium/issues/detail?id=827754#c15

Change-Id: Iaae884b42a94bdec6e1cfad97d46ef820a75a5f8
Reviewed-on: https://chromium-review.googlesource.com/1014027
Commit-Queue: Leon Scroggins <scroggo@chromium.org>
Reviewed-by: Noel Gordon <noel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#551838}
[add] https://crrev.com/e87a02987101e2dbe319a4aba6b52470f7624b4a/third_party/WebKit/LayoutTests/images/resources/crbug827754.png
[modify] https://crrev.com/e87a02987101e2dbe319a4aba6b52470f7624b4a/third_party/blink/renderer/platform/image-decoders/png/png_image_decoder_test.cc
[modify] https://crrev.com/e87a02987101e2dbe319a4aba6b52470f7624b4a/third_party/libpng/README.chromium
[modify] https://crrev.com/e87a02987101e2dbe319a4aba6b52470f7624b4a/third_party/libpng/pngrutil.c

Status: Fixed (was: Started)

Sign in to add a comment