Bug 119570 - Null-pointer derefence in WebCore::StylePropertySet::propertyCount
Summary: Null-pointer derefence in WebCore::StylePropertySet::propertyCount
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Renata Hodovan
URL:
Keywords:
Depends on:
Blocks: 116980
  Show dependency tree / graph
 
Reported: 2013-08-08 05:12 PDT by Renata Hodovan
Modified: 2014-02-11 02:32 PST (History)
6 users (show)

See Also:

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 an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Renata Hodovan 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
Comment 1 Renata Hodovan 2013-08-08 05:25:25 PDT 
Created attachment 208327 [details]
Proposed patch
Comment 2 Andreas Kling 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 :)
Comment 3 Renata Hodovan 2013-08-08 05:51:01 PDT 
Created attachment 208331 [details]
Proposed patch with test case
Comment 4 Andreas Kling 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.
Comment 5 Renata Hodovan 2013-08-08 11:42:17 PDT 
Created attachment 208356 [details]
Proposed patch
Comment 6 Oliver Hunt 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?
Comment 7 Andreas Kling 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.
Comment 8 Darin Adler 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.
Comment 9 EFL EWS Bot 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
Comment 10 EFL EWS Bot 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
Comment 11 Renata Hodovan 2013-08-08 13:14:59 PDT 
Created attachment 208365 [details]
Fixing efl build issue and removing parentheses
Comment 12 Ryosuke Niwa 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?
Comment 13 Renata Hodovan 2013-08-09 05:07:31 PDT 
Committed r153883: <http://trac.webkit.org/changeset/153883>
Comment 14 Renata Hodovan 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.)