Bug 44170

Summary:

HTML5 TreeBuilder ASSERTs on <a><svg><tr><input></a>

Product:

WebKit

Reporter:

Berend-Jan Wever <skylined>

Component:

WebCore Misc.

Assignee:

Tony Gentilcore <tonyg>

Status:

RESOLVED FIXED

Severity:

Major

CC:

abarth, ddkilzer, eric, inferno, sam, tonyg

Priority:

Keywords:

InRadar

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Bug Depends on:

43055, 44129

Bug Blocks:

44390

Attachments:

Description	Flags
Repro	none

Berend-Jan Wever

Reported 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>

Attachments
Repro (88 bytes, text/html) 2010-08-18 06:21 PDT, Berend-Jan Wever	no flags	Details
View All Add attachment proposed patch, testcase, etc.

Abhishek Arya

Comment 1 2010-08-18 09:04:21 PDT

Adding security as product.

Eric Seidel (no email)

Comment 2 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.

David Kilzer (:ddkilzer)

Comment 3 2010-08-18 09:09:33 PDT

<rdar://problem/8324425>

Eric Seidel (no email)

Comment 4 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. :)

Abhishek Arya

Comment 5 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

Eric Seidel (no email)

Comment 6 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.

Eric Seidel (no email)

Comment 7 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.

Abhishek Arya

Comment 8 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.

Eric Seidel (no email)

Comment 9 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.

Tony Gentilcore

Comment 10 2010-08-18 13:05:37 PDT

Eric and I chatted about this briefly on IRC. I'll take a look.

Tony Gentilcore

Comment 11 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.

Tony Gentilcore

Comment 12 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.

Tony Gentilcore

Comment 13 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

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