1770513 - 19x200000 png fails to load

Status: ASSIGNED

(Core :: Graphics: ImageLib, defect)

Description

[boxed]

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

Steps to reproduce:

I made a little hobby page to show how binary numbers work. It's just one image. You can see it here: https://bits.kodare.com/

This works in Safari on desktop and iOS, and in Chrome.

Actual results:

The image displays as broken.

Expected results:

I expected the image to load.

Comment 1

[boxed]

Funnily Firefox does load it properly if you go directly to the image: https://bits.kodare.com/bits.png But it won't allow me to zoom enough to see it.. and the background around it isn't evenly black (I'm on dark mode), which seems very strange.

Comment 2

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

The Bugbug bot thinks this bug should belong to the 'Core::ImageLib' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.

Comment 3

[Bob Hood [:bhood]]

I get the same result with 100.0.2. Andrew, please have a look when you get a chance.

Comment 4

[Timothy Nikkel (:tnikkel)]

Hmm, we have a 64k limit in any one image dimension

https://searchfox.org/mozilla-central/rev/8bd5597baef9be63fc4689a69e550d607dc68444/image/SurfaceCache.cpp#1875

I wonder why.

Comment 5

[Timothy Nikkel (:tnikkel)]

For context, where we removed 32k limit on macos

https://bugzilla.mozilla.org/show_bug.cgi?id=1298652

Maybe

https://bugzilla.mozilla.org/show_bug.cgi?id=1298652#c6

is the reason we had the 64k limit? Maybe it can be increased now?

Comment 6

[Timothy Nikkel (:tnikkel)]

Looks like it comes from
https://bugzilla.mozilla.org/show_bug.cgi?id=255067

I guess the idea was to limit each dimension to 2^16 so their product would be limited to 2^32? Later we added a check for exactly that (including the fast we use 4 byte per pixel). So perhaps we can loosen this to just check that the bytes to hold the image are less than 2^31?

Comment 7

[Timothy Nikkel (:tnikkel)]

(In reply to boxed from comment #1)

Funnily Firefox does load it properly if you go directly to the image: https://bits.kodare.com/bits.png But it won't allow me to zoom enough to see it.. and the background around it isn't evenly black (I'm on dark mode), which seems very strange.

The size that the image is actually drawn to the screen is what determines if the size of the decoded surface is outside of the 64k limit or not. The background is imagedoc-darknoise.png, ie it's not intended to be a uniform color.

Comment 8

[Timothy Nikkel (:tnikkel)]

(In reply to Timothy Nikkel (:tnikkel) from comment #6)

Looks like it comes from
https://bugzilla.mozilla.org/show_bug.cgi?id=255067

I guess the idea was to limit each dimension to 2^16 so their product would be limited to 2^32? Later we added a check for exactly that (including the fast we use 4 byte per pixel). So perhaps we can loosen this to just check that the bytes to hold the image are less than 2^31?

And I believe the idea to limit each dimension to 2^16 wasn't fully successful because several years back we had some bugs come in where image dimensions overflowed 2^31 (signed int) but not 2^32, and we use signed ints in some places (it makes sense in some places, iirc the bmp spec is such that negatives make the implementation easier). And I think I audited every multiplication in imagelib that involved the word width or height at the time and fixed whatever I found.

Comment 10

[Timothy Nikkel (:tnikkel)]

Attachment: Bug 1770513. Relax 64k pixel limit on each dimension of images. r?jrmuizel!,aosmond!

See bug comments for results of my investigation into the history of this limit. It seems that there were two motivations for the limit.

One was CoreGraphics on macOS limits on image size. I'm not sure if that limit is still relevant.

The second seems to be limiting each dimension of the image to 2^16 so that their product is limited to 2^32 to avoid overflow in any calculations from bug 255067. This limit was not successful in preventing overflow because sometimes the calculation happens on signed ints, and we can still overflow 2^31, and some of the calculations are performed on bytes, so we add an extra factor of 4 into them. We had a period years back in imagelib where we fixed several calculations to avoid overflow because of this and I audited every multiplation in the image/ direction that involved the word width or height. Further, overflows of this type should be disallowed because we still reject images whose total byte storage for the decoded image surface would exceed a signed int (in addition to rejecting decoding of image surfaces that can't fit into the surface cache which by default is limited to 2GB).

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?
If you still have some work to do, you can add an action "Plan Changes" in Phabricator.
For more information, please visit auto_nag documentation.

Comment 12

[Thomas Wisniewski [:twisniewski]]

Hi Tim, did this bug slip under the radar? Would it be good to assign a priority?

Comment 13

[Timothy Nikkel (:tnikkel)]

I wanted to make sure that after this patch we wouldn't get into some negative feedback loop where we keep decoding an image and then failing to insert it into the surface cache and then trying to decode is again forever, see comment in the phab
https://phabricator.services.mozilla.com/D162256#5341749

As for priority, do we still use those? Thought we switched to severity.

Comment 14

[Thomas Wisniewski [:twisniewski]]

Ah, ok, I guess I've lost track of how we use Bugzilla fields myself :)

I'm fine with things as long as it doesn't completely go under the radar, thanks.