Bug 44170 - HTML5 TreeBuilder ASSERTs on <a><svg><tr><input></a>
Summary: HTML5 TreeBuilder ASSERTs on <a><svg><tr><input></a>
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P1 Major
Assignee: Tony Gentilcore
URL:
Keywords: InRadar
Depends on: 43055 44129
Blocks: 44390
  Show dependency treegraph
 
Reported: 2010-08-18 06:21 PDT by Berend-Jan Wever
Modified: 2010-08-21 20:07 PDT (History)
6 users (show)

See Also:


Attachments
Repro (88 bytes, text/html)
2010-08-18 06:21 PDT, Berend-Jan Wever
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Berend-Jan Wever 2010-08-18 06:21:16 PDT
Created attachment 64697 [details]
Repro

The following repro causes memory corruption in Chromium:
<body onload="
  document.writeln('<a ><svg ><tr/s><input></a>');
  document.open();
">
Crashes vary wildly, but most commonly I see a crash in 
WebCore::removeAllChildrenInContainer<WebCore::Node,WebCore::ContainerNode>
Comment 1 Abhishek Arya 2010-08-18 09:04:21 PDT
Adding security as product.
Comment 2 Eric Seidel (no email) 2010-08-18 09:08:44 PDT
I feel like this is identical to another one you filed this morning.  Like bug 44172.

These are probably all related to/duplicates of bug 43055.
Comment 3 David Kilzer (:ddkilzer) 2010-08-18 09:09:33 PDT
<rdar://problem/8324425>
Comment 4 Eric Seidel (no email) 2010-08-18 09:11:19 PDT
I'll be landing a fix for bug 43055 this afternoon, then I can look at some of these others.  I suspect most of them will be dupes. :)
Comment 5 Abhishek Arya 2010-08-18 09:20:45 PDT
Eric, this bug reproduces with just

<a><svg><tr><input></a> and pressing page reload. It hits this assert without reload. I am taking a closer look now, dont think it is related to 43055.

ASSERT(!newChild->parent()); // Use appendChild if you need to handle reparenting.

 WebCore::ContainerNode::addChildCommon(WebCore::Node * newChild=0x08153780)  Line 539 + 0x2b bytes
 WebCore::ContainerNode::parserAddChild(WTF::PassRefPtr<WebCore::Node> newChild={m_document=0x08219000 m_previous=0x00000000 m_next=0x00000000 ...})  Line 559
 WebCore::HTMLConstructionSite::attachAtSite(const WebCore::HTMLConstructionSite::AttachmentSite & site={...}, WTF::PassRefPtr<WebCore::Node> prpChild=NULL)  Line 133
 WebCore::HTMLConstructionSite::fosterParent(WebCore::Node * node=0x08153780)  Line 457
 WebCore::HTMLTreeBuilder::callTheAdoptionAgency(WebCore::AtomicHTMLToken & token={...})  Line 1835
 WebCore::HTMLTreeBuilder::processEndTagForInBody(WebCore::AtomicHTMLToken & token={...})  Line 2161
 WebCore::HTMLTreeBuilder::processEndTag(WebCore::AtomicHTMLToken & token={...})  Line 2295
 WebCore::HTMLTreeBuilder::processToken(WebCore::AtomicHTMLToken & token={...})  Line 629
 WebCore::HTMLTreeBuilder::processUsingSecondaryInsertionModeAndAdjustInsertionMode(WebCore::AtomicHTMLToken & token={...})  Line 2539
 WebCore::HTMLTreeBuilder::processEndTag(WebCore::AtomicHTMLToken & token={...})  Line 2499
 WebCore::HTMLTreeBuilder::processToken(WebCore::AtomicHTMLToken & token={...})  Line 629
 WebCore::HTMLTreeBuilder::constructTreeFromToken(WebCore::HTMLToken & rawToken={...})  Line 612
 WebCore::HTMLDocumentParser::pumpTokenizer(WebCore::HTMLDocumentParser::SynchronousMode mode=AllowYield)  Line 196
 WebCore::HTMLDocumentParser::pumpTokenizerIfPossible(WebCore::HTMLDocumentParser::SynchronousMode mode=AllowYield)  Line 151
 WebCore::HTMLDocumentParser::append(const WebCore::SegmentedString & source={...})  Line 282
 WebCore::DecodedDataDocumentParser::appendBytes(WebCore::DocumentWriter * writer=0x0508b18c, const char * data=0x00000000, int length=0, bool shouldFlush=true)  Line 55 + 0x1f bytes
 WebCore::DocumentWriter::addData(const char * str=0x00000000, int len=0, bool flush=true)  Line 200 + 0x20 bytes
 WebCore::DocumentWriter::endIfNotLoadingMainResource()  Line 221
 WebCore::DocumentWriter::end()  Line 207
 WebCore::DocumentLoader::finishedLoading()  Line 270
 WebCore::FrameLoader::finishedLoading()  Line 2160
Comment 6 Eric Seidel (no email) 2010-08-18 09:26:55 PDT
Sweet!  A new test for the parser.

No one has shipped this.  I'm not sure this needs to be a security bug.  Adam, Tony or I will have a patch landed this afternoon.
Comment 7 Eric Seidel (no email) 2010-08-18 09:30:28 PDT
Thanks again for the test case!

This is obviously security related, but doesn't need to be marked sensitive since this code has not (and will not) ship as is.
Comment 8 Abhishek Arya 2010-08-18 09:37:18 PDT
Thanks Eric, Adam, Tony for taking care of it. 

Another security bug off our radar :) Marking it security helps to keep track, but since i am on the cc, i will update chromium side bug when it gets fixed - http://code.google.com/p/chromium/issues/detail?id=52581.
Comment 9 Eric Seidel (no email) 2010-08-18 09:46:40 PDT
I suspect we'll find a number of edge-cases like this in the new tree builder.  They'll be simple to fix (unlike the old tree builder), but I expect there may be a bunch, so I'm glad you're fuzzing.
Comment 10 Tony Gentilcore 2010-08-18 13:05:37 PDT
Eric and I chatted about this briefly on IRC. I'll take a look.
Comment 11 Tony Gentilcore 2010-08-18 14:53:08 PDT
(In reply to comment #10)
> Eric and I chatted about this briefly on IRC. I'll take a look.

I'm releasing this bug for now so that I can focus on 44129 (which turned out to be more involved than I thought).

Eric, if you don't get to this first, I'm happy to pick it up again.

Here are my notes so far:

TreeBuilder has this branch:
if causesFosterParenting
  tree.fosterParent()
else
  commonAncestor.appendChild()

This case takes the fosterParent() route for the <svg> element which invokes attachAtSite() and calls parserAddChild(). That method doesn't handle reparenting.

Changing parserAddChild() to appendChild() fixes the crash because it handles reparenting. However, I'm positive that is not the solution in light of our discussion of mutation events. Perhaps the answer is to teach parserAddChild() about reparenting or in some other way address the FIXME in attach() about attachAtSite() not handling foster parenting.
Comment 12 Tony Gentilcore 2010-08-19 11:09:06 PDT
It is looking like a single patch that reparents in AAA step 7 and uses parser* methods for insertBefore, removeChild, addChild will fix this and 44129.
Comment 13 Tony Gentilcore 2010-08-20 17:28:54 PDT
Crash fixed by r65769.

The dom tree doesn't seem right. Filed spec bug here:
http://www.w3.org/Bugs/Public/show_bug.cgi?id=10409