Closed Bug 930270 Opened 6 years ago Closed 6 years ago

NULL deref in libxul.so!TreeMatchContext::InitAncestors

Categories

(Core :: CSS Parsing and Computation, defect)

27 Branch
x86_64
All
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla28
Tracking Status
firefox26 --- unaffected
firefox27 + verified
firefox28 --- verified

People

(Reporter: tsmith, Assigned: heycam)

References

Details

(4 keywords)

Attachments

(2 files)

Attached file 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
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
Blocks: 899808
Status: UNCONFIRMED → NEW
Component: General → CSS Parsing and Computation
Ever confirmed: true
Keywords: regression
Product: Firefox → Core
OS: Linux → All
Keywords: csec-dos
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.
Assignee: nobody → cam
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
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.
Attached patch patchSplinter Review
Attachment #823730 - Flags: review?(dbaron)
(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 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?
Attachment #823730 - Flags: review?(dbaron) → review+
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.
(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 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
Attachment #823730 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/3ba4001d257b
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Attachment #823730 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Keywords: verifyme
Confirmed crash in FF27, 2013-10-25.
Verified fixed in FF27, 2013-11-22.
Verified fixed in FF28, 2013-11-20.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.