732941 - OOM Crash [@ nsCOMArray<nsISelectionListener>::operator[]] due to unhandled alloc failure in nsTypedSelection::NotifySelectionListeners

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Christian Holler (:decoder)]

Tested on m-c revision 8ea5c983743f: The code in nsTypedSelection::NotifySelectionListeners does not properly handle an OOM condition (I assume it's while cloning selectionListeners) that leads to the following assertion and crash:

###!!! ASSERTION: nsVoidArray::FastElementAt: index out of range: '0 <= aIndex && aIndex < Count()', file ../../dist/include/nsVoidArray.h, line 78

Program received signal SIGSEGV, Segmentation fault.
nsCOMArray<nsISelectionListener>::operator[] (this=0x7fffffff8620, aIndex=0) at ../../dist/include/nsCOMArray.h:186
186             return ObjectAt(aIndex);
#0  nsCOMArray<nsISelectionListener>::operator[] (this=0x7fffffff8620, aIndex=0) at ../../dist/include/nsCOMArray.h:186
#1  0x00002aaaac7382ed in nsTypedSelection::NotifySelectionListeners (this=0x4e90470) at /srv/repos/browser/mozilla-central/layout/generic/nsSelection.cpp:5712
#2  0x00002aaaac738408 in nsTypedSelection::EndBatchChanges (this=<optimized out>) at /srv/repos/browser/mozilla-central/layout/generic/nsSelection.cpp:5734
#3  0x00002aaaacbb44e2 in nsEditor::DoTransaction (this=0x5c71bb0, aTxn=0x5c72210) at /srv/repos/browser/mozilla-central/editor/libeditor/base/nsEditor.cpp:716
#4  0x00002aaaacbacea0 in nsEditor::InsertNode (this=0x5c71bb0, aNode=0x5c72108, aParent=0x4e90668, aPosition=0) at /srv/repos/browser/mozilla-central/editor/libeditor/base/nsEditor.cpp:1404
#5  0x00002aaaacba81cd in CreateBogusNodeIfNeeded (aSelection=0x4e90470, this=0x5c71ee0) at /srv/repos/browser/mozilla-central/editor/libeditor/text/nsTextEditRules.cpp:1168
#6  nsTextEditRules::CreateBogusNodeIfNeeded (this=0x5c71ee0, aSelection=0x4e90470) at /srv/repos/browser/mozilla-central/editor/libeditor/text/nsTextEditRules.cpp:1115
#7  0x00002aaaacba97eb in Init (aEditor=<optimized out>, this=0x5c71ee0) at /srv/repos/browser/mozilla-central/editor/libeditor/text/nsTextEditRules.cpp:139
=> 0x2aaaac736e73 <nsCOMArray<nsISelectionListener>::operator[](int) const+73>: mov    0x8(%rax,%rbx,8),%rax
rax            0x0      0
rbx            0x0      0

(gdb) f 1
#1  0x00002aaaac7382ed in nsTypedSelection::NotifySelectionListeners (this=0x4fef960) at /srv/repos/browser/mozilla-central/layout/generic/nsSelection.cpp:5712
5712        nsISelectionListener* thisListener = selectionListeners[i];
(gdb) p selectionListeners
$1 = {<nsCOMArray_base> = {mArray = {mImpl = 0x0}}, <No data fields>}


The backtrace of the failing allocation is as follows:

#0 /srv/repos/browser/mozilla-central/objdir-ff-gcc64dbg/dist/bin/libmozalloc.so(moz_malloc+0x5f)
#1 nsVoidArray::SizeTo(int) at objdir-ff-gcc64dbg/xpcom/build/nsVoidArray.cpp:231
#2 nsVoidArray::Count() const at xpcom/glue/nsVoidArray.h:67
#3 nsCOMPtr<nsIPresShell>::begin_assignment() at objdir-ff-gcc64dbg/dist/include/nsCOMPtr.h:1241 
#4 nsTypedSelection::EndBatchChanges() at layout/generic/nsSelection.cpp:5737
#5 nsEditor::DoTransaction(nsITransaction*) at editor/libeditor/base/nsEditor.cpp:702
#6 nsEditor::InsertNode(nsIDOMNode*, nsIDOMNode*, int) at editor/libeditor/base/nsEditor.cpp:1404
#7 nsTextEditRules::CreateBogusNodeIfNeeded(nsISelection*) at editor/libeditor/text/nsTextEditRules.cpp:1169
#8 nsTextEditRules::Init(nsPlaintextEditor*) at editor/libeditor/text/nsTextEditRules.cpp:140
#9 nsPlaintextEditor::EndEditorInit() at editor/libeditor/text/nsPlaintextEditor.cpp:221
#10 ~nsAutoEditInitRulesTrigger at editor/libeditor/text/nsTextEditUtils.cpp:123
#11 nsPlaintextEditor::Init(nsIDOMDocument*, nsIContent*, nsISelectionController*, unsigned int) at editor/libeditor/text/nsPlaintextEditor.cpp:159
#12 nsTextEditorState::PrepareEditor(nsAString_internal const*) at content/html/content/src/nsTextEditorState.cpp:1225
#13 nsTextEditorState::GetEditor() at content/html/content/src/nsTextEditorState.cpp:1010
#14 ns_if_addref<nsIEditor*> at objdir-ff-gcc64dbg/dist/include/nsISupportsUtils.h:93
#15 /srv/repos/browser/mozilla-central/objdir-ff-gcc64dbg/dist/bin/libxul.so(NS_InvokeByIndex_P+0x23a)


I'm marking this as s-s for now because it's not clear to me what mSelectionListeners contains exactly, especially if the attacker can control how many listeners are in there (which could turn the crash into a controlled read). However, even in that case, exploitation is likely very hard because creating a controlled OOM in that location is surely not trivial.

Comment 1

[Mats Palmgren (inactive)]

Attachment: fix

Check that we copied the whole array.

I'm also removing the null-check inside the loop since
nsTypedSelection::AddSelectionListener rejects null and mSelectionListeners
isn't inserted to from anywhere else.

https://tbpl.mozilla.org/?usebuildbot=1&tree=Try&rev=b147664bf01a

Comment 2

[Mats Palmgren (inactive)]

'thisListener' used to be the result of a QI at some point so that's where the
null-check came from:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/generic/nsSelection.cpp&rev=3.1&root=/cvsroot#1849

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 604662 [details] [diff] [review]
fix

r=me

Comment 4

[Mats Palmgren (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/eeb3752c717c

Comment 5

[Daniel Holbert [:dholbert]]

https://hg.mozilla.org/mozilla-central/rev/eeb3752c717c

Comment 6

[Daniel Veditz [:dveditz]]

This looks like a null deref unless I'm reading over this too quickly. If I've gotten that wrong please nominate for the ESR branch (and ask for approval on the patch).

Comment 7

[Mats Palmgren (inactive)]

It's not a null deref.  We're reading beyond the array buffer end and interpreting
that as a nsISelectionListener object and calling a virtual function on that
bogus object.

Comment 8

[Mats Palmgren (inactive)]

Comment on attachment 604662 [details] [diff] [review]
fix

see comment 7.  Low risk.

Comment 9

[Alex Keybl [:akeybl]]

Comment on attachment 604662 [details] [diff] [review]
fix

[Triage Comment]
Low risk sg: crit fix. Please land on m-b and prepare a patch for the ESR.

Comment 10

[Mats Palmgren (inactive)]

https://hg.mozilla.org/releases/mozilla-beta/rev/8486e1b8a256

Comment 11

[Mats Palmgren (inactive)]

Comment on attachment 604662 [details] [diff] [review]
fix

The patch also applies to esr10.

Comment 12

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 604662 [details] [diff] [review]
fix

low risk, go ahead and land on ESR.

Comment 13

[u279076]

Is there anything QA can do to verify this fix?

Comment 14

[Mats Palmgren (inactive)]

https://hg.mozilla.org/releases/mozilla-esr10/rev/076f467733b4

Comment 15

[Mats Palmgren (inactive)]

(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #13)
> Is there anything QA can do to verify this fix?

Christian triggered this crash with a OOM testing tool, presumably it's
repeatable.

Comment 16

[Christian Holler (:decoder)]

There is no portable solution yet to trigger this crash because my tool uses absolute addresses to indicate which functions to cause failure in. I'm not entirely sure if there can be any way to test this in a portable way without instrumenting the particular code in question.

Comment 17

[Al Billings [:abillings - ex-MoCo]]

Christian, can you verify that this is fixed on trunk and, if you have builds, 12?

Comment 18

[u279076]

qa- for verification given recent comments. Christian is there any assistance you can give in verifying this for Firefox 12, 13 and ESR 10.0.4?

Comment 19

[Christian Holler (:decoder)]

Unfortunately there is no direct way for me to verify this, other than performing another full run of my scripts on the initial test/url which takes really long.

The reason for this is that there is no portable way (portable across builds) to indicate where an OOM condition should be triggered. I might be working on that in the future but right now, we can't verify this directly.

Comment 20

[u279076]

Okay, thanks Christian. Does this go for all OOM crashes?

Comment 21

[Christian Holler (:decoder)]

(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #20)
> Okay, thanks Christian. Does this go for all OOM crashes?

At least for those that I filed blocking bug 687256, yes.

For other OOM crashes (e.g. those that Bob Clary files), it is usually also very hard to verify a fix, because OOM conditions are very fragile, so not seeing a crash anymore does not mean it is fixed.

The only OOM conditions that can somewhat be verified fixed are those triggered in the JS shell, either by a fuzz test or those involving -A parameter. The parameter varies with builds too, but it's possible to cover a certain range for that parameter to ensure the issue is gone, and I do have scripts for that.