Bug 224259 - Nullptr dereference in ReplaceSelectionCommand::removeRedundantStylesAndKeepStyleSpanInline
Summary: Nullptr dereference in ReplaceSelectionCommand::removeRedundantStylesAndKeepS...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-04-06 19:34 PDT by Julian Gonzalez
Modified: 2021-04-07 22:21 PDT (History)
5 users (show)

See Also:


Attachments
Patch (6.84 KB, patch)
2021-04-06 19:42 PDT, Julian Gonzalez
no flags Details | Formatted Diff | Diff
Patch (6.50 KB, patch)
2021-04-07 14:04 PDT, Julian Gonzalez
no flags Details | Formatted Diff | Diff
Patch (6.54 KB, patch)
2021-04-07 17:44 PDT, Julian Gonzalez
no flags Details | Formatted Diff | Diff
Patch (6.48 KB, patch)
2021-04-07 20:42 PDT, Julian Gonzalez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Julian Gonzalez 2021-04-06 19:34:47 PDT
e.g.

    #0 0x114f9e976 in WebCore::ReplaceSelectionCommand::removeRedundantStylesAndKeepStyleSpanInline(WebCore::ReplaceSelectionCommand::InsertedNodes&)
    #1 0x114faf0c0 in WebCore::ReplaceSelectionCommand::doApply()
    #2 0x114d83d80 in WebCore::CompositeEditCommand::apply()
    #3 0x114f2b7ab in WebCore::executeInsertFragment(WebCore::Frame&, WTF::Ref<WebCore::DocumentFragment, WTF::RawPtrTraits<WebCore::DocumentFragment> >&&)
    #4 0x114f207d5 in WebCore::executeInsertHTML(WebCore::Frame&, WebCore::Event*, WebCore::EditorCommandSource, WTF::String const&)
    #5 0x1149f779e in WebCore::Document::execCommand(WTF::String const&, bool, WTF::String const&)

<rdar://problem/76003461>
Comment 1 Julian Gonzalez 2021-04-06 19:42:33 PDT
Created attachment 425343 [details]
Patch
Comment 2 Ryosuke Niwa 2021-04-06 19:51:23 PDT
Comment on attachment 425343 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=425343&action=review

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:402
> +inline void ReplaceSelectionCommand::InsertedNodes::willRemovePossibleAncestorNode(Node *node)

Nit: * should be on the right of Node, not on the left of node.

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:409
> +        m_lastNodeInserted = nullptr;
> +    } else {

We should just add an early exit here.

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:415
> +        // Check if the first and last nodes have swapped relative positions.

I don't think we need to have this comment.
It's pretty obvious what we're doing from the code below.

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:420
> +        else if (m_firstNodeInserted != m_lastNodeInserted && m_lastNodeInserted->contains(m_firstNodeInserted.get())) {

This simply m_firstNodeInserted->isDescendantOf(*m_lastNodeInserted)

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:421
> +            // Now first == last, and last == first. Swap the pointers.

This is a confusing comment. If we used std::swap below, it would be trivially obvious.

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:422
> +            ASSERT(!m_firstNodeInserted->contains(m_lastNodeInserted.get()));

This assertion is unnecessary if we used isDescendantOf because it would be trivially true.

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:425
> +            auto temp = m_firstNodeInserted;
> +            m_firstNodeInserted = m_lastNodeInserted;
> +            m_lastNodeInserted = temp;

Why not just std::swap(~)?

> LayoutTests/editing/inserting/insert-display-contents-crash.html:23
> +    document.execCommand('InsertHTML', false, '<h1>PASS');

Can we add some description that this test passes if we don't crash?
Comment 3 Julian Gonzalez 2021-04-07 11:18:41 PDT
(In reply to Ryosuke Niwa from comment #2)
> Comment on attachment 425343 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=425343&action=review
> 
> > Source/WebCore/editing/ReplaceSelectionCommand.cpp:402
> > +inline void ReplaceSelectionCommand::InsertedNodes::willRemovePossibleAncestorNode(Node *node)
> 
> Nit: * should be on the right of Node, not on the left of node.
> 

This is matching willRemoveNodePreservingChildren() - that seems wrong too. What's the policy on fixing style nits in code that would not otherwise be part of a patch?


> > Source/WebCore/editing/ReplaceSelectionCommand.cpp:409
> > +        m_lastNodeInserted = nullptr;
> > +    } else {
> 
> We should just add an early exit here.

Agreed.

> 
> > Source/WebCore/editing/ReplaceSelectionCommand.cpp:415
> > +        // Check if the first and last nodes have swapped relative positions.
> 
> I don't think we need to have this comment.
> It's pretty obvious what we're doing from the code below.
> 

It doesn't feel obvious to me, but I'll remove it.

> > Source/WebCore/editing/ReplaceSelectionCommand.cpp:420
> > +        else if (m_firstNodeInserted != m_lastNodeInserted && m_lastNodeInserted->contains(m_firstNodeInserted.get())) {
> 
> This simply m_firstNodeInserted->isDescendantOf(*m_lastNodeInserted)

Thanks, that's definitely an improvement.

> 
> > Source/WebCore/editing/ReplaceSelectionCommand.cpp:421
> > +            // Now first == last, and last == first. Swap the pointers.
> 
> This is a confusing comment. If we used std::swap below, it would be
> trivially obvious.

I'll use std::swap.

> 
> > Source/WebCore/editing/ReplaceSelectionCommand.cpp:422
> > +            ASSERT(!m_firstNodeInserted->contains(m_lastNodeInserted.get()));
> 
> This assertion is unnecessary if we used isDescendantOf because it would be
> trivially true.

I'll use it here too.

> 
> > Source/WebCore/editing/ReplaceSelectionCommand.cpp:425
> > +            auto temp = m_firstNodeInserted;
> > +            m_firstNodeInserted = m_lastNodeInserted;
> > +            m_lastNodeInserted = temp;
> 
> Why not just std::swap(~)?

You made that point already :)

> 
> > LayoutTests/editing/inserting/insert-display-contents-crash.html:23
> > +    document.execCommand('InsertHTML', false, '<h1>PASS');
> 
> Can we add some description that this test passes if we don't crash?

This test does not crash if you add a body (or doctype, even). I can add an HTML comment.
Comment 4 Julian Gonzalez 2021-04-07 12:45:35 PDT
(In reply to Julian Gonzalez from comment #3)
> (In reply to Ryosuke Niwa from comment #2)
> > Comment on attachment 425343 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=425343&action=review
> > 
> > > Source/WebCore/editing/ReplaceSelectionCommand.cpp:402
> > > +inline void ReplaceSelectionCommand::InsertedNodes::willRemovePossibleAncestorNode(Node *node)
> > 
> > Nit: * should be on the right of Node, not on the left of node.
> > 
> 
> This is matching willRemoveNodePreservingChildren() - that seems wrong too.
> What's the policy on fixing style nits in code that would not otherwise be
> part of a patch?

Ah, I think that was a typo I introduced, never mind.
> 
> 
> > > Source/WebCore/editing/ReplaceSelectionCommand.cpp:409
> > > +        m_lastNodeInserted = nullptr;
> > > +    } else {
> > 
> > We should just add an early exit here.
> 
> Agreed.
> 
> > 
> > > Source/WebCore/editing/ReplaceSelectionCommand.cpp:415
> > > +        // Check if the first and last nodes have swapped relative positions.
> > 
> > I don't think we need to have this comment.
> > It's pretty obvious what we're doing from the code below.
> > 
> 
> It doesn't feel obvious to me, but I'll remove it.
> 
> > > Source/WebCore/editing/ReplaceSelectionCommand.cpp:420
> > > +        else if (m_firstNodeInserted != m_lastNodeInserted && m_lastNodeInserted->contains(m_firstNodeInserted.get())) {
> > 
> > This simply m_firstNodeInserted->isDescendantOf(*m_lastNodeInserted)
> 
> Thanks, that's definitely an improvement.
> 
> > 
> > > Source/WebCore/editing/ReplaceSelectionCommand.cpp:421
> > > +            // Now first == last, and last == first. Swap the pointers.
> > 
> > This is a confusing comment. If we used std::swap below, it would be
> > trivially obvious.
> 
> I'll use std::swap.
> 
> > 
> > > Source/WebCore/editing/ReplaceSelectionCommand.cpp:422
> > > +            ASSERT(!m_firstNodeInserted->contains(m_lastNodeInserted.get()));
> > 
> > This assertion is unnecessary if we used isDescendantOf because it would be
> > trivially true.
> 
> I'll use it here too.
> 
> > 
> > > Source/WebCore/editing/ReplaceSelectionCommand.cpp:425
> > > +            auto temp = m_firstNodeInserted;
> > > +            m_firstNodeInserted = m_lastNodeInserted;
> > > +            m_lastNodeInserted = temp;
> > 
> > Why not just std::swap(~)?
> 
> You made that point already :)
> 
> > 
> > > LayoutTests/editing/inserting/insert-display-contents-crash.html:23
> > > +    document.execCommand('InsertHTML', false, '<h1>PASS');
> > 
> > Can we add some description that this test passes if we don't crash?
> 
> This test does not crash if you add a body (or doctype, even). I can add an
> HTML comment.
Comment 5 Julian Gonzalez 2021-04-07 14:04:31 PDT
Created attachment 425435 [details]
Patch
Comment 6 Ryosuke Niwa 2021-04-07 16:26:39 PDT
Comment on attachment 425435 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=425435&action=review

> LayoutTests/editing/inserting/insert-display-contents-crash.html:23
> +    document.execCommand('InsertHTML', false, '<h1>PASS');

Oh I mean that here, can do:
document.execCommand('InsertHTML', false, '<h1>This test passes if WebKit does not crash. PASS');
Comment 7 Julian Gonzalez 2021-04-07 17:44:26 PDT
Created attachment 425464 [details]
Patch
Comment 8 Darin Adler 2021-04-07 20:07:17 PDT
Comment on attachment 425464 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=425464&action=review

Our new code has many branches. Our new test does not cover them. I think we need more tests to cover all the cases that require different logic.

> Source/WebCore/editing/ReplaceSelectionCommand.cpp:428
> +    ASSERT(!m_firstNodeInserted || m_firstNodeInserted == node || !node->contains(m_firstNodeInserted.get()));
> +    ASSERT(!m_lastNodeInserted || m_lastNodeInserted == node || !node->contains(m_lastNodeInserted.get()));

Seems like we should consider "!isDescendantOf" here to shorten these assertions:

    ASSERT(!m_firstNodeInserted || !m_firstNodeInserted->isDescendantOf(node));
    ASSERT(!m_lastNodeInserted || !m_lastNodeInserted->isDescendantOf(node));

Would be even better if we had a non-member version of isDescendantOf that understands that null isn’t a descendant of anything.
Comment 9 Julian Gonzalez 2021-04-07 20:42:44 PDT
Created attachment 425474 [details]
Patch
Comment 10 EWS 2021-04-07 22:21:15 PDT
Committed r275652: <https://commits.webkit.org/r275652>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 425474 [details].