1364133 - Crash [@ mozilla::HTMLEditor::SetInlinePropertyOnTextNode]

Status: RESOLVED FIXED

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

Description

[Jason Kratzer [:jkratzer]]

Attachment: Testcase

Testcase found while fuzzing mozilla-central rev 20170510-d8762cb96742.

=================================================================
==25903==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7f47a70b4bf8 bp 0x7ffd75a6df80 sp 0x7ffd75a6de60 T0)
==25903==The signal is caused by a READ memory access.
==25903==Hint: address points to the zero page.
    #0 0x7f47a70b4bf7 in GetAsText /home/worker/workspace/build/src/obj-firefox/dist/include/mozilla/dom/Text.h:46:10
    #1 0x7f47a70b4bf7 in mozilla::HTMLEditor::SetInlinePropertyOnTextNode(mozilla::dom::Text&, int, int, nsIAtom&, nsAString const*, nsAString const&) /home/worker/workspace/build/src/editor/libeditor/HTMLStyleEditor.cpp:335
    #2 0x7f47a7075be0 in mozilla::HTMLEditor::SetInlineProperty(nsIAtom*, nsAString const&, nsAString const&) /home/worker/workspace/build/src/editor/libeditor/HTMLStyleEditor.cpp:210:14
    #3 0x7f47a713b142 in nsFontColorStateCommand::SetState(nsIEditor*, nsString&) /home/worker/workspace/build/src/editor/composer/nsComposerCommands.cpp:812:22
    #4 0x7f47a7138081 in nsMultiStateCommand::DoCommandParams(char const*, nsICommandParams*, nsISupports*) /home/worker/workspace/build/src/editor/composer/nsComposerCommands.cpp:595:12
    #5 0x7f47a539de83 in nsControllerCommandTable::DoCommandParams(char const*, nsICommandParams*, nsISupports*) /home/worker/workspace/build/src/dom/commandhandler/nsControllerCommandTable.cpp:162:26
    #6 0x7f47a53956b3 in DoCommandWithParams /home/worker/workspace/build/src/dom/commandhandler/nsBaseCommandController.cpp:152:25
    #7 0x7f47a53956b3 in non-virtual thunk to nsBaseCommandController::DoCommandWithParams(char const*, nsICommandParams*) /home/worker/workspace/build/src/dom/commandhandler/nsBaseCommandController.cpp:140
    #8 0x7f47a539b2bb in nsCommandManager::DoCommand(char const*, nsICommandParams*, mozIDOMWindowProxy*) /home/worker/workspace/build/src/dom/commandhandler/nsCommandManager.cpp:212:29
    #9 0x7f47a58d49e5 in nsHTMLDocument::ExecCommand(nsAString const&, bool, nsAString const&, nsIPrincipal&, mozilla::ErrorResult&) /home/worker/workspace/build/src/dom/html/nsHTMLDocument.cpp:3346:18
    #10 0x7f47a4e1be1c in mozilla::dom::HTMLDocumentBinding::execCommand(JSContext*, JS::Handle<JSObject*>, nsHTMLDocument*, JSJitMethodCallArgs const&) /home/worker/workspace/build/src/obj-firefox/dom/bindings/HTMLDocumentBinding.cpp:835:21
    #11 0x7f47a50e4dfe in mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*) /home/worker/workspace/build/src/dom/bindings/BindingUtils.cpp:2954:13
    #12 0x7f47aa8f53c3 in CallJSNative /home/worker/workspace/build/src/js/src/jscntxtinlines.h:293:15
    #13 0x7f47aa8f53c3 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:470
    #14 0x7f47aa8ddf6f in CallFromStack /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:521:12
    #15 0x7f47aa8ddf6f in Interpret(JSContext*, js::RunState&) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:3025

Comment 1

[Makoto Kato [:m_kato]]

Attachment: Bug 1364133 - Part 1. Check whether SplitNode returns error.

When SplitNode returns nullptr, GetAsText causes crash.  So we should check
error before casting by GetAsText.

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

Comment 2

[Makoto Kato [:m_kato]]

Attachment: Bug 1364133 - Part 2. Add test.

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

Comment 3

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

Comment on attachment 8917315 [details]
Bug 1364133 - Part 1. Check whether SplitNode returns error.

https://reviewboard.mozilla.org/r/188304/#review193562

::: editor/libeditor/HTMLStyleEditor.cpp:293
(Diff revision 1)
> -    text = SplitNode(aText, aEndOffset, rv)->GetAsText();
> +    text = SplitNode(*text, aEndOffset, rv);
>      NS_ENSURE_TRUE(!rv.Failed(), rv.StealNSResult());
>    }

However, you're patch doesn't check if the result is nullptr nor a Text. If it's not necessary, r+, otherwise, please fix add the check

Comment 4

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

Comment on attachment 8917316 [details]
Bug 1364133 - Part 2. Add test.

https://reviewboard.mozilla.org/r/188306/#review193564

Comment 5

[Makoto Kato [:m_kato]]

> However, you're patch doesn't check if the result is nullptr nor a Text. If it's not necessary, r+, otherwise, please fix add the check

unnecessary.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/92d2ebea6fbc
Part 1. Check whether SplitNode returns error. r=masayuki
https://hg.mozilla.org/integration/autoland/rev/095f6f26bbce
Part 2. Add test. r=masayuki

Comment 7

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/92d2ebea6fbc
https://hg.mozilla.org/mozilla-central/rev/095f6f26bbce