1368407 - Small PNG image hangs browser through memory consumption

Status: VERIFIED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[glen.carmichael]

Attachment: huge-malloc.png

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_4) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/58.0.3029.110 Safari/537.36

Steps to reproduce:

Open the attached PNG image.


Actual results:

Firefox tries to eat all the memory. I let it run to >50GB on macOS, which meant the kernel started to pause other processes/applications.

The image is only 41 bytes, but it defines its width as 2^32 - 1. This results in a huge malloc in libpng's png_read_start_row (pngrutil.c).


Expected results:

The image width should be verified or limited somehow. I believe it can be trivially fixed by defining PNG_MAX_MALLOC_64K for libpng.

Comment 1

[:Gijs (he/him)]

Bobby, based on bug 505762 (blast from the past...), seems you might know about this code - any chance you could take a look, and/or could recommend someone else? :-)

Comment 2

[Bobby Holley (:bholley)]

I stepped down from imagelib stuff years ago. You want whoever's maintaining imagelib now. I think that's tn now that seth is gone?

Comment 3

[Glenn Randers-Pehrson]

This is a known feature and probably doesn't need to be hidden.
At one time there existed a width and height limit of 32k but
that was removed a long time ago.  Libpng by default uses
1,000,000 by 1,000,000 limits, but we override those (in
media/libpng/pnglibconf.h) with 0x7ffffffff (2^31 -1) by 0x7fffffff
limits (essentially unlimited).  I don't remember the rationale
for doing that.

One can build with different limits by defining MOZ_PNG_MAX_WIDTH
and/or MOZ_PNG_MAX_HEIGHT.

Comment 4

[Glenn Randers-Pehrson]

Note that image/decoders/nsPNGDecoder.cpp also abides by
user-supplied MOZ_PNG_MAX_WIDTH and MOZ_PNG_MAX_HEIGHT
limits when using a system libpng instead of the in-tree
libpng.

Comment 5

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking for 55, not really a new issue, but if we land a fix we could still potentially take a patch for 54.

Comment 6

[Glenn Randers-Pehrson]

What would you think of making MOZ_PNG_MAX_WIDTH and _HEIGHT
(and maybe a new MOZ_PNG_MAX_PIXELS) settable in build:config ?

Comment 7

[Glenn Randers-Pehrson]

Or, more generally, MOZ_IMAGE_MAX_WIDTH, etc.?

Comment 8

[Timothy Nikkel (:tnikkel)]

Other places in imagelib we have used SurfaceCache::CanHold before making a potentially large allocation (even if the allocated data won't be inserted into the surface cache) to decide if we want to make the allocation to prevent making huge allocations that will likely never produce anything visible to the user.

Comment 9

[Glenn Randers-Pehrson]

In this case the alloc happens inside libpng where SurfaceCache::CanHold is not available.  I'll see what I can do.

Comment 10

[Glenn Randers-Pehrson]

Attachment: v00-1368407-check-png-width

This patch checks for width exceeding MaximumCapacity()/8 (because libpng will allocate two row buffers of 4*width).  It also checks, in the case of ADAM7 interlaced images if there is enough room to allocate the complete 4*width*height buffer that we require for deinterlacing.

Please submit it to try.

Comment 11

[Glenn Randers-Pehrson]

On my Ubuntu-16.04 platform, I get

Nightly: Attempts to allocate a huge amount of memory, then
[ImgDecoder #1]: E/PNGDecoder libpng error: Out of memory

Patched: Immediately abandons the PNG with this message
[ImgDecoder #1]: E/PNGDecoder libpng error: Image is too wide

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Sorry for the delay.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cf2da3c1a0b895657d4814e94fdcd42369e3048b

Comment 13

[Glenn Randers-Pehrson]

Comment on attachment 8874162 [details] [diff] [review]
v00-1368407-check-png-width

Try results seem ok

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4142967e1f23
Check for too-large PNG width. r=tn

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4142967e1f23

Comment 16

[Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact [e-mail]]

I reproduced this issue using Fx55.0a1 (2017-05-28) on macOS X 10.12.5.
I can confirm this issue is fixed, I verified using Fx 55.0b2 on macOS X 10.12.5.

Cheers!