Last Comment Bug 893308 - (CVE-2013-1722) Heap-use-after-free in nsAnimationManager::BuildAnimations
(CVE-2013-1722)
: Heap-use-after-free in nsAnimationManager::BuildAnimations
Status: RESOLVED FIXED
[asan][adv-main24+][adv-esr1709+]
: csectype-uaf, regression, sec-critical
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla26
Assigned To: David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-07-12 19:19 PDT by Abhishek Arya
Modified: 2014-11-19 20:12 PST (History)
16 users (show)
dchanm+bugzilla: sec‑bounty+
ryanvm: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
wontfix
+
verified
+
verified
+
verified
24+
verified
24+
fixed
fixed
fixed


Attachments
Testcase (522 bytes, application/x-zip-compressed)
2013-07-12 19:19 PDT, Abhishek Arya
no flags Details
patch 1: Make keyframes rule table hold strong references. (2.68 KB, patch)
2013-07-23 16:35 PDT, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
no flags Details | Diff | Splinter Review
patch 2: Call KeyframesListIsDirty when doing ClearRuleCascades. (4.11 KB, patch)
2013-07-23 16:36 PDT, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
bzbarsky: review+
abillings: sec‑approval+
Details | Diff | Splinter Review
move hash tble to RuleCascadeData (so we have one per cascade level and can manage it easily) (14.90 KB, patch)
2013-08-12 16:02 PDT, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
no flags Details | Diff | Splinter Review
move hash table to RuleCascadeData (so we have one per cascade level and can manage it easily) (14.97 KB, patch)
2013-08-13 14:39 PDT, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
cam: review+
bajaj.bhavana: approval‑mozilla‑aurora+
bajaj.bhavana: approval‑mozilla‑beta+
lukasblakk+bugs: approval‑mozilla‑esr17+
abillings: sec‑approval+
Details | Diff | Splinter Review

Description Abhishek Arya 2013-07-12 19:19:55 PDT
Created attachment 775067 [details]
Testcase

Need to install fuzzPriv extension [https://www.squarefree.com/extensions/domFuzzLite3.xpi]. Usually reproduces in one try, but do try with reload if it does not.

==1210==ERROR: AddressSanitizer: heap-use-after-free on address 0x61900d449420 at pc 0x7f183c1696ee bp 0x7fff6f500cd0 sp 0x7fff6f500cc8
READ of size 8 at 0x61900d449420 thread T0
    #0 0x7f183c1696ed in nsTArray_base<nsTArrayInfallibleAllocator, nsTArray_CopyElements<nsISupports*> >::Length() const ../../../../dist/include/nsTArray.h:371
    #1 0x7f183c1695d1 in nsCOMArray_base::Count() const ../../../../dist/include/nsCOMArray.h:95
    #2 0x7f18448ee188 in mozilla::css::GroupRule::StyleRuleCount() const ../../dist/include/mozilla/css/GroupRule.h:54
    #3 0x7f18448e8895 in nsAnimationManager::BuildAnimations(nsStyleContext*, nsTArray<ElementAnimation>&) layout/style/nsAnimationManager.cpp:783
    #4 0x7f18448e4f78 in nsAnimationManager::CheckAnimationRule(nsStyleContext*, mozilla::dom::Element*) layout/style/nsAnimationManager.cpp:568
    #5 0x7f1844e8b9ba in nsStyleSet::GetContext(nsStyleContext*, nsRuleNode*, nsRuleNode*, nsIAtom*, nsCSSPseudoElements::Type, mozilla::dom::Element*, unsigned int) layout/style/nsStyleSet.cpp:833
    #6 0x7f1844e9496a in nsStyleSet::ResolveStyleFor(mozilla::dom::Element*, nsStyleContext*, TreeMatchContext&) layout/style/nsStyleSet.cpp:1205
    #7 0x7f1843daf486 in nsFrameManager::ReResolveStyleContext(nsPresContext*, nsIFrame*, nsIContent*, nsStyleChangeList*, nsChangeHint, nsChangeHint, nsRestyleHint, mozilla::css::RestyleTracker&, nsFrameManager::DesiredA11yNotifications, nsTArray<nsIContent*>&, TreeMatchContext&) layout/base/nsFrameManager.cpp:1256
    #8 0x7f1843db79d9 in nsFrameManager::ComputeStyleChangeFor(nsIFrame*, nsStyleChangeList*, nsChangeHint, mozilla::css::RestyleTracker&, bool) layout/base/nsFrameManager.cpp:1695
    #9 0x7f1843a81991 in nsCSSFrameConstructor::RestyleElement(mozilla::dom::Element*, nsIFrame*, nsChangeHint, mozilla::css::RestyleTracker&, bool) layout/base/nsCSSFrameConstructor.cpp:8340
    #10 0x7f18439d12d2 in mozilla::css::RestyleTracker::ProcessOneRestyle(mozilla::dom::Element*, nsRestyleHint, nsChangeHint) layout/base/RestyleTracker.cpp:122
    #11 0x7f18439ccf11 in mozilla::css::RestyleTracker::DoProcessRestyles() layout/base/RestyleTracker.cpp:209
    #12 0x7f1843aa6a06 in mozilla::css::RestyleTracker::ProcessRestyles() layout/base/RestyleTracker.h:217
    #13 0x7f1843aa6397 in nsCSSFrameConstructor::ProcessPendingRestyles() layout/base/nsCSSFrameConstructor.cpp:12071
    #14 0x7f1843f384cc in PresShell::FlushPendingNotifications(mozilla::ChangesToFlush) layout/base/nsPresShell.cpp:3858
    #15 0x7f1843f37087 in PresShell::FlushPendingNotifications(mozFlushType) layout/base/nsPresShell.cpp:3743
    #16 0x7f1845a8456e in nsDocument::FlushPendingNotifications(mozFlushType) content/base/src/nsDocument.cpp:7085
    #17 0x7f184b9d1391 in nsDocLoader::DocLoaderIsEmpty(bool) uriloader/base/nsDocLoader.cpp:709
    #18 0x7f184b9d5b2b in nsDocLoader::OnStopRequest(nsIRequest*, nsISupports*, tag_nsresult) uriloader/base/nsDocLoader.cpp:639
    #19 0x7f184b9d71ea in non-virtual thunk to nsDocLoader::OnStopRequest(nsIRequest*, nsISupports*, tag_nsresult) uriloader/base/nsDocLoader.cpp:643
    #20 0x7f1841e32480 in nsLoadGroup::RemoveRequest(nsIRequest*, nsISupports*, tag_nsresult) netwerk/base/src/nsLoadGroup.cpp:684
    #21 0x7f1845a98575 in nsDocument::DoUnblockOnload() content/base/src/nsDocument.cpp:7965
    #22 0x7f1845a97fcb in nsDocument::UnblockOnload(bool) content/base/src/nsDocument.cpp:7893
    #23 0x7f1845a3b713 in nsDocument::DispatchContentLoadedEvents() content/base/src/nsDocument.cpp:4680
    #24 0x7f1845b52b1a in nsRunnableMethodImpl<void (nsDocument::*)(), true>::Run() ../../../dist/include/nsThreadUtils.h:350
    #25 0x7f18416e2feb in nsThread::ProcessNextEvent(bool, bool*) xpcom/threads/nsThread.cpp:621
    #26 0x7f18412fdd22 in NS_ProcessNextEvent(nsIThread*, bool) objdir-ff-asan-sym/xpcom/build/nsThreadUtils.cpp:238
    #27 0x7f183cffea89 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) ipc/glue/MessagePump.cpp:81
    #28 0x7f1841aa3bfb in MessageLoop::RunInternal() ipc/chromium/src/base/message_loop.cc:219
    #29 0x7f1841aa3a4e in MessageLoop::RunHandler() ipc/chromium/src/base/message_loop.cc:212
    #30 0x7f1841aa3939 in MessageLoop::Run() ipc/chromium/src/base/message_loop.cc:186
    #31 0x7f184d36ce1e in nsBaseAppShell::Run() widget/xpwidgets/nsBaseAppShell.cpp:163
    #32 0x7f184bf10ae0 in nsAppStartup::Run() toolkit/components/startup/nsAppStartup.cpp:269
    #33 0x7f183c115aeb in XREMain::XRE_mainRun() toolkit/xre/nsAppRunner.cpp:3853
    #34 0x7f183c11a89d in XREMain::XRE_main(int, char**, nsXREAppData const*) toolkit/xre/nsAppRunner.cpp:3921
    #35 0x7f183c11d4d5 in XRE_main toolkit/xre/nsAppRunner.cpp:4123
    #36 0x42a0d7 in do_main(int, char**, nsIFile*) browser/app/nsBrowserApp.cpp:272
    #37 0x4270e1 in main browser/app/nsBrowserApp.cpp:632
    #38 0x7f18602ad76c in
    #39 0x426854 in _start
0x61900d449420 is located 32 bytes inside of 72-byte region [0x61900d449400,0x61900d449448)
freed by thread T0 here:
    #0 0x41a5f2 in __interceptor_free
    #1 0x7f185e2096de in moz_free memory/mozalloc/mozalloc.cpp:48
    #2 0x7f1844aec3b9 in operator delete(void*) ../../dist/include/mozilla/mozalloc.h:225
    #3 0x7f1844aec3b9 in nsCSSKeyframesRule::~nsCSSKeyframesRule() layout/style/nsCSSRules.cpp:2454
    #4 0x7f1844ab7122 in mozilla::css::GroupRule::DeleteCycleCollectable() layout/style/nsCSSRules.cpp:568
    #5 0x7f1844b14928 in mozilla::css::GroupRule::cycleCollection::DeleteCycleCollectableImpl(void*) ../../dist/include/mozilla/css/GroupRule.h:39
    #6 0x7f18417aad7f in SnowWhiteKiller::~SnowWhiteKiller() xpcom/base/nsCycleCollector.cpp:2173
    #7 0x7f184176837e in SnowWhiteKiller::~SnowWhiteKiller() xpcom/base/nsCycleCollector.cpp:2168
    #8 0x7f184175c525 in nsCycleCollector::FreeSnowWhite(bool) xpcom/base/nsCycleCollector.cpp:2280
    #9 0x7f184175a984 in nsCycleCollectorRunner::Collect(ccType, nsCycleCollectorResults*, nsICycleCollectorListener*) xpcom/base/nsCycleCollector.cpp:1171
    #10 0x7f1841773500 in nsCycleCollector::Collect(ccType, nsCycleCollectorResults*, nsICycleCollectorListener*) xpcom/base/nsCycleCollector.cpp:2883
    #11 0x7f184177a715 in nsCycleCollector_collect(bool, nsCycleCollectorResults*, nsICycleCollectorListener*) xpcom/base/nsCycleCollector.cpp:3312
    #12 0x7f1848a0fae2 in nsJSContext::CycleCollectNow(nsICycleCollectorListener*, int, bool) dom/base/nsJSEnvironment.cpp:2566
    #13 0x7f184873af46 in nsDOMWindowUtils::CycleCollect(nsICycleCollectorListener*, int) dom/base/nsDOMWindowUtils.cpp:1203
    #14 0x7f1841838bbb in NS_InvokeByIndex xpcom/reflect/xptcall/src/md/unix/xptcinvoke_x86_64_unix.cpp:162
    #15 0x7f184b4caa70 in CallMethodHelper::Invoke() js/xpconnect/src/XPCWrappedNative.cpp:2795
    #16 0x7f184b4caa70 in CallMethodHelper::Call() js/xpconnect/src/XPCWrappedNative.cpp:2133
    #17 0x7f184b4caa70 in XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) js/xpconnect/src/XPCWrappedNative.cpp:2099
    #18 0x7f184b5231df in XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*) js/xpconnect/src/XPCWrappedNativeJSOps.cpp:1315
    #19 0x7f1854cc4acc in js::CallJSNative(JSContext*, int (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) js/src/jscntxtinlines.h:225
    #20 0x7f1854cc4acc in js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) js/src/vm/Interpreter.cpp:478
    #21 0x7f1854ca3c90 in Interpret(JSContext*, js::RunState&) js/src/vm/Interpreter.cpp:2503
    #22 0x7f1854c525d6 in js::RunScript(JSContext*, js::RunState&) js/src/vm/Interpreter.cpp:435
    #23 0x7f1854cc5157 in js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) js/src/vm/Interpreter.cpp:497
    #24 0x7f1854cc9013 in js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) js/src/vm/Interpreter.cpp:528
    #25 0x7f1855e25d13 in js::DirectProxyHandler::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) js/src/jsproxy.cpp:451
    #26 0x7f185617e722 in js::CrossCompartmentWrapper::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) js/src/jswrapper.cpp:446
    #27 0x7f1855e9400e in js::Proxy::call(JSContext*, JS::Handle<JSObject*>, JS::CallArgs const&) js/src/jsproxy.cpp:2633
    #28 0x7f1855eb16ca in proxy_Call(JSContext*, unsigned int, JS::Value*) js/src/jsproxy.cpp:3205
    #29 0x7f1854cc43dc in js::CallJSNative(JSContext*, int (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) js/src/jscntxtinlines.h:225
    #30 0x7f1854cc43dc in js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) js/src/vm/Interpreter.cpp:471
    #31 0x7f1854ca3c90 in Interpret(JSContext*, js::RunState&) js/src/vm/Interpreter.cpp:2503
    #32 0x7f1854c525d6 in js::RunScript(JSContext*, js::RunState&) js/src/vm/Interpreter.cpp:435
    #33 0x7f1854cc5157 in js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) js/src/vm/Interpreter.cpp:497
    #34 0x7f1854cc9013 in js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) js/src/vm/Interpreter.cpp:528
previously allocated by thread T0 here:
    #0 0x41a6d2 in malloc
    #1 0x7f185e209825 in moz_xmalloc memory/mozalloc/mozalloc.cpp:54
    #2 0x7f18449ee3f3 in operator new(unsigned long) ../../dist/include/mozilla/mozalloc.h:201
    #3 0x7f18449ee3f3 in (anonymous namespace)::CSSParserImpl::ParseKeyframesRule(void (*)(mozilla::css::Rule*, void*), void*) layout/style/nsCSSParser.cpp:2624
    #4 0x7f18449e59ef in (anonymous namespace)::CSSParserImpl::ParseAtRule(void (*)(mozilla::css::Rule*, void*), void*, bool) layout/style/nsCSSParser.cpp:1673
    #5 0x7f184492dd6e in (anonymous namespace)::CSSParserImpl::ParseSheet(nsAString_internal const&, nsIURI*, nsIURI*, nsIPrincipal*, unsigned int, bool) layout/style/nsCSSParser.cpp:988
    #6 0x7f184492cdcf in nsCSSParser::ParseSheet(nsAString_internal const&, nsIURI*, nsIURI*, nsIPrincipal*, unsigned int, bool) layout/style/nsCSSParser.cpp:10870
    #7 0x7f184485ef6c in mozilla::css::Loader::ParseSheet(nsAString_internal const&, mozilla::css::SheetLoadData*, bool&) layout/style/Loader.cpp:1628
    #8 0x7f184485cc97 in mozilla::css::SheetLoadData::OnStreamComplete(nsIUnicharStreamLoader*, nsISupports*, tag_nsresult, nsAString_internal const&) layout/style/Loader.cpp:938
    #9 0x7f184485f8fb in non-virtual thunk to mozilla::css::SheetLoadData::OnStreamComplete(nsIUnicharStreamLoader*, nsISupports*, tag_nsresult, nsAString_internal const&) layout/style/Loader.cpp:941
    #10 0x7f184203714e in nsUnicharStreamLoader::OnStopRequest(nsIRequest*, nsISupports*, tag_nsresult) netwerk/base/src/nsUnicharStreamLoader.cpp:99
    #11 0x7f1841d57c17 in nsBaseChannel::OnStopRequest(nsIRequest*, nsISupports*, tag_nsresult) netwerk/base/src/nsBaseChannel.cpp:737
    #12 0x7f1841d5810a in non-virtual thunk to nsBaseChannel::OnStopRequest(nsIRequest*, nsISupports*, tag_nsresult) netwerk/base/src/nsBaseChannel.cpp:752
    #13 0x7f1841e1e61c in nsInputStreamPump::OnStateStop() netwerk/base/src/nsInputStreamPump.cpp:623
    #14 0x7f1841e1ae7c in nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream*) netwerk/base/src/nsInputStreamPump.cpp:395
    #15 0x7f1841e1eaa2 in non-virtual thunk to nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream*) netwerk/base/src/nsInputStreamPump.cpp:439
    #16 0x7f18415bfcdc in nsInputStreamReadyEvent::Run() xpcom/io/nsStreamUtils.cpp:82
    #17 0x7f18416e2feb in nsThread::ProcessNextEvent(bool, bool*) xpcom/threads/nsThread.cpp:621
    #18 0x7f18412fdd22 in NS_ProcessNextEvent(nsIThread*, bool) objdir-ff-asan-sym/xpcom/build/nsThreadUtils.cpp:238
    #19 0x7f183cffea89 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) ipc/glue/MessagePump.cpp:81
    #20 0x7f1841aa3bfb in MessageLoop::RunInternal() ipc/chromium/src/base/message_loop.cc:219
    #21 0x7f1841aa3a4e in MessageLoop::RunHandler() ipc/chromium/src/base/message_loop.cc:212
    #22 0x7f1841aa3939 in MessageLoop::Run() ipc/chromium/src/base/message_loop.cc:186
    #23 0x7f184d36ce1e in nsBaseAppShell::Run() widget/xpwidgets/nsBaseAppShell.cpp:163
    #24 0x7f184bf10ae0 in nsAppStartup::Run() toolkit/components/startup/nsAppStartup.cpp:269
    #25 0x7f183c115aeb in XREMain::XRE_mainRun() toolkit/xre/nsAppRunner.cpp:3853
    #26 0x7f183c11a89d in XREMain::XRE_main(int, char**, nsXREAppData const*) toolkit/xre/nsAppRunner.cpp:3921
    #27 0x7f183c11d4d5 in XRE_main toolkit/xre/nsAppRunner.cpp:4123
    #28 0x42a0d7 in do_main(int, char**, nsIFile*) browser/app/nsBrowserApp.cpp:272
    #29 0x4270e1 in main browser/app/nsBrowserApp.cpp:632
    #30 0x7f18602ad76c in
Shadow bytes around the buggy address:
  0x0c3281a81230: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c3281a81240: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c3281a81250: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c3281a81260: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c3281a81270: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
=>0x0c3281a81280: fd fd fd fd[fd]fd fd fd fd fa fa fa fa fa fa fa
  0x0c3281a81290: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c3281a812a0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c3281a812b0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c3281a812c0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c3281a812d0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07
  Heap left redzone:     fa
  Heap righ redzone:     fb
  Freed Heap region:     fd
  Stack left redzone:    f1
  Stack mid redzone:     f2
  Stack right redzone:   f3
  Stack partial redzone: f4
  Stack after return:    f5
  Stack use after scope: f8
  Global redzone:        f9
  Global init order:     f6
  Poisoned by user:      f7
  ASan internal:         fe
==1210==ABORTING
Comment 1 Daniel Veditz [:dveditz] 2013-07-17 10:48:25 PDT
Anyone mucking with animations lately?
Comment 2 David Bolter [:davidb] 2013-07-18 13:52:39 PDT
dbaron, any suggestion who could take this one?
Comment 3 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2013-07-22 18:05:48 PDT
I haven't actually debugged, but based on looking at the testcase and the stacks in comment 0, this smells like a cycle collection bug.  I don't see any reason there should be a keyframes rule being cycle collected here; the testcase hasn't created any garbage.  (And, in particular, I don't see any reason we should have cloned the sheet, which might lead to that happening legitimately.  And even if we had, EnsureUniqueInner would have called ClearRuleCascades which would have cleaned the pointer we're getting the rule from.)

Was this a recent regression?
Comment 4 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2013-07-23 15:25:06 PDT
Oh, I guess the reason there's cloning happening is stylesheet preloading.  Though that's a bit unfortunate.
Comment 5 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2013-07-23 16:35:58 PDT
Created attachment 780076 [details] [diff] [review]
patch 1:  Make keyframes rule table hold strong references.

This is one of two patches, either of which fixes the bug on its own.  I'm inclined to do both.
Comment 6 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2013-07-23 16:36:30 PDT
Created attachment 780077 [details] [diff] [review]
patch 2:  Call KeyframesListIsDirty when doing ClearRuleCascades.
Comment 7 Boris Zbarsky [:bz] 2013-07-23 18:51:09 PDT
Comment on attachment 780076 [details] [diff] [review]
patch 1:  Make keyframes rule table hold strong references.

Hmm.  What ensures we don't leak due to cycles through this hashtable?
Comment 8 Boris Zbarsky [:bz] 2013-07-23 18:52:13 PDT
Comment on attachment 780077 [details] [diff] [review]
patch 2:  Call KeyframesListIsDirty when doing ClearRuleCascades.

r=me
Comment 9 Mats Palmgren (:mats) 2013-07-24 10:23:42 PDT
Fwiw, this is the regression changeset, in case someone wants to investigate
the reason why:
changeset:   137909:d42a2a82f3d2
user:        Olli Pettay <Olli.Pettay@helsinki.fi>
date:        Tue Jul 09 13:30:58 2013 -0400
summary:     bug 789919, (snow-white) make addref/release of CCable objects faster by removing indirect refcnt increase/decrease, r=mccr8, test changes r=ehsan
Comment 10 Olli Pettay [:smaug] 2013-07-24 10:29:27 PDT
Yes, Snow-White has revealed various bugs in other code.
Comment 11 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2013-07-24 14:00:00 PDT
What's the issue with cycles? Something with bindings?
Comment 12 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2013-07-24 14:10:17 PDT
Comment on attachment 780077 [details] [diff] [review]
patch 2:  Call KeyframesListIsDirty when doing ClearRuleCascades.

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

  Probably not trivial, but also not really hard.  (At least to the point of
  use after free).

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

  I don't think so.

Which older supported branches are affected by this flaw?

  Possibly back to Firefox 5, although this testcase doesn't show the bug
  until Firefox 25 (see comment 9).

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

  bug 789919 (Firefox 25) and/or bug 435442 (Firefox 5)

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

  I don't have them, but I think they should be trivial, at least for
  supported branches.

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

  I think it's relatively low risk.  The risk of regression would either be
  null-dereference crashes or performance regressions.
Comment 13 Al Billings [:abillings] 2013-07-24 16:50:31 PDT
Need release management input here since it is late in cycle.
Comment 14 Boris Zbarsky [:bz] 2013-07-25 22:28:39 PDT
David, see comment 7?
Comment 15 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2013-07-25 22:52:54 PDT
see comment 11?
Comment 16 Boris Zbarsky [:bz] 2013-07-25 23:14:18 PDT
Oh, sorry.  

So in the new setup, the nsAnimationManager holds strong references to the keyframes rules, right?  My question was how we know this can't cause leaks, given that nsAnimationManager is a refcounted class that's not cycle-collected.

I guess the point is that the only things holding references to the nsAnimationManager will be killed off when PresShell::Destroy happens?  If so, it's worth documenting that...
Comment 17 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2013-07-26 00:01:16 PDT
So I'd traditionally have thought of nsCSSKeyframeRule as something leaf-like that wouldn't entrain things outside its data structure, and therefore I wasn't particularly worried about cycles.  Maybe there's something with bindings that changes that?

Either way, I suppose if we explicitly clear this hash table on PresShell::Destroy, we shouldn't have anything to worry about, since that would mean it should be cleared at any time a rule becomes no longer part of an active document.  (And if the theory is wrong, we'd leak rather than crash, which is probably preferable.)  I'll try to do that tomorrow.
Comment 18 Boris Zbarsky [:bz] 2013-07-26 00:06:16 PDT
Oh, I see.

The keyframe rule holds a strong ref to its mDOMDeclaration, which can hold a strong reference to its JS object if someone sets expandos on it.  And once we switch keyframe rule to WebIDL bindings, it will be able to hold a strong ref to its own JSObject once expandos are set.

Basically, no DOM object is a leaf.  :(
Comment 19 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2013-07-26 03:18:25 PDT
(In reply to Boris Zbarsky (:bz) from comment #18)
> Basically, no DOM object is a leaf.  :(

Yeah, I'd sort of heard that was the case with new bindings.
Comment 20 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2013-07-26 03:43:58 PDT
Also, it turns out that in patch 2, I'm missing a null-check of mDocument in nsCSSStyleSheet::ClearRuleCascades.  I'm a little worried about just null-checking it, though; it seems likely to fail to clear in cases where I need to.
Comment 21 Lukas Blakk [:lsblakk] use ?needinfo 2013-07-26 15:07:57 PDT
(In reply to Al Billings [:abillings] from comment #13)
> Need release management input here since it is late in cycle.

If bug 789919 (in FF25) is what shows this in the testcase and it's been around as far back as Firefox 5 then I don't see the need to cram this in so late in the FF23 cycle.
Comment 22 Lukas Blakk [:lsblakk] use ?needinfo 2013-07-26 15:08:42 PDT
adjusting tracking flags to reflect that we should just take this on FF25 and ESR 24.1
Comment 23 Al Billings [:abillings] 2013-07-26 15:41:07 PDT
Comment on attachment 780077 [details] [diff] [review]
patch 2:  Call KeyframesListIsDirty when doing ClearRuleCascades.

sec-approval+ for trunk.
Comment 24 Boris Zbarsky [:bz] 2013-07-29 17:29:23 PDT
Comment on attachment 780076 [details] [diff] [review]
patch 1:  Make keyframes rule table hold strong references.

Pending the possible leak issues being sorted out.
Comment 25 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2013-08-12 14:32:06 PDT
(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #20)
> Also, it turns out that in patch 2, I'm missing a null-check of mDocument in
> nsCSSStyleSheet::ClearRuleCascades.  I'm a little worried about just
> null-checking it, though; it seems likely to fail to clear in cases where I
> need to.

The only wa that I'm cmfortable will be a reliable fix is moving the hashtable from the pres context to the RuleCascadeData.  It's a tad slower for the normal case(although faster in some edge cases, I think).

I might even file that as a separate "perf" bug as cover rather than landing it with a bug number pointing to a security bug.
Comment 26 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2013-08-12 16:02:12 PDT
Created attachment 789237 [details] [diff] [review]
move hash tble to RuleCascadeData (so we have one per cascade level and can manage it easily)
Comment 27 Cameron McCormack (:heycam) 2013-08-13 02:43:49 PDT
Comment on attachment 789237 [details] [diff] [review]
move hash tble to RuleCascadeData (so we have one per cascade level and can manage it easily)

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

This looks good; two nits below.

Before I r+, can you explain the original issue in a bit more detail?  I am not clear on how/when/why style sheet cloning is done.

I gather that since ClearRuleCascades is called from nsCSSStyleSheet::DidDirty, that any modification to the sheet now will cause the keyframe rule table to be up-to-date next time we use it.

::: layout/style/nsAnimationManager.cpp
@@ +764,5 @@
>      }
>  
>      aDest.mIterationDuration = TimeDuration::FromMilliseconds(aSrc.GetDuration());
>  
> +    nsCSSKeyframesRule *rule =

"*" next to type.

::: layout/style/nsCSSRuleProcessor.cpp
@@ +2784,2 @@
>                                nsPresContext *aPresContext,
> +                              const nsString& aName)

"nsPresContext* aPresContext"

The arguments here should fit within the 80 characters now, too.  (Actually they did before.)
Comment 28 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2013-08-13 14:08:34 PDT
Turns out I was missing a null-check, so I'll be uploading a new patch with a null-check added shortly.

(In reply to Cameron McCormack (:heycam) from comment #27)
> Before I r+, can you explain the original issue in a bit more detail?  I am
> not clear on how/when/why style sheet cloning is done.

So it's not clear to me how cloning is related.  But I can explain it anyway.

Basicaly, CSS style sheets (nsCSSStyleSheet) implement copy-on-write cloning, so that we can clone a sheet and have two logical sheets pointing at the same inner until one or the other is mutated or its OM is accessed in a way that might lead to a harder-to-detect mutation (we call EnsureUniqueInner at various points).  The cases we use this are somewhat limited, but I'd like to increase them (bug 904836, etc.):  currently I think it's just:

 * having style sheets used by XUL documents cloned from instances on the prototype document (so that when we open new Firefox windows, for example, we share the inner between windows)

 * sharing style sheets when the same style sheet URL is linked multiple times from the same page, e.g., multiple link elements, multiple @imports, or some combination

> I gather that since ClearRuleCascades is called from
> nsCSSStyleSheet::DidDirty, that any modification to the sheet now will cause
> the keyframe rule table to be up-to-date next time we use it.

yes, that's the idea
Comment 29 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2013-08-13 14:39:00 PDT
Created attachment 789827 [details] [diff] [review]
move hash table to RuleCascadeData (so we have one per cascade level and can manage it easily)
Comment 30 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2013-08-13 14:54:34 PDT
Oh, and I guess the cloning issue from comment 4 was that somebody added a third use of cloning (which I both dislike and keep forgetting about) which is style sheet preloading.  In particular, when we kick off "optimistic" style sheet loads from lookahead in the HTML parser (because sometimes, due to scripts, we have markup but we can't yet construct the elements for that markup that would kick off the load in the normal way until the scripts load and run), we use the same multiple-stylesheet-loading management to deal with loading the same style sheet URL twice for the preload and the normal load.  This means that if we touch the CSSOM prior to dropping the preload copy (which happens here) we end up paying the extra cost of duplicating the entire sheet.

So what happens in the testcase is, if memory serves:

document.styleSheets[0].cssRules[1];

At this point the preload clone is still alive, so this triggers sheet cloning.  The preload sheet keeps the original copy and the document's sheet gets the new copy.  This calls ClearRuleCascades, but doesn't do any style sheet rebuilding (and thus doesn't call KeyframesListIsDirty) since the assumption is that it isn't making any logical changes.  (I tried to fix this directly in patch 2, but then see comment 20.)

fuzzPriv.CC();

This leads to the preload clone being destroyed, which means the pres context's list of keyframes rules now has dangling pointers in it.

test1.style.display = "-moz-inline-box";
getComputedStyle(test1, "").color;

This triggers and then flushes a style recomputation (before anything else happens that might call KeyframesListIsDirty).
Comment 31 Cameron McCormack (:heycam) 2013-08-13 15:17:26 PDT
Comment on attachment 789827 [details] [diff] [review]
move hash table to RuleCascadeData (so we have one per cascade level and can manage it easily)

Thanks, that all makes sense.
Comment 32 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2013-08-13 16:01:00 PDT
Comment on attachment 789827 [details] [diff] [review]
move hash table to RuleCascadeData (so we have one per cascade level and can manage it easily)

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

  I think it's probably doable (not trivial but also not really hard).  It's
  actually probably harder to recognize that the patch is for a security bug,
  except that the bug number will point to a hidden bug.  I could file a
  cover bug with the patch if that's desirable, though coming up with a good
  cover story isn't completely trivial.

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

  I don't think so.

Which older supported branches are affected by this flaw?

  Possibly back to Firefox 5, although this testcase doesn't show the bug
  until Firefox 25 (see comment 9).

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

  bug 789919 (Firefox 25) and/or bug 435442 (Firefox 5)

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

  I don't have them, but I think they should be trivial, at least for
  supported branches.

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

  I think it's relatively low risk.  The risk of regression would mostly be
  null-dereference crashes or performance regressions.
Comment 33 Al Billings [:abillings] 2013-08-13 16:14:30 PDT
I'm inclined to take this now for trunk. In a perfect world, you'd get it checked in with someone else's stuff (I don't know if that's really possible but an idea). Otherwise, just do it.

I set tracking to ? for Aurora and Beta so we can consider taking it there. Aurora is a no brainer though.
Comment 34 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2013-08-14 18:38:14 PDT
https://tbpl.mozilla.org/?tree=Try&rev=6cab1031c13a
Comment 35 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2013-08-14 21:59:43 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/b36a029cdb83
Comment 36 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2013-08-14 22:02:28 PDT
Comment on attachment 789827 [details] [diff] [review]
move hash table to RuleCascadeData (so we have one per cascade level and can manage it easily)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 789919 (Firefox 25) and/or bug 435442 (Firefox 5)
User impact if declined: possibly exploitable crash
Testing completed (on m-c, etc.): will be on m-c shortly; modifies code that's reasonably well-covered by mochitests
Risk to taking this patch (and alternatives if risky): low risk; any risk of regressions would likely be either null-dereference crashes or performance regressions (either very tiny or in unusual cases)
String or IDL/UUID changes made by this patch: none
Comment 37 Ed Morley [:emorley] 2013-08-15 05:38:24 PDT
https://hg.mozilla.org/mozilla-central/rev/b36a029cdb83
Comment 39 Boris Zbarsky [:bz] 2013-08-20 21:24:32 PDT
Hmm.  We should perhaps file a followup on making preloading do something fancier than just hitting the CSSLoader twice and relying on the existing coalescing.  The fact that the preload lives through cc and hence CSSOM access causes actual cloning is not that great....
Comment 40 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2013-08-20 21:53:40 PDT
Yeah, I meant to do that after comment 4 and failed to.  Filed bug 907564 now.
Comment 42 Al Billings [:abillings] 2013-08-27 15:21:54 PDT
Do we want to take this on ESR?
Comment 43 Lukas Blakk [:lsblakk] use ?needinfo 2013-08-28 09:24:26 PDT
Comment on attachment 789827 [details] [diff] [review]
move hash table to RuleCascadeData (so we have one per cascade level and can manage it easily)

It's sec-critical and landed in 24, so yes we should take this in ESR 17 unless there are engineering reasons for it not being able to land/stick there - however the nomination for uplift approval suggests this not to be the case so let's get it uplifted.
Comment 44 Ryan VanderMeulen [:RyanVM] 2013-08-28 10:03:23 PDT
https://hg.mozilla.org/releases/mozilla-esr17/rev/c56e2d7733ce
Comment 45 Matt Wobensmith [:mwobensmith][:matt:] 2013-09-11 15:59:31 PDT
I was never able to get the test case to crash, across numerous builds. However, we're not going to let that hold up bug verification.

Confirmed no crash in FF17esr, 24, 25, 26, using ASan builds built 2013-09-11.
Comment 46 Mats Palmgren (:mats) 2013-09-11 17:33:58 PDT
Fwiw, I could reproduce the Firefox crash before the fix but now I can't (m-c 5b9ad615d2e1 Linux64
debug asan).  I do get a new crash though, in a JS jit sub-process.
I think that's related to bug 890243 though, and unrelated to the crash in this bug.
Comment 47 bhavana bajaj [:bajaj] 2013-10-15 14:59:26 PDT
NI on :dbaron to help with a backport patch that applies on mozilla-b2g18.
Comment 48 Ryan VanderMeulen [:RyanVM] 2013-10-16 06:08:46 PDT
https://hg.mozilla.org/releases/mozilla-b2g18/rev/81a860f9635f

a=bajaj over email.

Note You need to log in before you can comment on or make changes to this bug.