Closed Bug 732941 Opened 12 years ago Closed 12 years ago

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


(Core :: Layout, defect)

Not set



Tracking Status
firefox12 --- fixed
firefox13 --- fixed
firefox-esr10 12+ fixed


(Reporter: decoder, Assigned: MatsPalmgren_bugz)



(Keywords: assertion, crash, Whiteboard: [sg:critical][qa-])

Crash Data


(1 file)

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/
#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/

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.
Assignee: nobody → matspal
Attached patch fixSplinter Review
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.
Attachment #604662 - Flags: review?(bzbarsky)
'thisListener' used to be the result of a QI at some point so that's where the
null-check came from:
Comment on attachment 604662 [details] [diff] [review]

Attachment #604662 - Flags: review?(bzbarsky) → review+
Whiteboard: [inbound]
Target Milestone: --- → mozilla13
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
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).
Whiteboard: [sg:dos null deref]
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.
Whiteboard: [sg:dos null deref] → [sg:critical]
Comment on attachment 604662 [details] [diff] [review]

see comment 7.  Low risk.
Attachment #604662 - Flags: approval-mozilla-beta?
Comment on attachment 604662 [details] [diff] [review]

[Triage Comment]
Low risk sg: crit fix. Please land on m-b and prepare a patch for the ESR.
Attachment #604662 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 604662 [details] [diff] [review]

The patch also applies to esr10.
Attachment #604662 - Flags: approval-mozilla-esr10?
Comment on attachment 604662 [details] [diff] [review]

low risk, go ahead and land on ESR.
Attachment #604662 - Flags: approval-mozilla-esr10? → approval-mozilla-esr10+
Is there anything QA can do to verify this fix?
Whiteboard: [sg:critical] → [sg:critical][qa?]
(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
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?
Whiteboard: [sg:critical][qa?] → [sg:critical][qa-]
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.
Group: core-security
You need to log in before you can comment on or make changes to this bug.