1206836 - If we're downscaling an ICO during decode, downscale the AND mask as well

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Seth Fowler [:seth] [:s2h]]

Bug 1201796 has an issue that can cause both rendering artifacts and memory corruption: when we downscale an ICO containing a BMP and an AND mask for transparency, we're not downscaling the AND mask. That causes us to write zeroes (1) in the wrong places in the image, and (2) past the end of the surface.

The solution is to downscale the AND mask as well, and then transfer the alpha values to the image.

This will fix all of the regressions that are currently blocking bug 1201796.

Comment 1

[Seth Fowler [:seth] [:s2h]]

I'm going to include a crashtest in the patch. We should really have a test that checks the rendered output too, but that should be done in a gtest and I need to add some infrastructure for that that I don't think belongs in this bug. Instead, I'll include an ICO-with-AND-mask test case in a general "gtests for downscale-during-decode" patch that I've been planning to write this week.

Comment 2

[Seth Fowler [:seth] [:s2h]]

Attachment: When downscaling ICOs, downscale the AND mask as well

Here's the patch. The idea is exactly what I proposed in comment 0: if we're
downscaling, downscale the AND mask separately, and then copy only the alpha
values from the downscaled AND mask to the final image.

I've tested and this resolves all of the regressions.

Comment 3

[Seth Fowler [:seth] [:s2h]]

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

Comment 4

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

So this would affect 43 and 44? Or other branches as well?
Should this have sec-approval to land?

Comment 5

[Seth Fowler [:seth] [:s2h]]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #4)
> So this would affect 43 and 44? Or other branches as well?
> Should this have sec-approval to land?

Since bug 1201796 got backed out from 43, for now this only affects 44. I want to reland bug 1201796 on 43, though, so this'll need to come with it.

I'm not sure about sec-approval. I made this a security bug mainly out of caution. I'll ping some security folks and try to figure out if that's necessary.

Comment 6

[Seth Fowler [:seth] [:s2h]]

Just discussed this with Al; since the bug is only on trunk right now, this can land without sec-approval.

Comment 7

[Seth Fowler [:seth] [:s2h]]

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

Comment 8

[Seth Fowler [:seth] [:s2h]]

The new try job in comment 7 was motivated by the crashtest failures in the previous try job. The failures are not for the new crashtest, and they don't appear related to this patch at all. I suspect that instead, they are triggered by the fact that my new crashtest uses reftest-zoom, which the |load| command we use for crashtests may not clean up after correctly. If that's the case, I'll fix that issue in a separate bug and then land the test.

We'll know if that's the case by whether the crashtest failures disappear in the new try job, which does not include my new crashtest.

Comment 9

[Seth Fowler [:seth] [:s2h]]

Thanks for the review!

Comment 10

[Seth Fowler [:seth] [:s2h]]

OK, everything looks good on that try job. (Unfortunately Windows jobs are taking *forever*, so I can't wait for them.)

The results also confirm that the crashtest issues in the first try job are the result of a harness bug. I'll fix the harness and the test in a separate bug.

We need this to end up in the next Nightly, so I'm going to push this directly to m-c.

Comment 11

[Seth Fowler [:seth] [:s2h]]

https://hg.mozilla.org/mozilla-central/rev/2235e56c94cf

Comment 12

[Seth Fowler [:seth] [:s2h]]

This survived on m-c and made it into the Nightly, so I'm going to mark this bug resolved.

Comment 14

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

Seth, does this still need uplift to 43 along with bug 1201796? Are you still working on it? I was just going through tracked bugs to unsnarl a bunch of duped bugs for 43, and they led here.

Comment 15

[Seth Fowler [:seth] [:s2h]]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #14)
> Seth, does this still need uplift to 43 along with bug 1201796? Are you
> still working on it? I was just going through tracked bugs to unsnarl a
> bunch of duped bugs for 43, and they led here.

Yes, but I want to be absolutely sure that we've resolved all the issues. Was planning to request uplift in another day or two.

Comment 16

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8663838 [details] [diff] [review]
When downscaling ICOs, downscale the AND mask as well

Approval Request Comment
[Feature/regressing bug #]: To support bug 1146663, which transitions us fully from the old "high-quality" downscaling code to downscale-during-decode. We measured a 96% reduction in time spent drawing images when painting the about:newtab page from making this change, so it'd be nice to get bug 1146663 into 43 if we can and ship those perf improvements.
[User impact if declined]: Can't uplift bug 1146663, which means we lose out on those performance gains for drawing large images, and in particular we lose out on about:newtab.
[Describe test coverage new/current, TreeHerder]: On m-c for weeks now, with tests. (And we're working to add more tests.)
[Risks and why]: Not expected to be high risk as the code has been on m-c for so long, and the regression it caused appears to be long since fixed.
[String/UUID change made/needed]: None.

Comment 17

[Seth Fowler [:seth] [:s2h]]

Can someone open this bug up? Do we need to keep it closed anymore at this point?

Comment 18

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

OK, I see why we didn't ask for sec-approval here originally,  but we probably should now. Would you mind filling out that form for Al or Dan to look over? Thanks

Comment 19

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

And if the result of looking it over means unhiding this bug, great.

Comment 20

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8663838 [details] [diff] [review]
When downscaling ICOs, downscale the AND mask as well

We've already landed this; I'm filling this out per the above comment as part of the Aurora uplift of the patch that caused the issue, together with this patch, and to try to unhide the bug.

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

The patch indicates that a buffer overflow existed. Since the original buffer overflow can only write zeroes for a small distance after the end of ICO image data, though, it's unclear to me how easy it would be to actually exploit. It's not possible to write arbitrary data.

Since the fix would land at the same time as the patch that introduced the buffer overflow, there wouldn't be a window in which someone could exploit this.

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

Probably not. The comments in this bug do, though.

Which older supported branches are affected by this flaw?

None, but I want to uplift both the original buggy patch *and* this fix simultaneously to Aurora.

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

Bug 1201796.

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

A backport is easy; that's what I'm trying to do.

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

Very unlikely as it's been on central for some time already.

Comment 21

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

Comment on attachment 8663838 [details] [diff] [review]
When downscaling ICOs, downscale the AND mask as well

Thank you for the data.

Comment 22

[Carsten Book [:Tomcat]]

Hey seth, this cause problems during uplift 

- grafting 302714:2235e56c94cf "Bug 1206836 - When downscaling ICOs, downscale the AND mask as well. r=tn a=KWierso"
merging image/decoders/nsBMPDecoder.h
merging image/decoders/nsICODecoder.cpp
merging image/decoders/nsICODecoder.h
warning: conflicts during merge.
merging image/decoders/nsICODecoder.h incomplete! (edit conflicts, then use 'hg resolve --mark')

could you take a look ?

Comment 23

[Seth Fowler [:seth] [:s2h]]

(In reply to Carsten Book [:Tomcat] from comment #22)
> Hey seth, this cause problems during uplift 

Yeah, this will require manual uplift I'm afraid. I'll take care of it.

Comment 24

[Seth Fowler [:seth] [:s2h]]

Uplifted:

https://hg.mozilla.org/releases/mozilla-aurora/rev/4e6a6482a65a

Comment 25

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

Setting the status flags to fixed for 43 and 44.