Bug 25335 - crash in VisibleSelection::appendTrailingWhitespace()
Summary: crash in VisibleSelection::appendTrailingWhitespace()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Eric Roman
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-04-22 16:03 PDT by Eric Roman
Modified: 2009-04-24 11:40 PDT (History)
0 users

See Also:


Attachments
Check that it.atEnd() in addition to it.length() > 0 (2.54 KB, patch)
2009-04-23 17:25 PDT, Eric Roman
no flags Details | Formatted Diff | Diff
Check that it.atEnd() in addition to it.length() > 0 (3.11 KB, patch)
2009-04-23 17:28 PDT, Eric Roman
darin: review-
Details | Formatted Diff | Diff
Check that it.atEnd() in addition to it.length() > 0 (3.71 KB, patch)
2009-04-23 17:34 PDT, Eric Roman
darin: review-
Details | Formatted Diff | Diff
Initialize TextIterator::m_textLength to 0, to avoid accessing un-initialized memory when construction fails. (4.52 KB, patch)
2009-04-23 18:02 PDT, Eric Roman
no flags Details | Formatted Diff | Diff
Initialize TextIterator::m_textLength to 0, to avoid accessing un-initialized memory when construction fails. (4.21 KB, patch)
2009-04-23 18:07 PDT, Eric Roman
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Roman 2009-04-22 16:03:24 PDT
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.
Comment 1 Eric Roman 2009-04-22 16:46:32 PDT
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.
Comment 2 Eric Roman 2009-04-23 17:25:33 PDT
Created attachment 29720 [details]
Check that it.atEnd() in addition to it.length() > 0
Comment 3 Eric Roman 2009-04-23 17:28:03 PDT
Created attachment 29721 [details]
Check that it.atEnd() in addition to it.length() > 0

actually add the file to patch this time :)
Comment 4 Darin Adler 2009-04-23 17:33:32 PDT
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.
Comment 5 Eric Roman 2009-04-23 17:34:48 PDT
Created attachment 29722 [details]
Check that it.atEnd() in addition to it.length() > 0

Correct the changelog.
Comment 6 Eric Roman 2009-04-23 17:35:49 PDT
Ok.

Please disregard the v3 patch I just uploaded (it was before I saw your comment).
Comment 7 Darin Adler 2009-04-23 17:35:52 PDT
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.
Comment 8 Eric Roman 2009-04-23 18:02:14 PDT
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.
Comment 9 Eric Roman 2009-04-23 18:07:20 PDT
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).
Comment 10 Dimitri Glazkov (Google) 2009-04-24 11:40:51 PDT
Landed as http://trac.webkit.org/changeset/42831.