1345015 - Crash [@mozilla::HTMLEditRules::GetPromotedPoint]

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect, P2)

Description

[Jason Kratzer [:jkratzer]]

Attachment: Testcase

Testcase found while fuzzing mozilla-central rev 20170302-d29f84406483.

ASAN:DEADLYSIGNAL
=================================================================
==18961==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7f0639e42d2d bp 0x7fff4e196440 sp 0x7fff4e196100 T0)
    #0 0x7f0639e42d2c in mozilla::HTMLEditRules::GetPromotedPoint(mozilla::HTMLEditRules::RulesEndpoint, nsIDOMNode*, int, EditAction, nsCOMPtr<nsIDOMNode>*, int*) /home/worker/workspace/build/src/editor/libeditor/HTMLEditRules.cpp:5422:16
    #1 0x7f0639dd6875 in mozilla::HTMLEditRules::PromoteRange(nsRange&, EditAction) /home/worker/workspace/build/src/editor/libeditor/HTMLEditRules.cpp:5657:3
    #2 0x7f0639dd566a in mozilla::HTMLEditRules::AfterEditInner(EditAction, short) /home/worker/workspace/build/src/editor/libeditor/HTMLEditRules.cpp:457:5
    #3 0x7f0639dd4e56 in mozilla::HTMLEditRules::AfterEdit(EditAction, short) /home/worker/workspace/build/src/editor/libeditor/HTMLEditRules.cpp:400:10
    #4 0x7f0639e7e6bb in mozilla::HTMLEditor::EndOperation() /home/worker/workspace/build/src/editor/libeditor/HTMLEditor.cpp:3515:25
    #5 0x7f0639f06b02 in ~AutoRules /home/worker/workspace/build/src/obj-firefox/dist/include/mozilla/EditorUtils.h:251:7
    #6 0x7f0639f06b02 in mozilla::TextEditor::InsertLineBreak() /home/worker/workspace/build/src/editor/libeditor/TextEditor.cpp:761
    #7 0x7f0639f03ccb in mozilla::TextEditor::TypedText(nsAString_internal const&, mozilla::TextEditor::ETypingAction) /home/worker/workspace/build/src/editor/libeditor/TextEditor.cpp:415:14
    #8 0x7f0639e5d2a1 in mozilla::HTMLEditor::TypedText(nsAString_internal const&, mozilla::TextEditor::ETypingAction) /home/worker/workspace/build/src/editor/libeditor/HTMLEditor.cpp:1013:10
    #9 0x7f0639da5820 in mozilla::InsertParagraphCommand::DoCommand(char const*, nsISupports*) /home/worker/workspace/build/src/editor/libeditor/EditorCommands.cpp:1025:10
    #10 0x7f06380ec3c6 in nsControllerCommandTable::DoCommand(char const*, nsISupports*) /home/worker/workspace/build/src/dom/commandhandler/nsControllerCommandTable.cpp:147:10
    #11 0x7f06380e2d8a in nsBaseCommandController::DoCommand(char const*) /home/worker/workspace/build/src/dom/commandhandler/nsBaseCommandController.cpp:136:10
    #12 0x7f06380e96d6 in nsCommandManager::DoCommand(char const*, nsICommandParams*, mozIDOMWindowProxy*) /home/worker/workspace/build/src/dom/commandhandler/nsCommandManager.cpp:214:10
    #13 0x7f06386a1bb8 in nsHTMLDocument::ExecCommand(nsAString_internal const&, bool, nsAString_internal const&, nsIPrincipal&, mozilla::ErrorResult&) /home/worker/workspace/build/src/dom/html/nsHTMLDocument.cpp:3239:10
    #14 0x7f0637b4fb6d in mozilla::dom::HTMLDocumentBinding::execCommand(JSContext*, JS::Handle<JSObject*>, nsHTMLDocument*, JSJitMethodCallArgs const&) /home/worker/workspace/build/src/obj-firefox/dom/bindings/HTMLDocumentBinding.cpp:835:15
    #15 0x7f0637dfc387 in mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*) /home/worker/workspace/build/src/dom/bindings/BindingUtils.cpp:2953:13

Comment 1

[Makoto Kato [:m_kato]]

Attachment: Bug 1345015 - Part 1. Clean up GetPromotedPoint.

When start or node of current selection has no nsIContent, insertText will crash by GetPromotedPoint.
So we should check nullptr.

Review commit: https://reviewboard.mozilla.org/r/117848/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/117848/

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(still sick)]

Comment on attachment 8844358 [details]
Bug 1345015 - Part 1. Clean up GetPromotedPoint.

https://reviewboard.mozilla.org/r/117848/#review119598

::: editor/libeditor/HTMLEditRules.cpp:5422
(Diff revision 1)
> +    if (content) {
> -    *outNode = content->AsDOMNode();
> +      *outNode = content->AsDOMNode();
> +    }

Although, I think that this must be necessary change, I don't think that this is enough to fix this bug.

Currently, Selection.addRange() ignores selection limiter. Therefore, the testcase might be able to select all nodes in the document with:

> var range = document.createRange();
> range.selectNode(documetn.documentElement);
> document.getSelection().addRange(range);

However, this selected range should not be replaced with anything.  So, before handling input, we should shrink the selected range into root element (typically, document.body <https://dxr.mozilla.org/mozilla-central/rev/b7e42143bbbc9dc3e5c05bd1e93b6485ce1d0ad4/editor/libeditor/HTMLEditor.cpp#362,365-366,381>).

So, I think that |HTMLEditRules::PromoteRange()| should check the selected range is in proper node. I think that you should shrink the range if start and/or end node is out of the root element.

If you want to uplift this as far as safely, I'll give r+ for this patch with some fix of the testcase.  Otherwise, could you try to fix it? I think that even with your patch, document.execCommand() in the testcause does nothing (or replacing the <body> with <p> or something).

::: editor/libeditor/crashtests/1345015.html:2
(Diff revision 1)
> +<html>
> +    <head>

Please use 2 spaces for indent.

::: editor/libeditor/crashtests/1345015.html:5
(Diff revision 1)
> +            o1 = window.getSelection();
> +            o2 = document.createRange();
> +            o3 = document.createRange();
> +            o4 = document.createElement('div');
> +            o5 = document.createElement('div');
> +            document.documentElement.appendChild(o4);
> +            document.documentElement.appendChild(o5);
> +            o4.outerHTML = '<foo>';
> +            o3.selectNode(o5);
> +            o1.addRange(o3);
> +            o3.setStart(o4, 0);
> +            o2.selectNode(document.documentElement);
> +            o1.addRange(o2);

Hmm, I'd like you to give them better name with |var|.

So, I think:

o1: selection
o2: range1
o3: range3
o4: div1
o5: div2

::: editor/libeditor/crashtests/1345015.html:12
(Diff revision 1)
> +            o3 = document.createRange();
> +            o4 = document.createElement('div');
> +            o5 = document.createElement('div');
> +            document.documentElement.appendChild(o4);
> +            document.documentElement.appendChild(o5);
> +            o4.outerHTML = '<foo>';

o4 is really necessary to reproduce this crash?

::: editor/libeditor/crashtests/1345015.html:13
(Diff revision 1)
> +            o3.selectNode(o5);
> +            o1.addRange(o3);
> +            o3.setStart(o4, 0);

o3 (range selecting the second <div>) is really necessary for reproducing this crash?

::: editor/libeditor/crashtests/1345015.html:16
(Diff revision 1)
> +            o2.selectNode(document.documentElement);
> +            o1.addRange(o2);

So, selected range is now, the entire of the document tree.

Comment 4

[Makoto Kato [:m_kato]]

Comment on attachment 8844358 [details]
Bug 1345015 - Part 1. Clean up GetPromotedPoint.

https://reviewboard.mozilla.org/r/117848/#review119598

> Although, I think that this must be necessary change, I don't think that this is enough to fix this bug.
> 
> Currently, Selection.addRange() ignores selection limiter. Therefore, the testcase might be able to select all nodes in the document with:
> 
> > var range = document.createRange();
> > range.selectNode(documetn.documentElement);
> > document.getSelection().addRange(range);
> 
> However, this selected range should not be replaced with anything.  So, before handling input, we should shrink the selected range into root element (typically, document.body <https://dxr.mozilla.org/mozilla-central/rev/b7e42143bbbc9dc3e5c05bd1e93b6485ce1d0ad4/editor/libeditor/HTMLEditor.cpp#362,365-366,381>).
> 
> So, I think that |HTMLEditRules::PromoteRange()| should check the selected range is in proper node. I think that you should shrink the range if start and/or end node is out of the root element.
> 
> If you want to uplift this as far as safely, I'll give r+ for this patch with some fix of the testcase.  Otherwise, could you try to fix it? I think that even with your patch, document.execCommand() in the testcause does nothing (or replacing the <body> with <p> or something).

OK, crash data isn't large volumes, so I change fix.

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(still sick)]

Comment on attachment 8844358 [details]
Bug 1345015 - Part 1. Clean up GetPromotedPoint.

https://reviewboard.mozilla.org/r/117848/#review134740

r- until coming the new patch.

Comment 6

[Bob Clary [:bc] (inactive)]

dupe of bug 1267757 ?

Comment 7

[Makoto Kato [:m_kato]]

Attachment: Bug 1345015 - Part 2. Don't promote range when selection node isn't content.

When using EditAction::insertBreak action, if selection node isn't content, this crash occurs.  So we should reject to promote range when it isn't content node.

Review commit: https://reviewboard.mozilla.org/r/150346/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/150346/

Comment 8

[Makoto Kato [:m_kato]]

Attachment: Bug 1345015 - Part 3. Add crash test.

Add minimal crash test for this.  When removing a line, this doesn't occur.

Review commit: https://reviewboard.mozilla.org/r/150348/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/150348/

Comment 9

[Makoto Kato [:m_kato]]

Comment on attachment 8844358 [details]
Bug 1345015 - Part 1. Clean up GetPromotedPoint.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/117848/diff/1-2/

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(still sick)]

Comment on attachment 8879052 [details]
Bug 1345015 - Part 3. Add crash test.

https://reviewboard.mozilla.org/r/150348/#review155086

::: editor/libeditor/crashtests/1345015.html:4
(Diff revision 1)
> +    <script type="application/javascript">
> +       document.designMode = "on";
> +       let selection = window.getSelection();
> +
> +       let foo = document.createElement('foo');
> +       let div = document.createElement('div');
> +       document.documentElement.appendChild(foo);
> +       document.documentElement.appendChild(div);
> +       foo.outerHTML = '<foo>';
> +
> +       let range = document.createRange();
> +       range.selectNode(div);
> +       selection.addRange(range);
> +       range.setStart(foo, 0);
> +
> +       range = document.createRange();
> +       range.selectNode(document.documentElement);
> +       selection.addRange(range);
> +
> +       document.execCommand('insertparagraph', false, null);
> +    </script>

nit: you use *4* spaces for indent only in <script>

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(still sick)]

Comment on attachment 8844358 [details]
Bug 1345015 - Part 1. Clean up GetPromotedPoint.

https://reviewboard.mozilla.org/r/117848/#review155096

::: editor/libeditor/HTMLEditRules.cpp:5457
(Diff revision 2)
>    if (actionID == EditAction::insertText ||
>        actionID == EditAction::insertIMEText ||
>        actionID == EditAction::insertBreak ||
>        actionID == EditAction::deleteText) {
>      bool isSpace, isNBSP;
> -    nsCOMPtr<nsIContent> content = do_QueryInterface(node), temp;
> +    nsCOMPtr<nsIContent> content = aNode.AsContent();

nsCOMPtr<nsIContent> content = aNode.IsContent() ? aNode.AsContent() : nullptr;

# AsContent() assumes that the caller guarantees that the node always has nsIContent.

::: editor/libeditor/HTMLEditRules.cpp:5496
(Diff revision 2)
>    if (aWhere == kStart) {
>      // some special casing for text nodes
>      if (node->IsNodeOfType(nsINode::eTEXT)) {
>        if (!node->GetParentNode()) {
>          // Okay, can't promote any further
> -        return;
> +        return EditorDOMPoint(&aNode, aOffset);

Please use node and offset here because this code is really explicit what this return now.

However, node and offset may be modified before here in the future. Then, this code could cause unexpected bug (i.e., the developer may not realize this "solid" return statement).

::: editor/libeditor/HTMLEditRules.cpp:5560
(Diff revision 2)
>    // aWhere == kEnd
>    // some special casing for text nodes
>    if (node->IsNodeOfType(nsINode::eTEXT)) {
>      if (!node->GetParentNode()) {
>        // Okay, can't promote any further
> -      return;
> +      return EditorDOMPoint(&aNode, aOffset);

Same. Please use node and offset here.

::: editor/libeditor/HTMLEditRules.cpp:5696
(Diff revision 2)
> -  nsCOMPtr<nsIDOMNode> opDOMStartNode;
> -  nsCOMPtr<nsIDOMNode> opDOMEndNode;
> -  int32_t opStartOffset, opEndOffset;
> -
> +  EditorDOMPoint opStart =
> +    GetPromotedPoint(kStart, *startNode, startOffset, aOperationType);
> +  EditorDOMPoint opEnd =
> +    GetPromotedPoint(kEnd, *endNode, endOffset, aOperationType);

Hmm, startNode and endNode are not checked if they are nullptr. In here, both of them can be nullptr. So, could you check aRange.IsPositioned() above? If it's not positioned, this should (can) do nothing.

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(still sick)]

Comment on attachment 8879051 [details]
Bug 1345015 - Part 2. Don't promote range when selection node isn't content.

https://reviewboard.mozilla.org/r/150346/#review155104

::: editor/libeditor/HTMLEditRules.cpp:5692
(Diff revision 1)
> +  if (aOperationType == EditAction::insertText ||
> +      aOperationType == EditAction::insertIMEText ||
> +      aOperationType == EditAction::insertBreak ||
> +      aOperationType == EditAction::deleteText) {
> +     if (NS_WARN_IF(!startNode->IsContent()) ||
> +         NS_WARN_IF(!endNode->IsContent())) {
> +       return;
> +     }
> +  }

Sorry, I don't understand this change. Could you explain why do we need to check the edit action or remove it if it does not make sense.

(It's okay if you are not sure whether they are the all what should be handled specially. Even if so, I'd like you to add explanation here because others (including me) will be confused whether they can add other edit action into this condition. So, "not sure" comment is helpful in such case.)

::: editor/libeditor/HTMLEditRules.cpp:5696
(Diff revision 1)
> +     if (NS_WARN_IF(!startNode->IsContent()) ||
> +         NS_WARN_IF(!endNode->IsContent())) {

I don't think that we should output warnings here because I assume that this case occurs with your automated test. So, you should add comment here like that startNode and/or endNode can be a document node.

Comment 13

[Makoto Kato [:m_kato]]

Comment on attachment 8844358 [details]
Bug 1345015 - Part 1. Clean up GetPromotedPoint.

https://reviewboard.mozilla.org/r/117848/#review155096

> nsCOMPtr<nsIContent> content = aNode.IsContent() ? aNode.AsContent() : nullptr;
> 
> # AsContent() assumes that the caller guarantees that the node always has nsIContent.

This root cause of this issue is that HTMLEditRules::GetPromotedPoint doesn't support non-content on text transaction such as insertText etc.  If non-content node, it returns null.  So I would like to use AsContent for assertion check.  If callee passes non-content node for text transaction, we should detect it.

HTMLEditRules::GetPromotedPoint shouldn't return null node.

> Hmm, startNode and endNode are not checked if they are nullptr. In here, both of them can be nullptr. So, could you check aRange.IsPositioned() above? If it's not positioned, this should (can) do nothing.

OK.

Comment 14

[Makoto Kato [:m_kato]]

s/If callee passes non-content node/If caller passes non-content node/

Also, so, I added simple null check by previous fix, but you suggest that we should check caller by previous review

Comment 15

[Makoto Kato [:m_kato]]

Comment on attachment 8844358 [details]
Bug 1345015 - Part 1. Clean up GetPromotedPoint.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/117848/diff/2-3/

Comment 16

[Makoto Kato [:m_kato]]

Comment on attachment 8879051 [details]
Bug 1345015 - Part 2. Don't promote range when selection node isn't content.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/150346/diff/1-2/

Comment 17

[Makoto Kato [:m_kato]]

Comment on attachment 8879052 [details]
Bug 1345015 - Part 3. Add crash test.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/150348/diff/1-2/

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7a792bc76e13
Part 1. Clean up GetPromotedPoint. r=masayuki
https://hg.mozilla.org/integration/autoland/rev/99b10a5f5a3c
Part 2. Don't promote range when selection node isn't content. r=masayuki
https://hg.mozilla.org/integration/autoland/rev/8012d7f5aede
Part 3. Add crash test. r=masayuki

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7a792bc76e13
https://hg.mozilla.org/mozilla-central/rev/99b10a5f5a3c
https://hg.mozilla.org/mozilla-central/rev/8012d7f5aede

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Is this something we should consider for backport or can it ride the 56 train?

Comment 21

[Makoto Kato [:m_kato]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #20)
> Is this something we should consider for backport or can it ride the 56
> train?

There is a few crash data (It may be that all crashes are my test only) by this.  So I don't think that this should be uplifted.