Last Comment Bug 732941 - OOM Crash [@ nsCOMArray<nsISelectionListener>::operator[]] due to unhandled alloc failure in nsTypedSelection::NotifySelectionListeners
: OOM Crash [@ nsCOMArray<nsISelectionListener>::operator[]] due to unhandled a...
Status: RESOLVED FIXED
[sg:critical][qa-]
: assertion, crash
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: x86_64 Linux
: -- critical (vote)
: mozilla13
Assigned To: Mats Palmgren (:mats)
:
Mentors:
Depends on:
Blocks: 687256
  Show dependency treegraph
 
Reported: 2012-03-05 06:17 PST by Christian Holler (:decoder)
Modified: 2012-05-18 13:26 PDT (History)
9 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
fixed
12+
fixed


Attachments
fix (1.58 KB, patch)
2012-03-10 09:46 PST, Mats Palmgren (:mats)
bzbarsky: review+
akeybl: approval‑mozilla‑beta+
lukasblakk+bugs: approval‑mozilla‑esr10+
Details | Diff | Splinter Review

Description Christian Holler (:decoder) 2012-03-05 06:17:42 PST
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 (:mats) 2012-03-10 09:46:54 PST
Created attachment 604662 [details] [diff] [review]
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 (:mats) 2012-03-10 09:51:26 PST
'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 [:bz] 2012-03-10 09:54:16 PST
Comment on attachment 604662 [details] [diff] [review]
fix

r=me
Comment 5 Daniel Holbert [:dholbert] 2012-03-11 19:59:23 PDT
https://hg.mozilla.org/mozilla-central/rev/eeb3752c717c
Comment 6 Daniel Veditz [:dveditz] 2012-04-02 15:43:12 PDT
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 (:mats) 2012-04-02 16:20:34 PDT
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 (:mats) 2012-04-02 16:24:10 PDT
Comment on attachment 604662 [details] [diff] [review]
fix

see comment 7.  Low risk.
Comment 9 Alex Keybl [:akeybl] 2012-04-03 11:58:23 PDT
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 (:mats) 2012-04-03 12:44:35 PDT
https://hg.mozilla.org/releases/mozilla-beta/rev/8486e1b8a256
Comment 11 Mats Palmgren (:mats) 2012-04-03 12:45:29 PDT
Comment on attachment 604662 [details] [diff] [review]
fix

The patch also applies to esr10.
Comment 12 Lukas Blakk [:lsblakk] use ?needinfo 2012-04-03 13:03:24 PDT
Comment on attachment 604662 [details] [diff] [review]
fix

low risk, go ahead and land on ESR.
Comment 13 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-04-03 15:02:43 PDT
Is there anything QA can do to verify this fix?
Comment 15 Mats Palmgren (:mats) 2012-04-03 15:08:00 PDT
(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) 2012-04-03 15:36:13 PDT
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] 2012-04-16 17:39:27 PDT
Christian, can you verify that this is fixed on trunk and, if you have builds, 12?
Comment 18 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-04-17 06:02:04 PDT
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) 2012-04-17 06:07:40 PDT
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 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-04-17 06:12:15 PDT
Okay, thanks Christian. Does this go for all OOM crashes?
Comment 21 Christian Holler (:decoder) 2012-04-17 06:16:44 PDT
(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.

Note You need to log in before you can comment on or make changes to this bug.