Summary: | crash in VisibleSelection::appendTrailingWhitespace() | ||
---|---|---|---|
Product: | WebKit | Reporter: | Eric Roman <eroman> |
Component: | WebCore Misc. | Assignee: | Eric Roman <eroman> |
Status: | RESOLVED FIXED | ||
Severity: | Normal | ||
Priority: | P2 | ||
Version: | 528+ (Nightly build) | ||
Hardware: | PC | ||
OS: | OS X 10.5 | ||
Attachments: |
Description
Eric Roman
2009-04-22 16:03:24 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. Created attachment 29720 [details]
Check that it.atEnd() in addition to it.length() > 0
Created attachment 29721 [details]
Check that it.atEnd() in addition to it.length() > 0
actually add the file to patch this time :)
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.
Created attachment 29722 [details]
Check that it.atEnd() in addition to it.length() > 0
Correct the changelog.
Ok. Please disregard the v3 patch I just uploaded (it was before I saw your comment). 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.
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.
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).
Landed as http://trac.webkit.org/changeset/42831. |