Closed Bug 1283961 Opened 4 years ago Closed 3 years ago

Make the PNG decoder rely on SurfacePipe to determine if an image is too large to decode

Categories

(Core :: ImageLib, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: seth, Assigned: glennrp+bmo)

References

Details

Attachments

(2 files, 5 obsolete files)

In bug 1240665 we are failing to decode a PNG file that we easily *could* decode because the width is larger than MOZ_PNG_MAX_DIMENSION. However, it'd actually be fine to decode that image, as the height is very small. We should stop doing the MOZ_PNG_MAX_DIMENSION check and rely on the SurfacePipe code to decide whether we can decode the image. (i.e., if CreatePipe() returns a valid pipe, it's OK to proceed) This is what most of the other decoders do.
We also need to backout a change in pnglibconf.h. It should be safe unless upstream libpng has a bug.
(In reply to Masatoshi Kimura [:emk] from comment #1)
> We also need to backout a change in pnglibconf.h. It should be safe unless
> upstream libpng has a bug.

Thanks, that's good to know.

Glenn, can you confirm that this change is safe?
Flags: needinfo?(glennrp+bmo)
The limits were motivated by bug #542605 in which Cairo imposed a 32k x 32k limit and we observed that limit in the PNG decoder.
The limit in libpng itself is 1,000,000 x 1,000,000 by default but can be safely increased to much larger numbers (2.4G x 2.4G on 64-bit platforms, something like 16M x 2.4G on 32-bit platforms.  The 1,000,000 limit in libpng was chosen to provide a reasonable defense against DoS with large images.
Flags: needinfo?(glennrp+bmo)
(In reply to Glenn Randers-Pehrson from comment #3)
> The limits were motivated by bug #542605 in which Cairo imposed a 32k x 32k
> limit and we observed that limit in the PNG decoder.

Yeah, but please do not impose the limit in the image decoder.
Even though cairo (and today other backends[1]) has the limit, Firefox can display attachment 8709295 [details] if the image is scaled down and the image decoder does not have this limit.

[1] https://dxr.mozilla.org/mozilla-central/search?q=file%3ADrawTarget+32767&redirect=false
v00: override libpng's 1,000,000 limit on dimensions with 0x7fffffff (unlimited).
With the v00 patch I'm able to display very wide images (tested widths 70k, 200k, and 2M, all 10 pixels high).  When I also apply the v04 patch from bug #1240665, I see

  [ImgDecoder #1]: E/PNGDecoder libpng error: CreateFrame failed
  [ImgDecoder #2]: E/PNGDecoder libpng error: CreateFrame failed
  [ImgDecoder #1]: E/PNGDecoder libpng error: CreateFrame failed

in the console log, although the images still appear to be displayed properly.  I don't see this error messages for images that have widths less than 32k.
Comment on attachment 8767427 [details] [diff] [review]
v00-1283961-remove-png-dimension-limits.diff

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

Just took a look at your patch, Glenn. Thanks for putting this together! One question, though:

::: image/decoders/nsPNGDecoder.cpp
@@ +553,5 @@
>    png_get_IHDR(png_ptr, info_ptr, &width, &height, &bit_depth, &color_type,
>                 &interlace_type, &compression_type, &filter_type);
>  
>    // Are we too big?
> +  if (width > MOZ_PNG_MAX_WIDTH || height > MOZ_PNG_MAX_HEIGHT) {

Do we need this check anymore, Glenn? Allocating the surface will fail if the image is too large to be allocated, and the total amount of memory we permit to be used by image surfaces is 1GB. (I'm considering bumping that to 4GB on 64-bit platforms.) libpng itself will trigger a failure if the image is over 2GB in width or height, based upon the limits we set in pnglibconf.h, and presumably also the call to png_set_user_limits(). Certainly we should check if there are any concerns about overflow, but I suspect the failure to allocate a surface and the internal checks in libpng will be enough that we'll bail before an issue arises. So I *think* we can skip this check totally, but I'd like to hear your thoughts.
(In reply to Glenn Randers-Pehrson from comment #6)
>   [ImgDecoder #1]: E/PNGDecoder libpng error: CreateFrame failed

I'll investigate this.
Needinfo'ing Glenn to hear his thoughts on comment 7.
Flags: needinfo?(glennrp+bmo)
No problem. The limit in the PNG decoder is only there because there was an overall limit of 32k x 32k, so it gave us a quick bail-out of a too-large image.  If Mozilla is now doing its own calculation of image limits, we don't need to duplicate that
in the PNG decoder.  The v00 patch that I provided a few days ago removes the check from the PNG decoder.

But see the error messages that ensued, in comment #6.  Is there something we need to do outside of the PNG decoder to avoid those?
Flags: needinfo?(glennrp+bmo)
Oh, sorry, I missed the point of your question.  Yes, it should be OK to remove the "Are we too big?" check because libpng will have already done the check within png_get_IHDR().
Removed redundant test on image size
Attachment #8767427 - Attachment is obsolete: true
Rebased.
Attachment #8769025 - Attachment is obsolete: true
Rebased
Attachment #8770281 - Attachment is obsolete: true
Attachment #8772924 - Flags: review?(seth)
Split patch in two parts to simplify rebasing.
Attachment #8772924 - Attachment is obsolete: true
Attachment #8772924 - Flags: review?(mozilla.bugzilla)
Attachment #8773387 - Flags: review?(mozilla.bugzilla)
Rebased.
Attachment #8773388 - Flags: review?(mozilla.bugzilla)
Please "try" with both patches applied.
Flags: needinfo?(ryanvm)
Try failed due to a mistake in the patch for bug #1240665, now fixed.
Please retry.
Flags: needinfo?(ryanvm)
Assignee: seth.bugzilla → glennrp+bmo
Status: NEW → ASSIGNED
Comment on attachment 8773387 [details] [diff] [review]
v04-1283961-part01-remove-png-dimension-limits-libpng

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

Looks good. Thanks for tackling this, Glenn!
Attachment #8773387 - Flags: review?(seth.bugzilla) → review+
Comment on attachment 8773388 [details] [diff] [review]
v04-1283961-part02-remove-png-dimension-limits-nsPNGDecoder

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

Looks good!

::: image/decoders/nsPNGDecoder.cpp
@@ +324,5 @@
>                                (int)sizeof(unused_chunks)/5);
>  #endif
>  
>  #ifdef PNG_SET_USER_LIMITS_SUPPORTED
> +  png_set_user_limits (mPNG, MOZ_PNG_MAX_WIDTH, MOZ_PNG_MAX_HEIGHT);

Minor style nit: Should be no space between |png_set_user_limits| and |(mPng|.
Attachment #8773388 - Flags: review?(seth.bugzilla) → review+
nit fixed
Attachment #8773388 - Attachment is obsolete: true
Please check in both parts
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e271858ba5e
Part 1: Remove limits on PNG image dimensions (libpng). r=seth
https://hg.mozilla.org/integration/mozilla-inbound/rev/7841d3498cee
Part 2: Remove limits on PNG image dimensions (nsPNGDecoder). r=seth
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0e271858ba5e
https://hg.mozilla.org/mozilla-central/rev/7841d3498cee
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Blocks: 591822
You need to log in before you can comment on or make changes to this bug.