Bug 56004 – REGRESSION: crash in nextLinePosition when extending selection forward by line in an empty document

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Berend-Jan Wever]

Created attachment 85149 [details]
Repro

Chromium: 
http://code.google.com/p/chromium/issues/detail?id=75410

Repro:
<script>
  setTimeout(function() {
    document.open();
    _Selection_4 = getSelection();
    _Selection_4.setPosition(document,0);
    _Selection_4.modify("extend","forward","line"); // line, paragraph
  }, 1);
</script>

id:             chrome.dll!WebCore::lastPositionInNode ReadAV@NULL (23c8d35ca79c9994c6691ce3fa381d59)
description:    Attempt to read from unallocated NULL pointer in chrome.dll!WebCore::lastPositionInNode
application:    Chromium 11.0.696.0
stack:          chrome.dll!WebCore::lastPositionInNode
                chrome.dll!WebCore::nextLinePosition
                chrome.dll!WebCore::nextParagraphPosition
                chrome.dll!WebCore::SelectionController::modifyExtendingForward
                chrome.dll!WebCore::SelectionController::modify
                chrome.dll!WebCore::DOMSelection::modify
                chrome.dll!WebCore::DOMSelectionInternal::modifyCallback
                chrome.dll!v8::internal::HandleApiCallHelper<...>
                chrome.dll!v8::internal::Builtin_HandleApiCall
                chrome.dll!v8::internal::Invoke
                chrome.dll!v8::internal::Execution::Call
                ...

[Berend-Jan Wever]

Offending code in "\Source\WebCore\editing\visible_units.cpp":

VisiblePosition nextLinePosition(const VisiblePosition &visiblePosition, int x)
{
<snip>
    // Could not find a next line. This means we must already be on the last line.
    // Move to the end of the content in this block, which effectively moves us
    // to the end of the line we're on.
    Element* rootElement = node->isContentEditable() ? node->rootEditableElement() : node->document()->documentElement();
    return VisiblePosition(lastPositionInNode(rootElement), DOWNSTREAM);
}

"rootElement" can be NULL through "node->document()->documentElement()".
"lastPositionInNode" does not handle NULL ptrs.

A similar code construct exists in "previousLinePosition", but I couldn't get that to crash.

The comment in the code above seems to be incorrect; "Move to the end of the content in this block" should read "Move to the end of the document" as far as I can tell. Assuming this is the case, one could replace the code with a call to "endOfDocument". However, "endOfDocument" does not check for "node->isContentEditable()", or handle things differently if this is true.

In other words - I don't know exactly what the code should do, so I cannot fix this.

[Berend-Jan Wever]

Note to self on variations and ids:
chrome.dll!WebCore::lastPositionInNode ReadAV@NULL (23c8d35ca79c9994c6691ce3fa381d59) # move/extend forward paragraph
chrome.dll!WebCore::lastPositionInNode ReadAV@NULL (72514a93116f979bdda8b1f03984c440) # move forward line
chrome.dll!WebCore::lastPositionInNode ReadAV@NULL (dfca1ef539706bcaffa24f0035dceab4) # extend forward line

[Ryosuke Niwa]

Created attachment 85201 [details]
fixes the crash

[Darin Adler]

Comment on attachment 85201 [details]
fixes the crash

Seems like a null dereference and hence not security-sensitive.

[Eric Seidel (no email)]

Comment on attachment 85201 [details]
fixes the crash

OK.  Should we do previousLinePosition too?

[Ryosuke Niwa]

Wow! That was quick.

(In reply to comment #4)
> (From update of attachment 85201 [details])
> Seems like a null dereference and hence not security-sensitive.

Yes, I don't think this is a security vulnerability.

(In reply to comment #5)
> (From update of attachment 85201 [details])
> OK.  Should we do previousLinePosition too?

I can't reproduce the crash when I do backwards but you're right that we should probably fix there as well.  Should we include in this patch without a test?

[Ryosuke Niwa]

Created attachment 85204 [details]
Also fixes previousLinePosition

[Ryosuke Niwa]

Comment on attachment 85201 [details]
fixes the crash

(In reply to comment #6)
> (In reply to comment #5)
> > (From update of attachment 85201 [details] [details])
> > OK.  Should we do previousLinePosition too?
> 
> I can't reproduce the crash when I do backwards but you're right that we should probably fix there as well.  Should we include in this patch without a test?

I think we should given the symmetry of these two functions.

[Ryosuke Niwa]

Thanks for the review, Tony.

[Ryosuke Niwa]

Committed r80658: <http://trac.webkit.org/changeset/80658>