| Summary: | Crash when writing into a detached TITLE element | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Berend-Jan Wever <skylined> | ||||||||
| Component: | DOM | Assignee: | Nobody <webkit-unassigned> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | ap, chinmaya, commit-queue, eric, morrita, skylined | ||||||||
| Priority: | P1 | Keywords: | GoogleBug, InRadar | ||||||||
| Version: | 528+ (Nightly build) | ||||||||||
| Hardware: | All | ||||||||||
| OS: | All | ||||||||||
| URL: | http://skypher.com/SkyLined/Repro/WebKit/Bug%2025567%20-%20HTML%20document.write%20and%20document.title%20NULL%20ptr/repro.html | ||||||||||
| Attachments: |
|
||||||||||
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. Such a crash usually means that Node::appendChild() failed, but its result was ignored. 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.
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.
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. *** Bug 26251 has been marked as a duplicate of this bug. *** 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. Created attachment 52002 [details]
v0; add a guard for orphan node
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. Fixing this bug unveils another, that I filed on Bug 36802. I'll fix it after this one is closed. 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? 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... > This bug is about assertion failure.
And this is crash bug on release mode, as reported above.
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. 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.
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... Created attachment 52153 [details]
v1; prevent implicit DOM mutation during appendChild() for title element
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. 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> All reviewed patches have been landed. Closing bug. > 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? (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. |
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)