Bug 119570

Summary:

Null-pointer derefence in WebCore::StylePropertySet::propertyCount

Product:

WebKit

Reporter:

Renata Hodovan <rhodovan.u-szeged>

Component:

HTML Editing

Assignee:

Renata Hodovan <rhodovan.u-szeged>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

eflews.bot, gyuyoung.kim, kling, oliver, rniwa, tmpsantos

Priority:

Version:

528+ (Nightly build)

Hardware:

OS:

All

Bug Depends on:

Bug Blocks:

116980

Attachments:

Description	Flags
Proposed patch	kling: review-, rhodovan.u-szeged: commit-queue-
Proposed patch with test case	rhodovan.u-szeged: commit-queue-
Proposed patch	kling: review+, rhodovan.u-szeged: commit-queue-
Fixing efl build issue and removing parentheses	none

Renata Hodovan

Reported 2013-08-08 05:12:51 PDT

The following test crashes due to a null-pointer dereference issue: <html> <body> <table> <tr> <td contenteditable="plaintext-only">171</td> </tr> </table> <script> document.designMode = "on"; document.execCommand("SelectAll"); document.execCommand("CreateLink", 1, 'foo'); </script> </body> </html> The backtrace: Program received signal SIGSEGV, Segmentation fault. 0x00007ffff411bd4e in WebCore::StylePropertySet::propertyCount (this=0x0) at /home/reni/Data/REPOS/webkit_sec/Source/WebCore/css/StylePropertySet.h:254 254 if (m_isMutable) (gdb) bt #0 0x00007ffff411bd4e in WebCore::StylePropertySet::propertyCount (this=0x0) at /home/reni/Data/REPOS/webkit_sec/Source/WebCore/css/StylePropertySet.h:254 #1 0x00007ffff4148b86 in WebCore::MutableStylePropertySet::mergeAndOverrideOnConflict (this=0x8ce7a0, other=0x0) at /home/reni/Data/REPOS/webkit_sec/Source/WebCore/css/StylePropertySet.cpp:1039 #2 0x00007ffff42aff7c in WebCore::ApplyStyleCommand::applyInlineStyleToNodeRange (this=0x8d0620, style=0x8cd440, startNode=..., pastEndNode=...) at /home/reni/Data/REPOS/webkit_sec/Source/WebCore/editing/ApplyStyleCommand.cpp:774 #3 0x00007ffff42af9cd in WebCore::ApplyStyleCommand::fixRangeAndApplyInlineStyle (this=0x8d0620, style=0x8cd440, start=..., end=...) at /home/reni/Data/REPOS/webkit_sec/Source/WebCore/editing/ApplyStyleCommand.cpp:711 #4 0x00007ffff42af5e9 in WebCore::ApplyStyleCommand::applyInlineStyle (this=0x8d0620, style=0x8cd440) at /home/reni/Data/REPOS/webkit_sec/Source/WebCore/editing/ApplyStyleCommand.cpp:674 #5 0x00007ffff42ac785 in WebCore::ApplyStyleCommand::doApply (this=0x8d0620) at /home/reni/Data/REPOS/webkit_sec/Source/WebCore/editing/ApplyStyleCommand.cpp:225 #6 0x00007ffff42bbdfa in WebCore::CompositeEditCommand::applyCommandToComposite (this=0x8d03b0, prpCommand=...) at /home/reni/Data/REPOS/webkit_sec/Source/WebCore/editing/CompositeEditCommand.cpp:266 #7 0x00007ffff42bc0e5 in WebCore::CompositeEditCommand::applyStyledElement (this=0x8d03b0, element=...) at /home/reni/Data/REPOS/webkit_sec/Source/WebCore/editing/CompositeEditCommand.cpp:297 #8 0x00007ffff42c85e4 in WebCore::CreateLinkCommand::doApply (this=0x8d03b0) at /home/reni/Data/REPOS/webkit_sec/Source/WebCore/editing/CreateLinkCommand.cpp:50 #9 0x00007ffff42bbbc2 in WebCore::CompositeEditCommand::apply (this=0x8d03b0) at /home/reni/Data/REPOS/webkit_sec/Source/WebCore/editing/CompositeEditCommand.cpp:215 #10 0x00007ffff42bb94a in WebCore::applyCommand (command=...) at /home/reni/Data/REPOS/webkit_sec/Source/WebCore/editing/CompositeEditCommand.cpp:171 #11 0x00007ffff42ecae9 in WebCore::executeCreateLink (frame=0x7aff80, value=...) at /home/reni/Data/REPOS/webkit_sec/Source/WebCore/editing/EditorCommand.cpp:293 #12 0x00007ffff42f0c7a in WebCore::Editor::Command::execute (this=0x7fffffffb680, parameter=..., triggeringEvent=0x0) at /home/reni/Data/REPOS/webkit_sec/Source/WebCore/editing/EditorCommand.cpp:1706 #13 0x00007ffff41c19b0 in WebCore::Document::execCommand (this=0x87ab60, commandName=..., userInterface=true, value=...) at /home/reni/Data/REPOS/webkit_sec/Source/WebCore/dom/Document.cpp:4150 #14 0x00007ffff4ec743a in WebCore::jsDocumentPrototypeFunctionExecCommand (exec=0x7fffe40a10b0) at generated/JSDocument.cpp:2748 #15 0x00007fff9ffff0e5 in ?? () #16 0x00007fffffffb820 in ?? () #17 0x00007ffff6814614 in llint_op_call () from /home/reni/Data/REPOS/webkit_sec/WebKitBuild/Debug/lib/libQt5WebKit.so.5 #18 0x00007fffffffb7d0 in ?? () #19 0x00007ffff555005b in JSC::JSStack::installTrapsAfterFrame (this=0x0, frame=0x0) at /home/reni/Data/REPOS/webkit_sec/Source/JavaScriptCore/interpreter/JSStackInlines.h:212 #20 0x00007ffff5560ba4 in JSC::JITCode::execute (this=0x8b5be0, stack=0x772c58, callFrame=0x7fffe40a1058, vm=0x7f5ce0) at /home/reni/Data/REPOS/webkit_sec/Source/JavaScriptCore/jit/JITCode.cpp:46 #21 0x00007ffff554c8e1 in JSC::Interpreter::execute (this=0x772c40, program=0x7fff863afef0, callFrame=0x7fff9c05f8e0, thisObj=0x7fffe403ffd8) at /home/reni/Data/REPOS/webkit_sec/Source/JavaScriptCore/interpreter/Interpreter.cpp:851 #22 0x00007ffff562b776 in JSC::evaluate (exec=0x7fff9c05f8e0, source=..., thisValue=..., returnedException=0x7fffffffc520) at /home/reni/Data/REPOS/webkit_sec/Source/JavaScriptCore/runtime/Completion.cpp:83 #23 0x00007ffff3f7097d in WebCore::JSMainThreadExecState::evaluate (exec=0x7fff9c05f8e0, source=..., thisValue=..., exception=0x7fffffffc520) at /home/reni/Data/REPOS/webkit_sec/Source/WebCore/bindings/js/JSMainThreadExecState.h:74 #24 0x00007ffff3f8f182 in WebCore::ScriptController::evaluateInWorld (this=0x773770, sourceCode=..., world=0x7674f0) at /home/reni/Data/REPOS/webkit_sec/Source/WebCore/bindings/js/ScriptController.cpp:142 #25 0x00007ffff3f8f288 in WebCore::ScriptController::evaluate (this=0x773770, sourceCode=...) at /home/reni/Data/REPOS/webkit_sec/Source/WebCore/bindings/js/ScriptController.cpp:158 #26 0x00007ffff427c72f in WebCore::ScriptElement::executeScript (this=0x8b69a8, sourceCode=...) at /home/reni/Data/REPOS/webkit_sec/Source/WebCore/dom/ScriptElement.cpp:316 #27 0x00007ffff427bf0a in WebCore::ScriptElement::prepareScript (this=0x8b69a8, scriptStartPosition=..., supportLegacyTypes=WebCore::ScriptElement::DisallowLegacyTypeInTypeAttribute) at /home/reni/Data/REPOS/webkit_sec/Source/WebCore/dom/ScriptElement.cpp:245 #28 0x00007ffff442c215 in WebCore::HTMLScriptRunner::runScript (this=0x7b2b00, script=0x8b6940, scriptStartPosition=...) at /home/reni/Data/REPOS/webkit_sec/Source/WebCore/html/parser/HTMLScriptRunner.cpp:312 #29 0x00007ffff442b966 in WebCore::HTMLScriptRunner::execute (this=0x7b2b00, scriptElement=..., scriptStartPosition=...) at /home/reni/Data/REPOS/webkit_sec/Source/WebCore/html/parser/HTMLScriptRunner.cpp:181 ---Type <return> to continue, or q <return> to quit--- #30 0x00007ffff4418871 in WebCore::HTMLDocumentParser::runScriptsForPausedTreeBuilder (this=0x7745c0) at /home/reni/Data/REPOS/webkit_sec/Source/WebCore/html/parser/HTMLDocumentParser.cpp:271 #31 0x00007ffff441895c in WebCore::HTMLDocumentParser::canTakeNextToken (this=0x7745c0, mode=WebCore::HTMLDocumentParser::AllowYield, session=...) at /home/reni/Data/REPOS/webkit_sec/Source/WebCore/html/parser/HTMLDocumentParser.cpp:290 #32 0x00007ffff4418f74 in WebCore::HTMLDocumentParser::pumpTokenizer (this=0x7745c0, mode=WebCore::HTMLDocumentParser::AllowYield) at /home/reni/Data/REPOS/webkit_sec/Source/WebCore/html/parser/HTMLDocumentParser.cpp:535 #33 0x00007ffff44186df in WebCore::HTMLDocumentParser::pumpTokenizerIfPossible (this=0x7745c0, mode=WebCore::HTMLDocumentParser::AllowYield) at /home/reni/Data/REPOS/webkit_sec/Source/WebCore/html/parser/HTMLDocumentParser.cpp:235 #34 0x00007ffff441987e in WebCore::HTMLDocumentParser::append (this=0x7745c0, inputSource=...) at /home/reni/Data/REPOS/webkit_sec/Source/WebCore/html/parser/HTMLDocumentParser.cpp:747 #35 0x00007ffff41ae089 in WebCore::DecodedDataDocumentParser::flush (this=0x7745c0, writer=0x694190) at /home/reni/Data/REPOS/webkit_sec/Source/WebCore/dom/DecodedDataDocumentParser.cpp:60 #36 0x00007ffff45b011f in WebCore::DocumentWriter::end (this=0x694190) at /home/reni/Data/REPOS/webkit_sec/Source/WebCore/loader/DocumentWriter.cpp:245 #37 0x00007ffff45a2c98 in WebCore::DocumentLoader::finishedLoading (this=0x6940f0, finishTime=0) at /home/reni/Data/REPOS/webkit_sec/Source/WebCore/loader/DocumentLoader.cpp:402 #38 0x00007ffff45a2a06 in WebCore::DocumentLoader::notifyFinished (this=0x6940f0, resource=0x7528b0) at /home/reni/Data/REPOS/webkit_sec/Source/WebCore/loader/DocumentLoader.cpp:344 #39 0x00007ffff4589d02 in WebCore::CachedResource::checkNotify (this=0x7528b0) at /home/reni/Data/REPOS/webkit_sec/Source/WebCore/loader/cache/CachedResource.cpp:369 #40 0x00007ffff4589dd8 in WebCore::CachedResource::finishLoading (this=0x7528b0) at /home/reni/Data/REPOS/webkit_sec/Source/WebCore/loader/cache/CachedResource.cpp:385 #41 0x00007ffff458652c in WebCore::CachedRawResource::finishLoading (this=0x7528b0, data=0x7a0920) at /home/reni/Data/REPOS/webkit_sec/Source/WebCore/loader/cache/CachedRawResource.cpp:94 #42 0x00007ffff45ecb11 in WebCore::SubresourceLoader::didFinishLoading (this=0x799800, finishTime=0) at /home/reni/Data/REPOS/webkit_sec/Source/WebCore/loader/SubresourceLoader.cpp:282 #43 0x00007ffff45e33ff in WebCore::ResourceLoader::didFinishLoading (this=0x799800, finishTime=0) at /home/reni/Data/REPOS/webkit_sec/Source/WebCore/loader/ResourceLoader.cpp:488 #44 0x00007ffff4a8b121 in WebCore::QNetworkReplyHandler::finish (this=0x7a8af0) at /home/reni/Data/REPOS/webkit_sec/Source/WebCore/platform/network/qt/QNetworkReplyHandler.cpp:516 #45 0x00007ffff4a89e40 in WebCore::QNetworkReplyHandlerCallQueue::flush (this=0x7a8b28) at /home/reni/Data/REPOS/webkit_sec/Source/WebCore/platform/network/qt/QNetworkReplyHandler.cpp:250 #46 0x00007ffff4a89b3d in WebCore::QNetworkReplyHandlerCallQueue::push (this=0x7a8b28, method=(void (WebCore::QNetworkReplyHandler::*)(WebCore::QNetworkReplyHandler * const)) 0x7ffff4a8af66 <WebCore::QNetworkReplyHandler::finish()>) at /home/reni/Data/REPOS/webkit_sec/Source/WebCore/platform/network/qt/QNetworkReplyHandler.cpp:216 #47 0x00007ffff4a8aa8a in WebCore::QNetworkReplyWrapper::didReceiveFinished (this=0x796a40) at /home/reni/Data/REPOS/webkit_sec/Source/WebCore/platform/network/qt/QNetworkReplyHandler.cpp:409 #48 0x00007ffff4a8d41a in WebCore::QNetworkReplyWrapper::qt_static_metacall (_o=0x796a40, _c=QMetaObject::InvokeMetaMethod, _id=1, _a=0x7fffffffcf70) at .moc/release-shared/moc_QNetworkReplyHandler.cpp:176 #49 0x00007ffff22175cb in QMetaObject::activate(QObject*, int, int, void**) () from /usr/local/Trolltech/Qt5/Qt-5.0.0-r40/lib/libQt5Core.so.5 #50 0x00007ffff221884e in QObject::event(QEvent*) () from /usr/local/Trolltech/Qt5/Qt-5.0.0-r40/lib/libQt5Core.so.5 #51 0x00007ffff305edbc in QApplicationPrivate::notify_helper(QObject*, QEvent*) () from /usr/local/Trolltech/Qt5/Qt-5.0.0-r40/lib/libQt5Widgets.so.5 #52 0x00007ffff3062075 in QApplication::notify(QObject*, QEvent*) () from /usr/local/Trolltech/Qt5/Qt-5.0.0-r40/lib/libQt5Widgets.so.5 #53 0x00007ffff21f2dbe in QCoreApplication::notifyInternal(QObject*, QEvent*) () from /usr/local/Trolltech/Qt5/Qt-5.0.0-r40/lib/libQt5Core.so.5 #54 0x00007ffff21f4a76 in QCoreApplicationPrivate::sendPostedEvents(QObject*, int, QThreadData*) () from /usr/local/Trolltech/Qt5/Qt-5.0.0-r40/lib/libQt5Core.so.5 #55 0x00007ffff223a333 in ?? () from /usr/local/Trolltech/Qt5/Qt-5.0.0-r40/lib/libQt5Core.so.5 #56 0x00007fffee37f0a6 in g_main_dispatch (context=0x6632f0) at /build/buildd/glib2.0-2.37.3/./glib/gmain.c:3058 #57 g_main_context_dispatch (context=context@entry=0x6632f0) at /build/buildd/glib2.0-2.37.3/./glib/gmain.c:3634 #58 0x00007fffee37f3f8 in g_main_context_iterate (context=context@entry=0x6632f0, block=block@entry=1, dispatch=dispatch@entry=1, self=<optimized out>) at /build/buildd/glib2.0-2.37.3/./glib/gmain.c:3705 #59 0x00007fffee37f49c in g_main_context_iteration (context=0x6632f0, may_block=1) at /build/buildd/glib2.0-2.37.3/./glib/gmain.c:3766 #60 0x00007ffff223a4bc in QEventDispatcherGlib::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) () from /usr/local/Trolltech/Qt5/Qt-5.0.0-r40/lib/libQt5Core.so.5 ---Type <return> to continue, or q <return> to quit--- #61 0x00007ffff21f1d3b in QEventLoop::exec(QFlags<QEventLoop::ProcessEventsFlag>) () from /usr/local/Trolltech/Qt5/Qt-5.0.0-r40/lib/libQt5Core.so.5 #62 0x00007ffff21f5120 in QCoreApplication::exec() () from /usr/local/Trolltech/Qt5/Qt-5.0.0-r40/lib/libQt5Core.so.5 #63 0x0000000000421ba0 in launcherMain (app=...) at /home/reni/Data/REPOS/webkit_sec/Tools/QtTestBrowser/qttestbrowser.cpp:49 #64 0x0000000000423680 in main (argc=2, argv=0x7fffffffdc48) at /home/reni/Data/REPOS/webkit_sec/Tools/QtTestBrowser/qttestbrowser.cpp:318

Attachments
Proposed patch (2.10 KB, patch) 2013-08-08 05:25 PDT, Renata Hodovan	kling: review- rhodovan.u-szeged: commit-queue-	Details Formatted Diff Diff
Proposed patch with test case (4.72 KB, patch) 2013-08-08 05:51 PDT, Renata Hodovan	rhodovan.u-szeged: commit-queue-	Details Formatted Diff Diff
Proposed patch (7.59 KB, patch) 2013-08-08 11:42 PDT, Renata Hodovan	kling: review+ rhodovan.u-szeged: commit-queue-	Details Formatted Diff Diff
Fixing efl build issue and removing parentheses (8.67 KB, patch) 2013-08-08 13:14 PDT, Renata Hodovan	no flags	Details Formatted Diff Diff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.

Renata Hodovan

Comment 1 2013-08-08 05:25:25 PDT

Created attachment 208327 [details] Proposed patch

Andreas Kling

Comment 2 2013-08-08 05:39:57 PDT

Comment on attachment 208327 [details] Proposed patch Needs a test. You already have a repro, so it shouldn't be hard :)

Renata Hodovan

Comment 3 2013-08-08 05:51:01 PDT

Created attachment 208331 [details] Proposed patch with test case

Andreas Kling

Comment 4 2013-08-08 06:20:25 PDT

Comment on attachment 208331 [details] Proposed patch with test case View in context: https://bugs.webkit.org/attachment.cgi?id=208331&action=review > Source/WebCore/ChangeLog:3 > + Null-pointer derefence in WebCore::StylePropertySet::propertyCount I don't think this name accurately describes the bug. The issue (and fix) is that ApplyStyleCommand::applyInlineStyleToNodeRange() is missing a null check. That said, I think we should be more defensive here. Since MutableStylePropertySet::mergeAndOverrideOnConflict() will not work when passed a null pointer, it would best to disallow it completely by changing it to take a const reference instead of a const pointer. You still have to dereference the pointer at the call site(s), but it becomes much clearer that it must not be null. > Source/WebCore/ChangeLog:9 > + The m_mutableStyleSet of EditingStyle can be NULL in ApplyStyleCommand::applyInlineStyleToNodeRange if > + the third parameter of execCommand is invalid, thus a sanity check is added before the usage. What is invalid about the third parameter? AFAIK CreateLink should just create an anchor element, with the href attribute set to whatever value is passed to the command. The Internet Explorer documentation for CreateLink states that the value is "invalid" unless userInterface is true. Is that what you mean? Looking at executeCreateLink() in WebKit, I see nothing but a FIXME alluding to handling userInterface=1.

Renata Hodovan

Comment 5 2013-08-08 11:42:17 PDT

Created attachment 208356 [details] Proposed patch

Oliver Hunt

Comment 6 2013-08-08 11:45:17 PDT

Comment on attachment 208356 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=208356&action=review > Source/WebCore/editing/EditingStyle.cpp:1113 > + style->mergeAndOverrideOnConflict(*(static_pointer_cast<StyleRule>(matchedRules[i])->properties())); You don't need the () around the static_pointer_cast > Source/WebCore/editing/EditingStyle.cpp:-1155 > - m_mutableStyle->mergeAndOverrideOnConflict(fromComputedStyle.get()); do we want to null check fromComputedStyle?

Andreas Kling

Comment 7 2013-08-08 11:47:09 PDT

Comment on attachment 208356 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=208356&action=review r=me >> Source/WebCore/editing/EditingStyle.cpp:-1155 >> - m_mutableStyle->mergeAndOverrideOnConflict(fromComputedStyle.get()); > > do we want to null check fromComputedStyle? I think it's OK not to. It's created a few lines above (not visible in this diff) by a ::create() that cannot fail.

Darin Adler

Comment 8 2013-08-08 11:52:22 PDT

Comment on attachment 208356 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=208356&action=review > Source/WebCore/editing/ApplyStyleCommand.cpp:775 > + inlineStyle->mergeAndOverrideOnConflict(*(style->style())); Parentheses aren’t needed here either.

EFL EWS Bot

Comment 9 2013-08-08 12:18:30 PDT

Comment on attachment 208356 [details] Proposed patch Attachment 208356 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/1386612

EFL EWS Bot

Comment 10 2013-08-08 12:31:37 PDT

Comment on attachment 208356 [details] Proposed patch Attachment 208356 [details] did not pass efl-wk2-ews (efl-wk2): Output: http://webkit-queues.appspot.com/results/1436130

Renata Hodovan

Comment 11 2013-08-08 13:14:59 PDT

Created attachment 208365 [details] Fixing efl build issue and removing parentheses

Ryosuke Niwa

Comment 12 2013-08-08 15:27:14 PDT

Comment on attachment 208365 [details] Fixing efl build issue and removing parentheses View in context: https://bugs.webkit.org/attachment.cgi?id=208365&action=review > Source/WebCore/editing/ApplyStyleCommand.cpp:774 > + if (style->style()) Can we define a local variable inside the if here?

Renata Hodovan

Comment 13 2013-08-09 05:07:31 PDT

Committed r153883: <http://trac.webkit.org/changeset/153883>

Renata Hodovan

Comment 14 2013-08-09 05:08:26 PDT

(In reply to comment #12) > (From update of attachment 208365 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=208365&action=review > > > Source/WebCore/editing/ApplyStyleCommand.cpp:774 > > + if (style->style()) > > Can we define a local variable inside the if here? Done and landed. (The conflicts with r153882 were also resolved.)

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