Bug 25335 – crash in VisibleSelection::appendTrailingWhitespace()

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Eric Roman]

This crash comes from <http://code.google.com/p/chromium/issues/detail?id=10645>

To hit it, must have EditorClient::isSelectTrailingWhitespaceEnabled() set to return true (therefore this affects chromium, but not safari).

To reproduce, load an image url, and double click in the whitespace around the image -- CRASH.

[Eric Roman]

The crash happens in VisibleSelection::appendTrailingWhitespace():

    CharacterIterator charIt(searchRange.get(), true);

    for (; charIt.length(); charIt.advance(1)) {
        UChar c = charIt.characters()[0]; // <====== CRASH HERE
        ...
    }

The problem is that charIt.length() is not a sufficient guard -- if CharacterIterator is constructed with a particular empty Range, then TextIterator::m_textLength remains uninitialized after CharacterIterator is constructed.

My proposed fix is to add a check for charIt.atEnd() in the loop:

-    for (; charIt.length(); charIt.advance(1)) {
+    for (; !charIt.atEnd() && charIt.length(); charIt.advance(1)) {

However, it might be better to instead change TextIterator to initialize m_textLength to 0 to avoid this happening elsewhere. This approach might be more consistent with the comment in <https://bugs.webkit.org/show_bug.cgi?id=23232#c2>:

> There's no need to check atEnd() here. Once you're at the end, length is
> guaranteed to return 0.

[Eric Roman]

Created attachment 29720 [details]
Check that it.atEnd() in addition to it.length() > 0

[Eric Roman]

Created attachment 29721 [details]
Check that it.atEnd() in addition to it.length() > 0

actually add the file to patch this time :)

[Darin Adler]

Comment on attachment 29721 [details]
Check that it.atEnd() in addition to it.length() > 0

This is the wrong fix. The CharacterIterator::length is guaranteed to return 0 when it's at the end, and the fact that it's not is a bug.

The correct fix, as suggested earlier in this report, is to make sure the TextIterator constructors initialize m_textLength to 0 in all code paths.

[Eric Roman]

Created attachment 29722 [details]
Check that it.atEnd() in addition to it.length() > 0

Correct the changelog.

[Eric Roman]

Ok.

Please disregard the v3 patch I just uploaded (it was before I saw your comment).

[Darin Adler]

Comment on attachment 29722 [details]
Check that it.atEnd() in addition to it.length() > 0

review- for same reason as previous.

The bug here is in TextIterator, not VisibleSelection.

[Eric Roman]

Created attachment 29723 [details]
Initialize TextIterator::m_textLength to 0, to avoid accessing un-initialized memory when construction fails.

I threw in initialization of m_textCharacters(0) for good measure, but it isn't strictly needed for this fix.

[Eric Roman]

Created attachment 29724 [details]
Initialize TextIterator::m_textLength to 0, to avoid accessing un-initialized memory when construction fails.

difference from previous version (v4), is fixed LayoutTests/ChangeLog (still getting the hang of it).

[Dimitri Glazkov (Google)]

Landed as http://trac.webkit.org/changeset/42831.