930270 - NULL deref in libxul.so!TreeMatchContext::InitAncestors

Status: VERIFIED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Tyson Smith [:tsmith]]

Attachment: 098F2905-197126.html

Found by the BlackBerry Security Automated Analysis Team's fuzzing framework ALF.

==28797==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000030 (pc 0x7fa5f97a3173 sp 0x7fffed8a2740 bp 0x7fffed8a2990 T0)
AddressSanitizer can not provide additional info.
    #0 0x7fa5f97a3172 (libxul.so!TreeMatchContext::InitAncestors(mozilla::dom::Element*)+0x2a2)
        Line 1322 of "../../dist/include/nsINode.h"
    #1 0x7fa5f98bb9fc (libxul.so!nsStyleSet::HasStateDependentStyle(nsPresContext*, mozilla::dom::Element*, nsEventStates)+0x16c)
        Line 1164 of "/builds/slave/m-in-l64-asan-0000000000000000/build/layout/style/nsStyleSet.cpp"
    #2 0x7fa5f929c134 (libxul.so!mozilla::RestyleManager::ContentStateChanged(nsIContent*, nsEventStates)+0x464)
        Line 890 of "/builds/slave/m-in-l64-asan-0000000000000000/build/layout/base/RestyleManager.cpp"
    #3 0x7fa5f94371ec (libxul.so!non-virtual thunk to PresShell::ContentStateChanged(nsIDocument*, nsIContent*, nsEventStates)+0x14c)
        Line 3953 of "/builds/slave/m-in-l64-asan-0000000000000000/build/layout/base/nsPresShell.cpp"
    #4 0x7fa5f9c45512 (libxul.so!nsDocument::ContentStateChanged(nsIContent*, nsEventStates)+0x2f2)
        Line 4732 of "/builds/slave/m-in-l64-asan-0000000000000000/build/content/base/src/nsDocument.cpp"
    #5 0x7fa5f9ad5bcc (libxul.so!mozilla::dom::Element::UpdateState(bool)+0x1fc)
        Line 193 of "/builds/slave/m-in-l64-asan-0000000000000000/build/content/base/src/Element.cpp"
    #6 0x7fa5f9fe09a0 (libxul.so!mozilla::dom::HTMLFormElement::UpdateValidity(bool)+0x1a0)
        Line 1895 of "/builds/slave/m-in-l64-asan-0000000000000000/build/content/html/content/src/HTMLFormElement.cpp"
    #7 0x7fa5fa135e11 (libxul.so!nsIConstraintValidation::SetValidityState(nsIConstraintValidation::ValidityStateType, bool)+0x1a1)
        Line 146 of "/builds/slave/m-in-l64-asan-0000000000000000/build/content/html/content/src/nsIConstraintValidation.cpp"
    #8 0x7fa5fa033745 (libxul.so!mozilla::dom::HTMLInputElement::UnbindFromTree(bool, bool)+0xe5)
        Line 6027 of "/builds/slave/m-in-l64-asan-0000000000000000/build/content/html/content/src/HTMLInputElement.cpp"
    #9 0x7fa5f9adebbc (libxul.so!mozilla::dom::Element::UnbindFromTree(bool, bool)+0xc8c)
        Line 1248 of "/builds/slave/m-in-l64-asan-0000000000000000/build/content/base/src/Element.cpp"
    #10 0x7fa5fa1181f6 (libxul.so!nsGenericHTMLElement::UnbindFromTree(bool, bool)+0x426)
        Line 644 of "/builds/slave/m-in-l64-asan-0000000000000000/build/content/html/content/src/nsGenericHTMLElement.cpp"
    #11 0x7fa5f9fdb17b (libxul.so!mozilla::dom::HTMLFormElement::UnbindFromTree(bool, bool)+0x44b)
        Line 449 of "/builds/slave/m-in-l64-asan-0000000000000000/build/content/html/content/src/HTMLFormElement.cpp"
    #12 0x7fa5f9cfe09b (libxul.so!nsINode::doRemoveChildAt(unsigned int, bool, nsIContent*, nsAttrAndChildArray&)+0x21b)
        Line 1515 of "/builds/slave/m-in-l64-asan-0000000000000000/build/content/base/src/nsINode.cpp"
    #13 0x7fa5f9b0dd78 (libxul.so!mozilla::dom::FragmentOrElement::RemoveChildAt(unsigned int, bool)+0x68)
        Line 964 of "/builds/slave/m-in-l64-asan-0000000000000000/build/content/base/src/FragmentOrElement.cpp"
    #14 0x7fa5fab52df9 (libxul.so!nsHtml5TreeOperation::Perform(nsHtml5TreeOpExecutor*, nsIContent**)+0x819)
        Line 249 of "/builds/slave/m-in-l64-asan-0000000000000000/build/parser/html/nsHtml5TreeOperation.cpp"
    #15 0x7fa5fab48cf7 (libxul.so!nsHtml5TreeOpExecutor::RunFlushLoop()+0x957)
        Line 524 of "/builds/slave/m-in-l64-asan-0000000000000000/build/parser/html/nsHtml5TreeOpExecutor.cpp"
    #16 0x7fa5faac36d8 (libxul.so!nsHtml5ExecutorFlusher::Run()+0x38)
        Line 131 of "/builds/slave/m-in-l64-asan-0000000000000000/build/parser/html/nsHtml5StreamParser.cpp"
    #17 0x7fa5fcffede9 (libxul.so!nsThread::ProcessNextEvent(bool, bool*)+0xaa9)
        Line 622 of "/builds/slave/m-in-l64-asan-0000000000000000/build/xpcom/threads/nsThread.cpp"
    #18 0x7fa5fcf26f01 (libxul.so!NS_ProcessNextEvent(nsIThread*, bool)+0xb1)
        Line 238 of "/builds/slave/m-in-l64-asan-0000000000000000/build/xpcom/glue/nsThreadUtils.cpp"
    #19 0x7fa5fbbb2481 (libxul.so!mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*)+0x311)
        Line 85 of "/builds/slave/m-in-l64-asan-0000000000000000/build/ipc/glue/MessagePump.cpp"
    #20 0x7fa5fd11cd43 (libxul.so!MessageLoop::Run()+0x1c3)
        Line 220 of "/builds/slave/m-in-l64-asan-0000000000000000/build/ipc/chromium/src/base/message_loop.cc"
    #21 0x7fa5fb985d5c (libxul.so!nsBaseAppShell::Run()+0x5c)
        Line 161 of "/builds/slave/m-in-l64-asan-0000000000000000/build/widget/xpwidgets/nsBaseAppShell.cpp"
    #22 0x7fa5fb371fee (libxul.so!nsAppStartup::Run()+0xbe)
        Line 268 of "/builds/slave/m-in-l64-asan-0000000000000000/build/toolkit/components/startup/nsAppStartup.cpp"
    #23 0x7fa5f87fd3f5 (libxul.so!XREMain::XRE_mainRun()+0x1e05)
        Line 3886 of "/builds/slave/m-in-l64-asan-0000000000000000/build/toolkit/xre/nsAppRunner.cpp"
    #24 0x7fa5f87fe32a (libxul.so!XREMain::XRE_main(int, char**, nsXREAppData const*)+0x4fa)
        Line 3954 of "/builds/slave/m-in-l64-asan-0000000000000000/build/toolkit/xre/nsAppRunner.cpp"
    #25 0x7fa5f87ff25b (libxul.so!XRE_main+0x3ab)
        Line 4156 of "/builds/slave/m-in-l64-asan-0000000000000000/build/toolkit/xre/nsAppRunner.cpp"
    #26 0x459b8d (firefox!main+0x94d)
        Line 275 of "/builds/slave/m-in-l64-asan-0000000000000000/build/browser/app/nsBrowserApp.cpp"
    #27 0x7fa6080b076c (libc.so.6!__libc_start_main+0xec)
        Line 226 of "libc-start.c"
    #28 0x45910c (firefox!_start+0x28)
==28797==ABORTING

Comment 1

[Alice0775 White]

Regression window(m-c)
Good:
http://hg.mozilla.org/mozilla-central/rev/672cd63528d3
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:27.0) Gecko/20100101 Firefox/27.0 ID:20131010190115
Bad:
http://hg.mozilla.org/mozilla-central/rev/6fef99317f21
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:27.0) Gecko/20100101 Firefox/27.0 ID:20131011021258
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=672cd63528d3&tochange=6fef99317f21

Regression window(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/62c53295e477
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:27.0) Gecko/20100101 Firefox/27.0 ID:20131010151944
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/c7f0cd7515ad
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:27.0) Gecko/20100101 Firefox/27.0 ID:20131010163000
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=62c53295e477&tochange=c7f0cd7515ad

Suspected:
Bug 899808

Comment 2

[Cameron McCormack (:heycam)]

Can't reproduce locally with a non-ASAN build on Mac; will try on Monday when I'm back at my Linux machine with ASAN stuff set up.

Comment 3

[Jesse Ruderman]

Doesn't require ASan.  Here's what I get in a debug build:

Assertion failure: aElement->IsInDoc() (aElement must be in the document for the assumption that GetParentNode() is non-null on all element ancestors of aElement to be true), at layout/style/nsCSSRuleProcessor.cpp:3420

Comment 4

[Cameron McCormack (:heycam)]

Got it.  So I think the issue is that we recently made nsStyleSet::HasStateDependentStyle call InitAncestorsIfInStyleScope so that the style scopes would be set up correctly, and include scoped :hover etc. rules in determining whether there are any state dependent styles applying to the element.  The ancestor filter initialisation assumes elements are in the document, however.  I think we should change this to only initialise the style scopes and not the ancestor filter.

Comment 5

[Cameron McCormack (:heycam)]

Attachment: patch

Comment 6

[Cameron McCormack (:heycam)]

(In reply to Cameron McCormack (:heycam) from comment #4)
> I think we should change this to only initialise the
> style scopes and not the ancestor filter.

And in that version of initialisation, don't assume that the element is in the document.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 823730 [details] [diff] [review]
patch

r=dbaron, I suppose, although it seems less than ideal that we're getting this far when a node isn't in the document -- maybe we should also patch things to avoid doing that?

Comment 8

[Cameron McCormack (:heycam)]

I'm not entirely sure why the node isn't in the document, btw.  But in general, since we could call getComputedStyle() with a node not in the document, we need to make this work.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Cameron McCormack (:heycam) from comment #8)
> I'm not entirely sure why the node isn't in the document, btw.

From the stack, I think it's because the markup triggered a case where there are parsing-time fixup rules that move things around, and the moving around actually triggers a style change (which, again, seems bad).

> But in
> general, since we could call getComputedStyle() with a node not in the
> document, we need to make this work.

Maybe it would be good to include a testcase of that case, especially if it asserts without the patch?

Comment 10

[Cameron McCormack (:heycam)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3ba4001d257b

Comment 11

[Cameron McCormack (:heycam)]

Comment on attachment 823730 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 899808
User impact if declined: Content can cause the browser to crash
Testing completed (on m-c, etc.): Local testing, crashtests added; only just landed on inbound
Risk to taking this patch (and alternatives if risky): low imo
String or IDL/UUID changes made by this patch: n/a

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/3ba4001d257b

Comment 13

[Cameron McCormack (:heycam)]

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

Comment 14

[Matt Wobensmith [:mwobensmith][:matt:]]

Confirmed crash in FF27, 2013-10-25.
Verified fixed in FF27, 2013-11-22.
Verified fixed in FF28, 2013-11-20.