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.

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

'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 on attachment 604662 [details] [diff] [review] fix r=me

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).

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 on attachment 604662 [details] [diff] [review] fix see comment 7. Low risk.

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 on attachment 604662 [details] [diff] [review] fix The patch also applies to esr10.

Comment on attachment 604662 [details] [diff] [review] fix low risk, go ahead and land on ESR.

Is there anything QA can do to verify this fix?

(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.

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.

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

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?

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.

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

(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.