560 bytes, text/html
22.54 KB, text/plain
1.65 KB, patch
|Details | Diff | Splinter Review|
1.84 KB, patch
|Details | Diff | Splinter Review|
The attached test case crashes the latest ASAN build of Firefox. ASAN output attached in crash.txt. The imgLoader object is freed while setting a new src in the onresize event handler.
This should have been the ASAN output
Attachment #8395672 - Attachment is obsolete: true
Ugh. The layout flush GetWidthHeightForImage does can nuke the image request member, presumably. We should protect agains that, but why are we firing resize sync on layout flush? It seems weird to me to do that.
That sync resize firing is a pre-refreshdriver thing. IIRC the idea was to fire the event before we do any painting. https://bugzilla.mozilla.org/show_bug.cgi?id=472730#c6 We should make resize event to fire at refreshdriver tick. But flushing may run scripts anyway, so resize event isn't the issue.
> But flushing may run scripts anyway Sure. The question is what this method should do if those scripts change the image size by loading a new image. That is, is the right fix to use the post-flush image or to use the pre-flush image but keep it alive? In the case when resize fires sync, the difference is web-detectable pretty easily. If not, then I'm not sure it is, so we can just do whatever is simplest.
I still don't see how resize is interesting, since flush may run other random scripts. I think we should flush, and then take request. Could GetWidthHeightForImage take a nsCOMPtr<imgIRequest>& aRequest, so that when we access it, we access the current value?
Comment on attachment 8396489 [details] [diff] [review] GetWidthHeightForImage_crash.diff Add a comment about how the argument is a reference because the function might change the value and we want to use the updated value if it does that? r=me with that.
Attachment #8396489 - Flags: review?(bzbarsky) → review+
[Security approval request comment] How easily could an exploit be constructed based on the patch? It is a kungfuDeathGrip style fix, but an usual variant of it, so perhaps not so obvious. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? Check-in comment should be something like "Return width/height from the most recent image request" Which older supported branches are affected by this flaw? Looks like a regression from bug 683855, so all the branches If not all supported branches, which bug introduced the flaw? See above Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? The patch seems to apply at least to beta/aurora, and with some fuzz to esr24 How likely is this patch to cause regressions; how much testing does it need? Unlikely to cause regressions
Attachment #8396521 - Flags: sec-approval?
Comment on attachment 8396521 [details] [diff] [review] +comment sec-approval+ for trunk. Please get it in and nominate Aurora, Beta, and ERS24 patches.
Attachment #8396521 - Flags: sec-approval? → sec-approval+
https://hg.mozilla.org/mozilla-central/rev/4ede230e0938 Please request aurora/beta/esr24 approval on this when you get a chance.
Comment on attachment 8396521 [details] [diff] [review] +comment [Approval Request Comment] Bug caused by (feature/regressing bug #): bug 683855 User impact if declined: sec-crit crash Testing completed (on m-c, etc.): Landed to m-c Risk to taking this patch (and alternatives if risky): Should be low risk String or IDL/UUID changes made by this patch: NA
Attachment #8396521 - Flags: approval-mozilla-esr24?
Attachment #8396521 - Flags: approval-mozilla-esr24+
Attachment #8396521 - Flags: approval-mozilla-beta?
Attachment #8396521 - Flags: approval-mozilla-beta+
Attachment #8396521 - Flags: approval-mozilla-aurora?
Attachment #8396521 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/8fac4b0aed5e https://hg.mozilla.org/releases/mozilla-beta/rev/dfb99f6e4843 https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/ab4a97b39d9d https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/ab6660d2d3eb https://hg.mozilla.org/releases/mozilla-esr24/rev/c54ad9af7c5c
I tried to reproduce this issues on Ubuntu 12.10 x86_64 using the attached testcase on the following ASAN builds: * 03/29 Firefox 28 * 03/23 Firefox 29 * 03/24 Firefox 30 Unfortunately I didn't get one crash on any of the builds. Nils, could you please verify that this is fixed for you on the versions where it was marked as such?
Ioana, I was able to reproduce the issue reliably before. I have now tried on the latest ASAN build from http://download.cdn.mozilla.net/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-linux64-asan/1396346523/ and can confirm that the issue doesn't reproduce anymore.
Thanks Nils! If you happen to have some free time soon, please also verify on: ftp://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-04-01-mozilla-beta-debug ftp://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-04-01-mozilla-aurora-debug ftp://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-04-01-mozilla-esr24-debug These will get released earlier than central and we'd like to know the bug is out for all of them.
Whiteboard: [asan] → [asan][adv-main29+][adv-esr24.5+]
You need to log in before you can comment on or make changes to this bug.