1385272 - Crash [@construct<bool, bool>]

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P2)

Description

[Jason Kratzer [:jkratzer]]

Attachment: trigger.html

Testcase found while fuzzing mozilla-central rev 20170726-e8400551c2e3.

ASAN:DEADLYSIGNAL
=================================================================
==32381==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7f90865650ad bp 0x7ffd82d96eb0 sp 0x7ffd82d96e70 T0)
==32381==The signal is caused by a READ memory access.
==32381==Hint: address points to the zero page.
    #0 0x7f90865650ac in construct<bool, bool> /home/worker/workspace/build/src/clang/bin/../lib/gcc/x86_64-unknown-linux-gnu/4.9.4/../../../../include/c++/4.9.4/ext/new_allocator.h:120:4
    #1 0x7f90865650ac in _M_push_back_aux<bool> /home/worker/workspace/build/src/clang/bin/../lib/gcc/x86_64-unknown-linux-gnu/4.9.4/../../../../include/c++/4.9.4/bits/deque.tcc:456
    #2 0x7f90865650ac in emplace_back<bool> /home/worker/workspace/build/src/clang/bin/../lib/gcc/x86_64-unknown-linux-gnu/4.9.4/../../../../include/c++/4.9.4/bits/deque.tcc:142
    #3 0x7f90865650ac in push_back /home/worker/workspace/build/src/clang/bin/../lib/gcc/x86_64-unknown-linux-gnu/4.9.4/../../../../include/c++/4.9.4/bits/stl_deque.h:1413
    #4 0x7f90865650ac in push /home/worker/workspace/build/src/clang/bin/../lib/gcc/x86_64-unknown-linux-gnu/4.9.4/../../../../include/c++/4.9.4/bits/stl_stack.h:195
    #5 0x7f90865650ac in nsPlainTextSerializer::ScanElementForPreformat(mozilla::dom::Element*) /home/worker/workspace/build/src/dom/base/nsPlainTextSerializer.cpp:384
    #6 0x7f908646dfee in nsDocumentEncoder::SerializeNodeStart(nsINode*, int, int, nsAString&, nsINode*) /home/worker/workspace/build/src/dom/base/nsDocumentEncoder.cpp:367:18
    #7 0x7f9086470765 in nsDocumentEncoder::SerializeRangeNodes(nsRange*, nsINode*, nsAString&, int) /home/worker/workspace/build/src/dom/base/nsDocumentEncoder.cpp:816:14
    #8 0x7f9086470a17 in nsDocumentEncoder::SerializeRangeNodes(nsRange*, nsINode*, nsAString&, int) /home/worker/workspace/build/src/dom/base/nsDocumentEncoder.cpp:853:16
    #9 0x7f9086472139 in nsDocumentEncoder::SerializeRangeToString(nsRange*, nsAString&) /home/worker/workspace/build/src/dom/base/nsDocumentEncoder.cpp:993:10
    #10 0x7f9086473a49 in nsDocumentEncoder::EncodeToStringWithMaxLength(unsigned int, nsAString&) /home/worker/workspace/build/src/dom/base/nsDocumentEncoder.cpp:1118:12
    #11 0x7f908639454f in SelectionCopyHelper(nsISelection*, nsIDocument*, bool, short, unsigned int, nsITransferable**) /home/worker/workspace/build/src/dom/base/nsCopySupport.cpp:182:22
    #12 0x7f908a7c0332 in nsAutoCopyListener::NotifySelectionChanged(nsIDOMDocument*, nsISelection*, short) /home/worker/workspace/build/src/layout/generic/nsFrameSelection.cpp:3078:10
    #13 0x7f90862cc9e6 in mozilla::dom::Selection::NotifySelectionListeners() /home/worker/workspace/build/src/dom/base/Selection.cpp:3771:15
    #14 0x7f908a7bc540 in NotifySelectionListeners /home/worker/workspace/build/src/layout/generic/nsFrameSelection.cpp:2067:23
    #15 0x7f908a7bc540 in nsFrameSelection::EndBatchChanges(short) /home/worker/workspace/build/src/layout/generic/nsFrameSelection.cpp:2055
    #16 0x7f90862cd1e6 in mozilla::dom::Selection::EndBatchChangesInternal(short) /home/worker/workspace/build/src/dom/base/Selection.cpp:3799:21
    #17 0x7f90862c85cb in ~SelectionBatcher /home/worker/workspace/build/src/dom/base/Selection.h:487:19
    #18 0x7f90862c85cb in mozilla::dom::Selection::SelectAllChildren(nsINode&, mozilla::ErrorResult&) /home/worker/workspace/build/src/dom/base/Selection.cpp:3192
    #19 0x7f90862c82c1 in mozilla::dom::Selection::SelectAllChildren(nsIDOMNode*) /home/worker/workspace/build/src/dom/base/Selection.cpp:3166:3
    #20 0x7f9089f8715c in mozilla::HTMLEditor::SelectAll() /home/worker/workspace/build/src/editor/libeditor/HTMLEditor.cpp:3637:21
    #21 0x7f9089eca498 in mozilla::SelectAllCommand::DoCommand(char const*, nsISupports*) /home/worker/workspace/build/src/editor/libeditor/EditorCommands.cpp:752:20
    #22 0x7f90881846d5 in nsControllerCommandTable::DoCommand(char const*, nsISupports*) /home/worker/workspace/build/src/dom/commandhandler/nsControllerCommandTable.cpp:147:26
    #23 0x7f908817b0ed in nsBaseCommandController::DoCommand(char const*) /home/worker/workspace/build/src/dom/commandhandler/nsBaseCommandController.cpp:136:25
    #24 0x7f90881819c4 in nsCommandManager::DoCommand(char const*, nsICommandParams*, mozIDOMWindowProxy*) /home/worker/workspace/build/src/dom/commandhandler/nsCommandManager.cpp:212:22
    #25 0x7f90886b59f4 in nsHTMLDocument::ExecCommand(nsAString const&, bool, nsAString const&, nsIPrincipal&, mozilla::ErrorResult&) /home/worker/workspace/build/src/dom/html/nsHTMLDocument.cpp:3349:18
    #26 0x7f9087ba0336 in mozilla::dom::HTMLDocumentBinding::execCommand(JSContext*, JS::Handle<JSObject*>, nsHTMLDocument*, JSJitMethodCallArgs const&) /home/worker/workspace/build/src/obj-firefox/dom/bindings/HTMLDocumentBinding.cpp:876:21
    #27 0x7f9087ec8360 in mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*) /home/worker/workspace/build/src/dom/bindings/BindingUtils.cpp:3060:13

Comment 1

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Ehsan touched relevant code in nsPlainTextSerializer.cpp, nsDocumentEncoder.cpp recently. Maybe he has thoughts...

Comment 2

[(no longer active)]

I bet this is an imbalance in the number of times we push and pop things onto this stack.  I last encountered a similar issue in bug 1370737.  Someone needs to debug this, it's really hard to predict how this could happen without looking under a debugger.

Andrew, any chance you could help find an owner please?

Comment 3

[Andrew Overholt [:overholt]]

Henri, can you take a look at this next week (I know you've got other pressing things on the go this week)?

Comment 4

[Henri Sivonen (:hsivonen)]

(In reply to Andrew Overholt [:overholt] from comment #3)
> Henri, can you take a look at this next week (I know you've got other
> pressing things on the go this week)?

I'll take a look next week.

Comment 5

[Henri Sivonen (:hsivonen)]

So far, the most likely explanation is that the code at
https://searchfox.org/mozilla-central/source/dom/base/nsDocumentEncoder.cpp#1093
doesn't deal properly with a <tr> as a child of a <tr> (situation that the HTML parser cannot produce), but I haven't located the exact bug yet. Anyway, something, and I'm pretty sure it's that code, manages to mask the start of a <tr> in a case where it fails to mask its end.

Comment 6

[Henri Sivonen (:hsivonen)]

Attachment: Bug 1385272 - Make plaintext serializer not crash when a <tr> is a child of a <tr>.

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

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Could you explain this a bit?
Is it enough to check for tr or could some other elements as parents trigger the crash too?

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Given that the crash is related to Bug 1370737, I'd prefer if ehsan could review.

Comment 9

[Henri Sivonen (:hsivonen)]

(In reply to Olli Pettay [:smaug] from comment #7)
> Could you explain this a bit?
> Is it enough to check for tr or could some other elements as parents trigger
> the crash too?

The conditional that I extended already special-cases <tr> in order to fix bug 137450. No other element is special-cased in this particular way, so I'm confident that this particular problem is <tr>-specific. (AFAICT, prevNode is always either nullptr or a <tr>.)

Omitting the special-casing when a <tr> that would otherwise be special-cased has a <tr> parent, so that's what the patch does. However, I'm not confident that this fixes all problematic conditions that might be lurking in the <tr> special-casing. I didn't develop a full understanding of how this code is supposed to work. I wanted to test variations of the theme, but there are parts of the test case that don't affect whether a <tr> nests in a <tr> but are somehow necessary for triggering the crash, but since I don't have clarity on that interaction, it's hard for me to produce variations of the <tr> placement with confidence that non-crashing means the <tr> situation is OK as opposed to whatever else makes the range trigger in a crashy way still keeping the range as potentially-dangerous relative to the <tr>.

On a high level, the problem is that nsDocumentEncoder.cpp is doing something complex in order to produce the context required by the Windows CF_HTML clipboard format (and doing so in an Excel-compatible special way for <tr>) and nsDocumentEncoder.cpp expects the plaintext serializer to make that a no-op, but instead, that part does affect the internal state of the plaintext serializer, so a real fix would be disentangling the plaintext serializer altogether from traversals whose purpose is to generate the CF_HTML context.

Going forward, I think we should rewrite our serializers to disentangle the plaintext and markup code paths properly and to omit features that aren't browser-relevant. (And if we are going to rewrite an identifiable component, we might as well do it in Rust.)

Comment 10

[(no longer active)]

Comment on attachment 8896933 [details]
Bug 1385272 - Make plaintext serializer not crash when a <tr> is a child of a <tr>.

https://reviewboard.mozilla.org/r/168218/#review174018

Hmm, OK, r=me based on the explanation.  The patch would certainly make the crash go away.  I think for now while we have this code the way to bullet proof it more is to fuzz it more.  Thankfully due to the Linux specific clipboard.autocopy pref that is on by default on that platform, this code gets triggered after all selection changes during fuzzing, so we should see more fuzzer crashes if there are other bugs lurking around sooner or later.

::: dom/base/crashtests/crashtests.list:224
(Diff revision 1)
>  HTTP(..) load xhr_abortinprogress.html
>  load xhr_empty_datauri.html
>  load xhr_html_nullresponse.html
>  load 1383478.html
>  load 1383780.html
> +load 1385272-1.html

Please prefix this line with |pref(clipboard.autocopy, true) |, otherwise the crashtest only tests something on Linux.

Comment 11

[(no longer active)]

BTW, Jason, please note that during fuzzing you *really* want to make sure you have the clipboard.autocopy pref turned on.  Needinfoing you to make sure you read this comment.  :-)

Comment 12

[Jason Kratzer [:jkratzer]]

(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from comment #11)
> BTW, Jason, please note that during fuzzing you *really* want to make sure
> you have the clipboard.autocopy pref turned on.  Needinfoing you to make
> sure you read this comment.  :-)

Thanks for the tip!  I've added it to our fuzzing pref configuration.

Comment 13

[Henri Sivonen (:hsivonen)]

Comment on attachment 8896933 [details]
Bug 1385272 - Make plaintext serializer not crash when a <tr> is a child of a <tr>.

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

Comment 14

[Henri Sivonen (:hsivonen)]

Comment on attachment 8896933 [details]
Bug 1385272 - Make plaintext serializer not crash when a <tr> is a child of a <tr>.

https://reviewboard.mozilla.org/r/168218/#review174018

> Please prefix this line with |pref(clipboard.autocopy, true) |, otherwise the crashtest only tests something on Linux.

Prefixed. Thanks.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/661723c4cd45
Make plaintext serializer not crash when a <tr> is a child of a <tr>. r=Ehsan

Comment 16

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

https://hg.mozilla.org/mozilla-central/rev/661723c4cd45

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Going off blame, it looks like this issue goes back a ways, but maybe only 56 if bug 1370737 exposed the problem. Either way, is there a user impact here that justifies Beta backport consideration or can this ride the 57 train?

Comment 18

[Henri Sivonen (:hsivonen)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #17)
> Going off blame, it looks like this issue goes back a ways, but maybe only
> 56 if bug 1370737 exposed the problem. Either way, is there a user impact
> here that justifies Beta backport consideration or can this ride the 57
> train?

Since the problematic code was introduced by bug 137450 (bug 1370737 just made it more visible), I was thinking of requesting uplift not only to beta but to ESR, too.

Assuming non-malicious pages, the user impact is unlikely, since XHTML pages that have <tr> as child of <tr> or scripted DOMs with the same don't make sense for non-malicious purposes. As for impact from malicious cases, I'd rather err on the safe side and uplift.

Comment 19

[Henri Sivonen (:hsivonen)]

Comment on attachment 8896933 [details]
Bug 1385272 - Make plaintext serializer not crash when a <tr> is a child of a <tr>.

[Approval Request Comment]
> If this is not a sec:{high,crit} bug, please state case for ESR consideration:

This is a memory safety problem.

> User impact if declined: 

Memory-safety problem (at minimum a crashable) left in the product.

> Fix Landed on Version:

57

> Risk to taking this patch (and alternatives if risky): 

Low risk, because the control flow changes only for DOM shapes that the HTML parser never outputs.

> String or UUID changes made by this patch: 

None.

Approval Request Comment
> [Feature/Bug causing the regression]:

Bug 137450 introduced the faulty code. Bug 1370737 introduced the code that interacts visibly with the faulty code.

> [User impact if declined]:

Memory-safety problem (at minimum a crashable) left in the product.

> [Is this code covered by automated tests?]:

Yes.

> [Has the fix been verified in Nightly?]:

Yes.

> [Needs manual test from QE? If yes, steps to reproduce]: 

Not needed, but the steps would be opening attachment 8891335 [details] *on Linux*. (Possibly also opening it on Windows and Mac, doing Edit: Select All followed by Edit: Copy.)

> [List of other uplifts needed for the feature/fix]:

Just this patch.

> [Is the change risky?]:

No.

> [Why is the change risky/not risky?]:

Not risky, because the control flow changes only for DOM shapes that the HTML parser never outputs.

> [String changes made/needed]:

None.

Comment 20

[Henri Sivonen (:hsivonen)]

Correction:
There is no evidence of a memory-safety problem on ESR even though the faulty code is on ESR. (The code that interacts with the faulty code to produce the *known* memory-unsafety is not on ESR.)

Comment 21

[Gerry Chang [:gchang]]

Comment on attachment 8896933 [details]
Bug 1385272 - Make plaintext serializer not crash when a <tr> is a child of a <tr>.

Fix a crash and include tests. Beta56+.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/33443d73b7f4

Comment 23

[Sylvestre Ledru [:Sylvestre]]

The patch doesn't apply to esr52     

grafting 417096:661723c4cd45 "Bug 1385272 - Make plaintext serializer not crash when a <tr> is a child of a <tr>. r=Ehsan"
merging dom/base/crashtests/crashtests.list
merging dom/base/nsDocumentEncoder.cpp
 warning: conflicts while merging dom/base/crashtests/crashtests.list! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use 'hg resolve' and 'hg graft --continue')

Comment 24

[Julien Cristau [:jcristau]]

Comment on attachment 8896933 [details]
Bug 1385272 - Make plaintext serializer not crash when a <tr> is a child of a <tr>.

crash fix for esr52.  conflict in manifest should be trivial to rebase around.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/d68fa12fbffc