Bug 25567 – Crash when writing into a detached TITLE element

• Component: DOM
• Status: RESOLVED FIXED

[Berend-Jan Wever]

When the document title is set by document.write()-ing an HTML title tag to the page, then set again using document.title=... after which document.write() is used again, this causes a NULL ptr.

<SCRIPT>
  document.write("<title>x");
  document.title = "x";
  document.write("");
</SCRIPT>

Here's some debugger output for Chrome 2.0.175.0 (WebKit 530.6) on Windows 5.1.2600(x86):

*** Start event av(1st)
-- Start info exception
ExceptionAddress: 034eda11 (chrome_28f0000!WebCore::Node::renderer+0x00000011)
   ExceptionCode: c0000005 (Access violation)
  ExceptionFlags: 00000000
NumberParameters: 2
   Parameter[0]: 00000000
   Parameter[1]: 00000020
Attempt to read from address 00000020
--- End info exception
-- Start info code
chrome_28f0000!WebCore::Node::renderer+0x11:
034eda11 8b4020          mov     eax,dword ptr [eax+20h]
034eda14 8be5            mov     esp,ebp
034eda16 5d              pop     ebp
034eda17 c3              ret
034eda18 cc              int     3
034eda19 cc              int     3
034eda1a cc              int     3
034eda1b cc              int     3
--- End info code
-- Start info process
0n2964 D:\trunk\src\chrome\debug\chrome.exe
--- End info process
-- Start info module
start    end        module name
028f0000 05cec000   chrome_28f0000 chrome.dll  
--- End info module
-- Start info registers
eax=00000000 ebx=03419b90 ecx=00000000 edx=00000000 esi=05fef3f0 edi=05fef424 esp=05fef3a8 ebp=05fef3ac eip=034eda11
--- End info registers
-- Start info stack
ChildEBP RetAddr  
05fef3ac 035cf42c chrome_28f0000!WebCore::Node::renderer(void)+0x11
05fef3dc 036764e6 chrome_28f0000!WebCore::Node::createRendererIfNeeded(void)+0xac
05fef3e8 03a2ce1d chrome_28f0000!WebCore::Text::attach(void)+0x16
05fef424 03a2c6eb chrome_28f0000!WebCore::HTMLParser::insertNode(class WebCore::Node * n = 0x097cf020, bool flat = false)+0x28d
05fef48c 038a78ef chrome_28f0000!WebCore::HTMLParser::parseToken(struct WebCore::Token * t = 0x0fcb5038)+0x24b
05fef4c0 038a6b8b chrome_28f0000!WebCore::HTMLTokenizer::processToken(void)+0x1df
05fef574 0358d3f5 chrome_28f0000!WebCore::HTMLTokenizer::write(class WebCore::SegmentedString * str = 0x05fef5a0, bool appendData = false)+0x5db
05fef58c 0358d444 chrome_28f0000!WebCore::Document::write(class WebCore::SegmentedString * text = 0x05fef5a0, class WebCore::Document * ownerDocument = 0x08e2f020)+0x75
05fef5d4 02ab3ad6 chrome_28f0000!WebCore::Document::write(class WebCore::String * text = 0x05fef5e8, class WebCore::Document * ownerDocument = 0x08e2f020)+0x34
05fef600 03419eb9 chrome_28f0000!WebCore::V8Custom::v8HTMLDocumentWriteCallback(class v8::Arguments * args = 0x05fef660)+0x96
05fef738 064f018b chrome_28f0000!v8::internal::Builtin_HandleApiCall(int __argc__ = 2, class v8::internal::Object ** __argv__ = 0x05fef75c)+0x329
WARNING: Frame IP not in any known module. Following frames may be wrong.
05fef824 03351891 0x64f018b
05fef8b8 03351762 chrome_28f0000!v8::internal::Invoke(bool construct = true, class v8::internal::Handle<v8::internal::JSFunction> func = class v8::internal::Handle<v8::internal::JSFunction>, class v8::internal::Handle<v8::internal::Object> receiver = class v8::internal::Handle<v8::internal::Object>, int argc = 113380529, class v8::internal::Object *** args = 0x05fef7cc, bool * has_pending_exception = 0x06502fe9)+0x111
05fef8dc 03320bb8 chrome_28f0000!v8::internal::Execution::Call(class v8::internal::Handle<v8::internal::JSFunction> func = class v8::internal::Handle<v8::internal::JSFunction>, class v8::internal::Handle<v8::internal::Object> receiver = class v8::internal::Handle<v8::internal::Object>, int argc = 0, class v8::internal::Object *** args = 0x00000000, bool * pending_exception = 0x05fef92b)+0x22
05fef96c 02a5e5ef chrome_28f0000!v8::Function::Call(class v8::Handle<v8::Object> recv = class v8::Handle<v8::Object>, int argc = 0, class v8::Handle<v8::Value> * argv = 0x00000000)+0x108
05fef9a4 02ace0bf chrome_28f0000!WebCore::V8Proxy::CallFunction(class v8::Handle<v8::Function> function = class v8::Handle<v8::Function>, class v8::Handle<v8::Object> receiver = class v8::Handle<v8::Object>, int argc = 0, class v8::Handle<v8::Value> * args = 0x00000000)+0x5f
05fefa20 035cb3da chrome_28f0000!WebCore::ScheduledAction::execute(class WebCore::ScriptExecutionContext * context = 0x08e2f054)+0xff
05fefa5c 036920c5 chrome_28f0000!WebCore::DOMTimer::fired(void)+0x12a
05fefa94 036921ea chrome_28f0000!WebCore::ThreadTimers::fireTimers(double fireTime = 1241520087.5222499, class WTF::Vector<WebCore::TimerBase *,0> * firingTimers = 0x05fefaac)+0xb5
05fefac8 03692136 chrome_28f0000!WebCore::ThreadTimers::sharedTimerFiredInternal(void)+0xaa
05fefad0 02904562 chrome_28f0000!WebCore::ThreadTimers::sharedTimerFired(void)+0x16
05fefae0 02904fdc chrome_28f0000!webkit_glue::WebKitClientImpl::DoTimeout(void)+0x22
05fefaec 02904af4 chrome_28f0000!DispatchToMethod<webkit_glue::WebKitClientImpl,void (class webkit_glue::WebKitClientImpl * obj = 0x061a5020, <function> * method = 0x02904540, struct Tuple0 * arg = 0x05fefb03)+0xc
05fefb08 03ac4b29 chrome_28f0000!base::BaseTimer<webkit_glue::WebKitClientImpl,0>::TimerTask::Run(void)+0x54
05fefbbc 03ac4bd5 chrome_28f0000!MessageLoop::RunTask(class Task * task = 0x08f35020)+0xb9
05fefbcc 03ac5246 chrome_28f0000!MessageLoop::DeferOrRunPendingTask(struct MessageLoop::PendingTask * pending_task = 0x05fefbf0)+0x35
05fefc10 03b51aa0 chrome_28f0000!MessageLoop::DoDelayedWork(class base::Time * next_delayed_work_time = 0x05dbf038)+0x116
05fefcf8 03ac444b chrome_28f0000!base::MessagePumpDefault::Run(class base::MessagePump::Delegate * delegate = 0x05fefeb0)+0xf0
05fefda8 03ac42b0 chrome_28f0000!MessageLoop::RunInternal(void)+0xfb
05fefde0 03ac413a chrome_28f0000!MessageLoop::RunHandler(void)+0x90
05fefe08 03ae9138 chrome_28f0000!MessageLoop::Run(void)+0x3a
05feffa4 03ae8271 chrome_28f0000!base::Thread::ThreadMain(void)+0xb8
05feffb4 7c80b729 chrome_28f0000!`anonymous namespace'::ThreadFunc(void * closure = 0x05d7702c)+0x21
05feffec 00000000 kernel32!GetModuleFileNameA+0x1ba
--- End info stack
*** End event av(1st)

[Eric Seidel (no email)]

This bug is definitely real.  But it's more complicated than my little brain can handle at this hour.

#0	0x03a743ae in WebCore::Node::createRendererIfNeeded at Node.cpp:1261
#1	0x03cce5b7 in WebCore::Text::attach at Text.cpp:250
#2	0x037f228e in WebCore::HTMLParser::insertNode at HTMLParser.cpp:377
#3	0x037f298e in WebCore::HTMLParser::parseToken at HTMLParser.cpp:243
#4	0x03809280 in WebCore::HTMLTokenizer::processToken at HTMLTokenizer.cpp:1887
#5	0x038095f0 in WebCore::HTMLTokenizer::end at HTMLTokenizer.cpp:1805
#6	0x03809a51 in WebCore::HTMLTokenizer::finish at HTMLTokenizer.cpp:1856

is the call stack.  The TextNode is detached by the time the parser gets along to the point in insertNode where it tries to attach it.  we hit ASSERT(parentNode()) or just crash after that in release builds.

[Alexey Proskuryakov]

Such a crash usually means that Node::appendChild() failed, but its result was ignored.

[Alexey Proskuryakov]

In Firefox, this test case results in two <title> elements being present in the document.

> Such a crash usually means

Apparently not in this case, though.

[mvijay]

Hi,

I have been analysing the bug and here are some of my observations.

In HTMLTokenizer, while parsing the "Title" tag, if there is no end "Title" tag, the state is reset to previous which it should not. This is causing the problem. So, comment out the following line in "if" condition to make this state regain its current value.

In HTMLTokenizer.cpp, line no.1532

if (state.inTitle() && src.isEmpty()) {
//comment the following line as this statement resets the state to previous one.
//state = savedState;
src = savedSrc;
m_lineNumber = savedLineno;
m_scriptCodeSize = 0;
}

After commenting this statement, the safari browser is not crashing.
Let me know if this proposal can be used to fix this bug.

[Berend-Jan Wever]

I am seeing a few reports of a ReadAV [NULL+0x1C]@chrome!MallocExtension::ReleaseFreeMemory+0x314e with this stack for the same repro:

chrome!MallocExtension::ReleaseFreeMemory+0x314e
chrome!MallocExtension::ReleaseFreeMemory+0x68d38
chrome!MallocExtension::GetNumericProperty+0x66139
chrome!MallocExtension::ReadStackTraces+0xa930b
chrome!MallocExtension::ReadStackTraces+0xa94db
chrome!MallocExtension::ReadStackTraces+0xaa00c
chrome!tcmalloc::StackTraceTable::bucket_total+0x102a37
chrome!tcmalloc::StackTraceTable::depth_total+0x2a7a
chrome!tcmalloc::StackTraceTable::depth_total+0x35d1
chrome!tcmalloc::StackTraceTable::depth_total+0x3ce7
chrome!tcmalloc::StackTraceTable::depth_total+0x4041
chrome!tcmalloc::StackTraceTable::depth_total+0xb49
chrome!MallocExtension::GetEstimatedAllocatedSize+0xce1c7
chrome!_sbrk+0x4ca4a
chrome!_sbrk+0x780a1
chrome!_sbrk+0x4bfd7
chrome!_sbrk+0x4c400
chrome!_sbrk+0x4c70d
chrome!_sbrk+0x5fc4a
chrome!_sbrk+0x5e7dd
kernel32!GetModuleFileNameA+0x1ba

This may be an indication that this is exploitable. I am seeing no other crashes or non-NULL values in the pointers, so I am not overly worried. Marking as security just in case... feel free to remove the flag if you find the root cause and determine it is not exploitable.

[Alexey Proskuryakov]

<rdar://problem/6980242>

[Alexey Proskuryakov]

*** Bug 26251 has been marked as a duplicate of this bug. ***

[Berend-Jan Wever]

The stack I reported earlier (which seemed to suggest memory corruption) is probably misleading because I used bad symbols - the offsets in the functions are too large for a decent stack.

So, I loaded the repro in Chrome 100 times to see what crashes I got 100 hits for the NULL pointer. I think it's safe to say that this is a reliable crash and not exploitable, so I am removing the security label.

[Hajime Morrita]

Created attachment 52002 [details]
v0; add a guard for orphan node

[Hajime Morrita]

In  HTMLParser::insertNode(), A new child of the <title> node can be removed 
inside  ContatinerNode::addChild() for its own(!)
because HTMLTitleElement::childrenChanged() try to concatenate its children.

Another alternative for this fix would be handling orphan children on ContainerNode::addChild().
But doing so, It would be hard for HTMLParser to know what type of error happened,
and implementing correct HTMLParser::reportError() wouldn't be trivial.
This is even not  an error because the contents of the new node get inside the tree (as a flatten form).
So handling the case inside HTMLParser looks better.

[Hajime Morrita]

Fixing this bug unveils another, that I filed on Bug 36802.
I'll fix it after this one is closed.

[Alexey Proskuryakov]

What exactly is the bug here? Is it that having a detached title causes a crash, or that the title becomes detached, in the first place?

What does HTML5 say?

[Hajime Morrita]

ap, thank you to give a comment.

> What exactly is the bug here? Is it that having a detached title causes a
> crash, or that the title becomes detached, in the first place?
This bug is about assertion failure.
I think a summary line is irrelevant. 
<title> element is not detached. Its new children is detached 
and failed to attach() because it is not on the tree.

For following example:
  document.write("<title>x");
  document.title = "y";
  document.write("");

The child text node "x" is detached because when adding "x", 
there already another child text node "y",
which is made by document.title setter, 
and code looks not assume such case.
By trying to append "x" trigger HTMLTitleElement to concatinating its child, 
and the concatination process removes "x"  from <title>.
(and new child "yx" remains.)

About HTML5, It says
- setting document.title should create <title> element unless there already is.
- existing children should be removed on setting documen.title
http://www.whatwg.org/specs/web-apps/current-work/multipage/dom.html#document.title

But it is not clear if we care "x" as inserted into the tree or not when accessing title property.

Another idea is flushing pending stream when setting  document.title.
But I think it would break another part of the DOM...

[Hajime Morrita]

> This bug is about assertion failure.
And this is crash bug on release mode, as reported above.

[Alexey Proskuryakov]

Created attachment 52058 [details]
test case for flushing

> But it is not clear if we care "x" as inserted into the tree or not when
> accessing title property.

I think that we should care somewhat. It makes sense to at least investigate what HTML5 says about this - it's important to have HTML5 say sensible things about parsing, because otherwise, we wouldn't be able to make our parser more standard compliant in the future.

I'm attaching a test case that shows largely inconsistent behavior in Safari and Firefox. Safari never flushes the text content, but creates the nodes (see bug 8961). Firefox doesn't even create the <title> node at first (which is why it ends up having two title nodes in original test case). It flushes input stream for <p>, though.

You mentioned that with this patch, we'll be getting "yx" as title. This behavior seems counter-intuitive.

+It is OK not to crash. Mixing document.write() and docuent.title make the title child node orphan, which did cause a crash.

As mentioned in another bug, "It is OK not to crash" doesn't describe the test expectation precisely enough.

[Alexey Proskuryakov]

Comment on attachment 52002 [details]
v0; add a guard for orphan node

Marking r- to get this out of review queue. Depending on investigation results, we may or may not end up making this exact change, but the test needs some rewording.

+    // Newly created nodes can be removed immediately inside
+    // Node::childrenChanged() inside Node::addChild() due to DOM
+    // normalization process like concatinating <title> text contents.
+    // We should skip processing such nodes because their contents already merged into the tree. 

Typo: should be "concatenation". I wonder if there are any other cases where this could happen, besides setting document.title. Ideally, we'd need as many test cases for different scenarios as possible.

[Hajime Morrita]

ap, thank you for you suggestion.

> I think that we should care somewhat. It makes sense to at least investigate
> what HTML5 says about this - it's important to have HTML5 say sensible things
> about parsing, because otherwise, we wouldn't be able to make our parser more
> standard compliant in the future.
Agreed. I read it again and found that:
- text that is passed to document.write() should be flushed - or in spec term,
   "processed" until the tokenizer reaches an insertion point.
   So the last patch is wrong.
http://www.whatwg.org/specs/web-apps/current-work/multipage/apis-in-html-documents.html#document.write()

I'll look around  the code further.

>Ideally, we'd need as many
>test cases for different scenarios as possible.
Yes. we might have same problem with innerHTML, textarea, etc...

[Hajime Morrita]

Created attachment 52153 [details]
v1; prevent implicit DOM mutation during appendChild() for title element

[Hajime Morrita]

Updated patch. 
original code causes unexpected DOM mutation during addChild(), 
that make addChild() fail,  that causes crash. 
This patch prevent such implicit mutation. for HTMLTitleElement. 
now addChild() really append a new child.

BTW on parsing issue, I file it on Bug 31881 because that is not only for <title> and
I think we should tackle it separately.

[WebKit Commit Bot]

Comment on attachment 52153 [details]
v1; prevent implicit DOM mutation during appendChild() for title element

Clearing flags on attachment: 52153

Committed r56895: <http://trac.webkit.org/changeset/56895>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

> BTW on parsing issue, I file it on Bug 31881 because that is not only for
> <title> and I think we should tackle it separately.

OK. This doesn't look like correct bug number though - did you mean bug 36881?

[Hajime Morrita]

(In reply to comment #22)
> OK. This doesn't look like correct bug number though - did you mean bug 36881?
Oops. You are right. it's bug 36881. Thank you for the correction.