Closed Bug 1206836 Opened 6 years ago Closed 6 years ago
If we're downscaling an ICO during decode, downscale the AND mask as well
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.
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.
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.
Attachment #8663838 - Flags: review?(tnikkel)
So this would affect 43 and 44? Or other branches as well? Should this have sec-approval to land?
(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.
Just discussed this with Al; since the bug is only on trunk right now, this can land without sec-approval.
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.
Attachment #8663838 - Flags: review?(tnikkel) → review+
Thanks for the review!
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.
This survived on m-c and made it into the Nightly, so I'm going to mark this bug resolved.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
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.
(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 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.
Attachment #8663838 - Flags: approval-mozilla-aurora?
Can someone open this bug up? Do we need to keep it closed anymore at this point?
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
And if the result of looking it over means unhiding this bug, great.
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.
Attachment #8663838 - Flags: sec-approval?
Comment on attachment 8663838 [details] [diff] [review] When downscaling ICOs, downscale the AND mask as well Thank you for the data.
Attachment #8663838 - Flags: sec-approval? → sec-approval+
Attachment #8663838 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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 ?
(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.
Setting the status flags to fixed for 43 and 44.
You need to log in before you can comment on or make changes to this bug.