Bug 156809 – Crash under WebCore::TextIterator::subrange()

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

[Chris Dumez]

Crash unde WebCore::TextIterator::subrange():
Thread[0] EXC_BAD_ACCESS (SIGSEGV) (KERN_INVALID_ADDRESS at 0x0000000000000008)
[  0] 0x00007fff9969a57e WebCore`WebCore::TextIterator::subrange(WebCore::Range*, int, int) [inlined] WTF::Ref<WebCore::Document>::get() const at Ref.h:60

     0x00007fff9969a56c:     movq %rbx, %rdi
     0x00007fff9969a56f:    callq 0x1b06d0             ; WebCore::TextIterator::advance at TextIterator.cpp:377
     0x00007fff9969a574:     cmpq $0x0, -0xc8(%rbp)
     0x00007fff9969a57c:      jne 0x2ab560             ; <+96> [inlined] WebCore::CharacterIterator::CharacterIterator(WebCore::Range const&, unsigned short) + 55 at TextIterator.cpp:1399
 ->  0x00007fff9969a57e:     movq 0x8(%r13), %rsi
     0x00007fff9969a582:     leaq -0x128(%rbp), %rbx
     0x00007fff9969a589:     movq %r12, %rdi
     0x00007fff9969a58c:     movq %rbx, %rdx
     0x00007fff9969a58f:     movl %r15d, %ecx

[  0] 0x00007fff9969a57e WebCore`WebCore::TextIterator::subrange(WebCore::Range*, int, int) [inlined] WebCore::Range::ownerDocument() const at Range.h:62
       58  	    static PassRefPtr<Range> create(ScriptExecutionContext&);
       59  	    static PassRefPtr<Range> create(Document&, const VisiblePosition&, const VisiblePosition&);
       60  	    ~Range();
       61  	
    -> 62  	    Document& ownerDocument() const { return const_cast<Document&>(m_ownerDocument.get()); }
       63  	
       64  	    Node* startContainer() const { return m_start.container(); }
       65  	    int startOffset() const { return m_start.offset(); }
       66  	    Node* endContainer() const { return m_end.container(); }
    
[  0] 0x00007fff9969a57e WebCore`WebCore::TextIterator::subrange(WebCore::Range*, int, int) + 126 at TextIterator.cpp:2388
       2384	
       2385	PassRefPtr<Range> TextIterator::subrange(Range* entireRange, int characterOffset, int characterCount)
       2386	{
       2387	    CharacterIterator entireRangeIterator(*entireRange);
    -> 2388	    return characterSubrange(entireRange->ownerDocument(), entireRangeIterator, characterOffset, characterCount);
       2389	}
       2390	
       2391	static inline bool isInsideReplacedElement(TextIterator& iterator)
       2392	{
    
[  1] 0x00007fff9978ac5f WebCore`WebCore::AlternativeTextController::applyAlternativeTextToRange(WebCore::Range const*, WTF::String const&, WebCore::AlternativeTextType, WTF::Vector<WebCore::DocumentMarker::MarkerType, 0ul, WTF::CrashOnOverflow> const&) + 1647 at AlternativeTextController.cpp:281
       277 	    // Recalculate pragraphRangeContainingCorrection, since SpellingCorrectionCommand modified the DOM, such that the original paragraphRangeContainingCorrection is no longer valid. Radar: 10305315 Bugzilla: 89526
       278 	    paragraphRangeContainingCorrection = TextIterator::rangeFromLocationAndLength(&rootNode, paragraphStartIndex, correctionStartOffsetInParagraph + alternative.length());
       279 	    
       280 	    setEnd(paragraphRangeContainingCorrection.get(), m_frame.selection().selection().start());
    -> 281 	    RefPtr<Range> replacementRange = TextIterator::subrange(paragraphRangeContainingCorrection.get(), correctionStartOffsetInParagraph, alternative.length());
       282 	    String newText = plainText(replacementRange.get());
       283 	
       284 	    // Check to see if replacement succeeded.
       285 	    if (newText != alternative)
    
[  2] 0x00007fff9968af38 WebCore`WebCore::AlternativeTextController::handleAlternativeTextUIResult(WTF::String const&) + 632 at AlternativeTextController.cpp:420
[  3] 0x00007fff911aa9d7 WebKitLegacy`-[WebView(WebViewInternal) handleAcceptedAlternativeText:] + 71 at WebView.mm:8542

[Chris Dumez]

rdar://problem/21102730

[Chris Dumez]

Created attachment 276854 [details]
Patch

[Alex Christensen]

Comment on attachment 276854 [details]
Patch

paragraphRangeContainingCorrection is dereferenced before this.  If this is to avoid null pointer dereferencing in this function, I think this is in the wrong place.

[Chris Dumez]

(In reply to comment #3)
> Comment on attachment 276854 [details]
> Patch
> 
> paragraphRangeContainingCorrection is dereferenced before this.  If this is
> to avoid null pointer dereferencing in this function, I think this is in the
> wrong place.

But before this, it is initialized like so:
paragraphRangeContainingCorrection = range->cloneRange();

And range->cloneRange() cannot return null.

This is therefore the right place to null check.

[Chris Dumez]

(In reply to comment #4)
> (In reply to comment #3)
> > Comment on attachment 276854 [details]
> > Patch
> > 
> > paragraphRangeContainingCorrection is dereferenced before this.  If this is
> > to avoid null pointer dereferencing in this function, I think this is in the
> > wrong place.
> 
> But before this, it is initialized like so:
> paragraphRangeContainingCorrection = range->cloneRange();
> 
> And range->cloneRange() cannot return null.
> 
> This is therefore the right place to null check.

As explained in the Changelog, the issue is only with TextIterator::rangeFromLocationAndLength() potentially returning null.

[WebKit Commit Bot]

Comment on attachment 276854 [details]
Patch

Clearing flags on attachment: 276854

Committed r199807: <http://trac.webkit.org/changeset/199807>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.