1237171 - crash in skia::ConvolveHorizontally_SSE2 when open certain icon images

Status: VERIFIED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Alice0775 White]

This bug was filed from the Socorro interface and is 
report bp-59a9cd92-e368-416b-9ece-e41082160106.
=============================================================

Firefox crashed.
See http://forums.mozillazine.org/viewtopic.php?p=14465705


Reproducible: 100%

Steps to Reproduce:
1. Go to https://duckduckgo.com/?q=how+to+hide+%2Btrending+google+plus&t=ffab&ia=social
2. Now edit the edit box to make the search string like this : how to hide +trending +"google plus" (thus, simply adding the "" around the google plus string).
3. Click the search button again, and scroll down using the mousewheel.

OR
1. Open URL
2. Scroll down using the mousewheel

Actual results:
Crash

Comment 1

[Alice0775 White]

Regression pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=92407172ab7ce2cdcbbd8e8d54fcc22c90abcb17&tochange=609fa3c015842fa5bb9592e590164ea79088d247

Regressed by: Bug 1204394

Comment 2

[Alice0775 White]

Attachment: reduced html

STR
1. Clear cache
2. Open index.html
3. (optionally) Clear cache and then Reload(F5)

Comment 3

[Alice0775 White]

Crashed on Ubuntu
bp-7d153fe2-d005-499e-9659-2afe52160106

Comment 4

[Gervase Markham [:gerv]]

Alice: how exactly do you think I can help with this bug?

Gerv

Comment 5

[Alice0775 White]

(In reply to Gervase Markham [:gerv] from comment #4)
> Alice: how exactly do you think I can help with this bug?
> 
> Gerv

At least, I think that the bunch of offending patch should be packed out from beta and aurora channel.

Comment 6

[Nicholas Nethercote [inactive]]

Gerv's involvement in bug 1204394 was minor. I will take a look at this crash tomorrow.

Comment 7

[Nicholas Nethercote [inactive]]

I can reproduce the crash. The test case can be reduced down to just the www.papervisions.com.ico image.

In a debug build there's an assertion failure instead:

> 68	  MOZ_ASSERT(mTargetSize.width <= aOriginalSize.width,
> (gdb) bt
> #0  0x00007fffe4cbc28a in mozilla::image::Downscaler::BeginFrame (
>     this=0x7fffcbcb9c18, aOriginalSize=..., aFrameRect=..., 
>     aOutputBuffer=0x7fffcbcbac00 "", aHasAlpha=false, aFlipVertically=true)
>     at /home/njn/moz/mi7/image/Downscaler.cpp:68
> #1  0x00007fffe4cf72d0 in mozilla::image::nsBMPDecoder::ReadBitfields (
>     this=0x7fffcbcb9c00, aData=0x7fffcbcba228 "", aLength=0)
>     at /home/njn/moz/mi7/image/decoders/nsBMPDecoder.cpp:669
> #2  0x00007fffe4d06a6e in mozilla::image::nsBMPDecoder::WriteInternal(char const*, unsigned int)::$_0::operator()(mozilla::image::nsBMPDecoder::State, char const*, unsigned long) const (this=0x7fffd0193318, 
>     aState=mozilla::image::nsBMPDecoder::State::BITFIELDS, 
>     aData=0x7fffcbcba228 "", aLength=0)
>     at /home/njn/moz/mi7/image/decoders/nsBMPDecoder.cpp:439
> #3  0x00007fffe4cf5e39 in mozilla::image::StreamingLexer<mozilla::image::nsBMPDecoder::State, 16ul>::Lex<mozilla::image::nsBMPDecoder::WriteInternal(char const*, unsigned int)::$_0>(char const*, unsigned long, mozilla::image::nsBMPDecoder::WriteInternal(char const*, unsigned int)::$_0) (this=0x7fffcbcb9d70, 
>     aInput=0x7fffcbcba228 "", aLength=0, aFunc=...)
>     at ../../../image/StreamingLexer.h:343
> #4  0x00007fffe4cf55e7 in mozilla::image::nsBMPDecoder::WriteInternal (
>     this=0x7fffcbcb9c00, aBuffer=0x7fffcbcba200 "(", aCount=40)
>     at /home/njn/moz/mi7/image/decoders/nsBMPDecoder.cpp:433
> #5  0x00007fffe4cb93a4 in mozilla::image::Decoder::Write (
>     this=0x7fffcbcb9c00, aBuffer=0x7fffcbcba200 "(", aCount=40)
>     at /home/njn/moz/mi7/image/Decoder.cpp:183

Comment 8

[Nicholas Nethercote [inactive]]

It looks like this can cause dangerous writes to memory. In the instance I just caught in my debugger it wrote to 0xe5e5e5e5e5e5e5e5.

Comment 9

[Nicholas Nethercote [inactive]]

Attachment: Improve a case where ICO and BMP files disagree on an image size

There's no test because this still causes assertions in debug builds :/

Comment 10

[Nicholas Nethercote [inactive]]

Here's my understanding of the regression.

The ICO dirEntry says the image is 0x0, which is ICO-speak for 256x256. (Yes
really! See nsICODecoder::GetReal{Height,Width}().) At this stage, we try the
ICO dirEntry, so RasterImage::mSize is set to 256x256, via the PostSize() call
in nsICODecoder.

Then we set up the Downscaler using RasterImage::mSize (256x256) size and a
target size of 16x16, which seems ok.

But then we start decoding the BMP, which is truly 0x0, and call
Downscaler::BeginFrame(), passing it 0x0 for the size. It sanity checks 0x0
against the previously specified target size of 16x16 and asserts because we
shouldn't try to downscale a 0x0 image to a target size of 16x16.

Ideally, we'd completely ignore the ICO dirEntry size and always work with the
BMP size. (Indeed, later on we override the former with the latter; see
nsICODecode::FixBitMap{Width,Height}().) But it's hard to get the BMP size
early because that would hurt the speed of metadata-only decodes. This came up
in a similar context in https://bugzilla.mozilla.org/show_bug.cgi?id=1223319#c7.

We really need a general solution to the ICO-size-doesn't-match-BMP-size
problem; it's come up in several bugs now. But I don't know what that solution
is, so in the meantime I'm doing a simple work-around that fixes the regression
introduced by bug 1204394.

Comment 11

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/edf79b699755aaa8f0da7850ff0b2cccd9135c4e
Bug 1237171 - Improve a case where ICO and BMP files disagree on an image size. r=tn.

Comment 12

[Andrew McCreight [:mccr8]]

This bug needs sec-approval filled out because it is a sec-high or sec-critical bug that affects more than trunk.

Comment 13

[Nicholas Nethercote [inactive]]

Comment on attachment 8705402 [details] [diff] [review]
Improve a case where ICO and BMP files disagree on an image size

> [Security approval request comment]
> How easily could an exploit be constructed based on the patch?

No idea. But writes to unaddressable memory are generally dangerous, and it's easy to craft a BMP that triggers this behaviour.

> Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

The log message is vague. The patch has a comment explaining the existence of an exceptional case, but says nothing about the consequence of failing to handle that exceptional case.

> Which older supported branches are affected by this flaw?

It's on Aurora and Beta. It hasn't made it to the Release channel yet.

> If not all supported branches, which bug introduced the flaw?

Bug 1204394.

> Do you have backports for the affected branches? If not, how different, hard > to create, and risky will they be?

The same patch should apply without problem to Aurora and Beta.

> How likely is this patch to cause regressions; how much testing does it need?

Very unlikely to cause regressions. It's trivial.

Comment 14

[Nicholas Nethercote [inactive]]

Comment on attachment 8705402 [details] [diff] [review]
Improve a case where ICO and BMP files disagree on an image size

Approval Request Comment
[Feature/regressing bug #]: bug 1204394.

[User impact if declined]: crashes, potential security flaws.

[Describe test coverage new/current, TreeHerder]: no test coverage, unfortunately, because the triggering test case causes a (semi-related) assertion failure in debug builds.

[Risks and why]: low risk. The patch is trivial.

[String/UUID change made/needed]: none.

Comment 15

[Al Billings [:abillings - ex-MoCo]]

Let's fix this before we ship the problem. Approvals given.

Comment 16

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8705402 [details] [diff] [review]
Improve a case where ICO and BMP files disagree on an image size

Does this patch apply to Aurora? I don't see a nomination for that.

Comment 17

[Nicholas Nethercote [inactive]]

> Does this patch apply to Aurora? I don't see a nomination for that.

It does. I thought I nominated but I must have misclicked.

Comment 18

[Nicholas Nethercote [inactive]]

Can I land this on Aurora/Beta immediately, or do I have to wait? (I can never remember the drill with security bugs, sorry.)

Comment 19

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/edf79b699755

Comment 20

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8705402 [details] [diff] [review]
Improve a case where ICO and BMP files disagree on an image size

Just like for beta.

Comment 21

[Carsten Book [:Tomcat]]

(In reply to Sylvestre Ledru [:sylvestre] from comment #20)
> Comment on attachment 8705402 [details] [diff] [review]
> Improve a case where ICO and BMP files disagree on an image size
> 
> Just like for beta.

caused bustage and got backed out in https://treeherder.mozilla.org/logviewer.html#?job_id=725251&repo=mozilla-beta

Comment 22

[Nicholas Nethercote [inactive]]

Attachment: Improve a case where ICO and BMP files disagree on an image size

Here's a trivial change to the patch to make it compile on Beta. Apologies for
the bustage!

Comment 23

[Nicholas Nethercote [inactive]]

Comment on attachment 8706669 [details] [diff] [review]
Improve a case where ICO and BMP files disagree on an image size

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

Re-requesting approval for Beta; the other patch had a trivial compile error due to minor branch differences.

Comment 24

[Nicholas Nethercote [inactive]]

Comment on attachment 8705402 [details] [diff] [review]
Improve a case where ICO and BMP files disagree on an image size

Clearing Beta approval for the patch that doesn't build on Beta.

Comment 25

[Nicholas Nethercote [inactive]]

Tomcat: mccr8 suggested that I don't bother re-getting Beta approval, and that you should be able to land the new patch on Beta immediately. I'll let you decide if you're comfortable with that :)

Comment 26

[Nicholas Nethercote [inactive]]

I confirmed that the first patch (the one currently nominated for Aurora approval) applies and builds correctly on Aurora.

Comment 27

[Carsten Book [:Tomcat]]

(In reply to Nicholas Nethercote [:njn] from comment #25)
> Tomcat: mccr8 suggested that I don't bother re-getting Beta approval, and
> that you should be able to land the new patch on Beta immediately. I'll let
> you decide if you're comfortable with that :)

yeah sure mccr8 is right as always :) landed in https://hg.mozilla.org/releases/mozilla-beta/rev/72ae67129ad7

Comment 28

[Carsten Book [:Tomcat]]

make that https://hg.mozilla.org/releases/mozilla-beta/rev/40e457de9ce8

Comment 29

[Ada [:adalucinet]]

Reproduced the crash on 44.0b4 → bp-8a9ad111-1263-4806-9c6f-7723a2160112, under Windows 7 64-bit.
Verified fixed with latest Nightly 46.0a1 (from 2016-01-11), across platforms [1] - no crash encountered with any of the provided STR.

Although this fix shows up in 44 beta 8 pushlog, I suspect it didn’t get into this one because it's still crashing, across platforms - E.g.: bp-3876af75-f841-41f0-bb08-5ca3f2160112

Could you please confirm?

[1] Windows 7 64-bit, Ubuntu 14.04 32-bit and Mac OS X 10.10.5

Comment 30

[Carsten Book [:Tomcat]]

(In reply to Alexandra Lucinet, QA Mentor [:adalucinet] from comment #29)
> Reproduced the crash on 44.0b4 → bp-8a9ad111-1263-4806-9c6f-7723a2160112,
> under Windows 7 64-bit.
> Verified fixed with latest Nightly 46.0a1 (from 2016-01-11), across
> platforms [1] - no crash encountered with any of the provided STR.
> 
> Although this fix shows up in 44 beta 8 pushlog, I suspect it didn’t get
> into this one because it's still crashing, across platforms - E.g.:
> bp-3876af75-f841-41f0-bb08-5ca3f2160112
> 
> Could you please confirm?
> 
> [1] Windows 7 64-bit, Ubuntu 14.04 32-bit and Mac OS X 10.10.5

Hi Alexandra,

It might show up because it was shortly in and then got backedout (comment #21) for causing build bustage, so the fix landed in #comment 28 is not on beta 8

Comment 31

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Tomcat, I am confused. Does the patch need beta+? Based on some of the comments, it seems it already landed on m-b. Right?

Comment 32

[Nicholas Nethercote [inactive]]

(In reply to Ritu Kothari (:ritu) from comment #31)
> Hi Tomcat, I am confused. Does the patch need beta+? Based on some of the
> comments, it seems it already landed on m-b. Right?

It did land; the revised version was simple enough that Tomcat landed without waiting for a second approval. But if you (or someone else) could re-give approval just for completeness, that would be good.

Also, the other patch still needs Aurora approval and landing. It should be a rubber stamp given that it's almost identical to the already-landed beta patch.

Thank you.

Comment 33

[Andrew McCreight [:mccr8]]

Comment on attachment 8706669 [details] [diff] [review]
Improve a case where ICO and BMP files disagree on an image size

Yes, this was landed on beta per comment 28.

Comment 34

[Daniel Veditz [:dveditz]]

Comment on attachment 8706669 [details] [diff] [review]
Improve a case where ICO and BMP files disagree on an image size

[Triage Comment]
This appears to be what landed on beta: restoring flags

Comment 35

[Daniel Veditz [:dveditz]]

Comment on attachment 8705402 [details] [diff] [review]
Improve a case where ICO and BMP files disagree on an image size

a=dveditz for aurora (and restoring sec-approval)

Comment 36

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/40e457de9ce8

Comment 37

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c402368448d5

Comment 38

[Ada [:adalucinet]]

Verified fixed with 44.0b9 (Build ID: 20160114165817), across platforms [1].

[1] Mac OS X 10.10.4, Windows 7 64-bit and Ubuntu 12.04 32-bit