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

RESOLVED FIXED in Firefox 50

Status

()

Core
ImageLib
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: seth, Assigned: Glenn Randers-Pehrson)

Tracking

unspecified
mozilla50
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 fixed)

Details

Attachments

(2 attachments, 5 obsolete attachments)

(Reporter)

Description

2 years ago
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.
(Reporter)

Comment 2

2 years ago
(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)
(Assignee)

Comment 3

2 years ago
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
(Assignee)

Comment 5

2 years ago
Created attachment 8767427 [details] [diff] [review]
v00-1283961-remove-png-dimension-limits.diff

v00: override libpng's 1,000,000 limit on dimensions with 0x7fffffff (unlimited).
(Assignee)

Comment 6

2 years ago
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.
(Reporter)

Comment 7

2 years ago
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.
(Reporter)

Comment 8

2 years ago
(In reply to Glenn Randers-Pehrson from comment #6)
>   [ImgDecoder #1]: E/PNGDecoder libpng error: CreateFrame failed

I'll investigate this.
(Reporter)

Comment 9

2 years ago
Needinfo'ing Glenn to hear his thoughts on comment 7.
Flags: needinfo?(glennrp+bmo)
(Assignee)

Comment 10

2 years ago
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)
(Assignee)

Comment 11

2 years ago
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().
(Assignee)

Comment 12

2 years ago
Created attachment 8769025 [details] [diff] [review]
v01-1283961-remove-png-dimension-limits

Removed redundant test on image size
Attachment #8767427 - Attachment is obsolete: true
(Assignee)

Comment 13

a year ago
Created attachment 8770281 [details] [diff] [review]
v02-1283961-remove-png-dimension-limits

Rebased.
Attachment #8769025 - Attachment is obsolete: true
(Assignee)

Comment 14

a year ago
Created attachment 8772924 [details] [diff] [review]
v03-1283961-remove-png-dimension-limits

Rebased
Attachment #8770281 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8772924 - Flags: review?(seth)
(Assignee)

Comment 15

a year ago
Created attachment 8773387 [details] [diff] [review]
v04-1283961-part01-remove-png-dimension-limits-libpng

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)
(Assignee)

Comment 16

a year ago
Created attachment 8773388 [details] [diff] [review]
v04-1283961-part02-remove-png-dimension-limits-nsPNGDecoder

Rebased.
Attachment #8773388 - Flags: review?(mozilla.bugzilla)
(Assignee)

Comment 17

a year ago
Please "try" with both patches applied.
Flags: needinfo?(ryanvm)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c065d3613add13685c92cff220c68e4202150637
Flags: needinfo?(ryanvm)
(Assignee)

Comment 19

a year ago
Try failed due to a mistake in the patch for bug #1240665, now fixed.
Please retry.
Flags: needinfo?(ryanvm)
(Assignee)

Updated

a year ago
Assignee: seth.bugzilla → glennrp+bmo
Status: NEW → ASSIGNED
https://treeherder.mozilla.org/#/jobs?repo=try&revision=18b21945f8d6
Flags: needinfo?(ryanvm)
(Reporter)

Comment 21

a year ago
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+
(Reporter)

Comment 22

a year ago
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+
(Assignee)

Comment 23

a year ago
Created attachment 8775586 [details] [diff] [review]
v05-1283961-part02-remove-png-dimensions-limits-nsPNGDecoder

nit fixed
Attachment #8773388 - Attachment is obsolete: true
(Assignee)

Comment 24

a year ago
Please check in both parts
Keywords: checkin-needed

Comment 25

a year ago
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

Comment 26

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0e271858ba5e
https://hg.mozilla.org/mozilla-central/rev/7841d3498cee
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Blocks: 591822
You need to log in before you can comment on or make changes to this bug.