Closed
Bug 324918
Opened 19 years ago
Closed 19 years ago
This testcase triggers "index out of range" through nsHTMLSelectElement::RemoveOptionsFromListRecurse
Categories
(Core :: Layout: Form Controls, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: jruderman, Assigned: sicking)
References
(Depends on 1 open bug)
Details
(4 keywords, Whiteboard: [sg:investigate])
Attachments
(5 files, 1 obsolete file)
478 bytes,
application/xhtml+xml
|
Details | |
9.13 KB,
text/plain
|
Details | |
18.60 KB,
patch
|
bzbarsky
:
review+
jst
:
superreview+
jst
:
approval-branch-1.8.1+
dveditz
:
approval1.8.0.4+
|
Details | Diff | Splinter Review |
2.55 KB,
patch
|
bzbarsky
:
review+
bzbarsky
:
superreview+
jst
:
approval-branch-1.8.1+
dveditz
:
approval1.8.0.4+
|
Details | Diff | Splinter Review |
17.14 KB,
patch
|
Details | Diff | Splinter Review |
Steps to reproduce:
1. Load the testcase.
Result:
###!!! ASSERTION: index out of range: '0 <= aIndex && aIndex < Count()', file /Users/admin/trunk/mozilla/xpcom/glue/nsVoidArray.h, line 71
Break: at file /Users/admin/trunk/mozilla/xpcom/glue/nsVoidArray.h, line 71
Sicking noticed this assertion failure; and I figured out how to reproduce it and reduced it.
Security-sensitive because nsVoidArray::FastElementAt is not run-time checked.
Reporter | ||
Comment 1•19 years ago
|
||
Reporter | ||
Comment 2•19 years ago
|
||
Reporter | ||
Updated•19 years ago
|
Assignee: nobody → bugmail
Assignee | ||
Comment 3•19 years ago
|
||
So I fixed this on a few levels, since there were a few different bugs.
First off we tried to support <option>s that were children of non-optgroup elements. I.e. something like <select><p><option/></p><option/></select> would produce a select with two options. However, if options were added or removed from 'foregin' children using DOM methods the <select> would never be notified and so the options array would get out of sync.
I fixed this by simply removing support for foregin children. In the example above we would simply ignore the <p> and all its children.
Second, whenever the option array would get out of sync with reality we would deal with this very poorly. Ideally this should never happen, but the code is complex enough that I think we should at least try to deal. So I added some guards for that and some code that would rebuild the options array in extream cases, like when InsertChildAt fails.
Third, nsCOMArray::RemoveObjectAt does not deal well with out-of-bounds removes. It wouldn't release an object at the out-of-bounds index, but it would access it. I fixed this by adding bounds checks. RemoveObjectAt in general has a pretty bad signature, i filed bug 327598 on that.
Attachment #212198 -
Flags: superreview?(jst)
Attachment #212198 -
Flags: review?(bzbarsky)
Comment 4•19 years ago
|
||
Comment on attachment 212198 [details] [diff] [review]
Patch to fix
>Index: content/html/content/src/nsHTMLSelectElement.cpp
>+ /**
>+ * Rebuilds the options array from scratch as a fallback in error cases.
>+ */
>+ void RebuildOptionsArray();
What happens if the rebuild fails, if I might ask? Is it ok to have an out-of-sync array (having too little stuff in it)? The problem with this bug was too much stuff left in the array, right?
>@@ -458,27 +492,23 @@ nsHTMLSelectElement::nsHTMLSelectElement
I kinda wonder... Can't we give this sucker an Init() method that ensures a non-null mOptions? And then nuke all those null-checks? Followup bug, perhaps?
Also, no need to init mRestoreState to null here -- the nsRefPtr will do that on its own.
>+nsHTMLSelectElement::InsertChildAt(nsIContent* aKid,
>+ rv = nsGenericHTMLFormElement::InsertChildAt(aKid, aIndex, aNotify);
So say this threw NS_ERROR_OUT_OF_MEMORY (or say WillAddOptions did so). Is it safe to call RebuildOptionsArray at that point?
>+ if (mOptGroupCount && !prevOptGroups) {
>+ DispatchDOMEvent(NS_LITERAL_STRING("selectHasGroups"));
>+ }
Could we have a followup bug to remove that, as well as all similar event firing in this code? It was done for XBL form controls originally, and we don't do those. And I really don't like firing sync events from this code. Not safe at all. If we want to keep the events, we should do them only when the XBL form controls pref is set, and fire async. At least imo. We may be able to remove mOptGroupCount too, if we do this.
>@@ -675,46 +739,39 @@ nsHTMLSelectElement::InsertOptionsIntoLi
> if (optElement) {
>+ NS_ASSERTION(optElement, "not a real option");
That doesn't seem to be a useful assert, given that |if|.
>@@ -725,49 +782,43 @@ nsHTMLSelectElement::RemoveOptionsFromLi
> if (optElement) {
>+ NS_ASSERTION(optElement, "not a real option");
Same here.
>@@ -1683,17 +1734,16 @@ nsHTMLSelectElement::DoneAddingChildren(
> if (mRestoreState) {
> RestoreStateTo(mRestoreState);
>- NS_RELEASE(mRestoreState);
I'd still set mRestoreState = nsnull here. No reason to keep it around.
>@@ -1805,17 +1855,16 @@ nsHTMLSelectElement::HandleDOMEvent(nsPr
> nsSelectState* state = new nsSelectState();
> if (!state) {
> return NS_ERROR_OUT_OF_MEMORY;
> }
>- NS_ADDREF(state);
That looks wrong to me. What am I missing?
r=bzbarsky with the issues fixed.
Attachment #212198 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 5•19 years ago
|
||
(In reply to comment #4)
> (From update of attachment 212198 [details] [diff] [review] [edit])
> >Index: content/html/content/src/nsHTMLSelectElement.cpp
> >+ /**
> >+ * Rebuilds the options array from scratch as a fallback in error cases.
> >+ */
> >+ void RebuildOptionsArray();
>
> What happens if the rebuild fails, if I might ask? Is it ok to have an
> out-of-sync array (having too little stuff in it)? The problem with this bug
> was too much stuff left in the array, right?
See second last pagagraph of comment 3. I hope that we deal more or less well with this now. At least well enough that there are no security issues or crashes.
> >@@ -458,27 +492,23 @@ nsHTMLSelectElement::nsHTMLSelectElement
>
> I kinda wonder... Can't we give this sucker an Init() method that ensures a
> non-null mOptions? And then nuke all those null-checks? Followup bug,
> perhaps?
Yeah. Will file.
> Also, no need to init mRestoreState to null here -- the nsRefPtr will do that
> on its own.
Yup
> >+nsHTMLSelectElement::InsertChildAt(nsIContent* aKid,
> >+ rv = nsGenericHTMLFormElement::InsertChildAt(aKid, aIndex, aNotify);
>
> So say this threw NS_ERROR_OUT_OF_MEMORY (or say WillAddOptions did so). Is
> it safe to call RebuildOptionsArray at that point?
Well.. it should be. That method should be compleatly OOM safe in the sense that it shouldn't crash. I bet rendering and such look wrong, but there's no way to behave corrently when OOM.
> >+ if (mOptGroupCount && !prevOptGroups) {
> >+ DispatchDOMEvent(NS_LITERAL_STRING("selectHasGroups"));
> >+ }
>
> Could we have a followup bug to remove that, as well as all similar event
> firing in this code? It was done for XBL form controls originally, and we
> don't do those. And I really don't like firing sync events from this code.
> Not safe at all. If we want to keep the events, we should do them only when
> the XBL form controls pref is set, and fire async. At least imo. We may be
> able to remove mOptGroupCount too, if we do this.
Agreed. Will file.
> >@@ -675,46 +739,39 @@ nsHTMLSelectElement::InsertOptionsIntoLi
> > if (optElement) {
> >+ NS_ASSERTION(optElement, "not a real option");
>
> That doesn't seem to be a useful assert, given that |if|.
Doh! I had this code written another way, but I reverted back. Forgot to remove the assertion though. Will do.
> >@@ -1683,17 +1734,16 @@ nsHTMLSelectElement::DoneAddingChildren(
> > if (mRestoreState) {
> > RestoreStateTo(mRestoreState);
> >- NS_RELEASE(mRestoreState);
>
> I'd still set mRestoreState = nsnull here. No reason to keep it around.
Ah, good catch.
> >@@ -1805,17 +1855,16 @@ nsHTMLSelectElement::HandleDOMEvent(nsPr
> > nsSelectState* state = new nsSelectState();
> > if (!state) {
> > return NS_ERROR_OUT_OF_MEMORY;
> > }
> >- NS_ADDREF(state);
>
> That looks wrong to me. What am I missing?
Nothing, i goofed :)
Comment 6•19 years ago
|
||
Comment on attachment 212198 [details] [diff] [review]
Patch to fix
- In nsCOMArray_base::RemoveObjectAt(PRInt32 aIndex):
+ if (PRUint32(aIndex) < PRUint32(Count())) {
+ nsISupports* element = ObjectAt(aIndex);
This isn't safe if aIndex is a huge negative number since ObjectAt() calls into FastElementAt() on the internal array w/o any bounds checking. Not a big deal, nor likely to happen, but probably worth adding a < 0 check here too to be safe against that as well.
sr=jst with that and what bz pointed out fixed.
Attachment #212198 -
Flags: superreview?(jst) → superreview+
Assignee | ||
Comment 7•19 years ago
|
||
Err.. that's what the boundscheck on the line above is for. Check the implementation of nsVoidArray::SafeElementAt
Assignee | ||
Comment 8•19 years ago
|
||
We should let this bake on trunk for a while and then consider for 1.8.0.3
Flags: blocking1.8.0.3?
Assignee | ||
Comment 9•19 years ago
|
||
Oh, and the patch has been checked in. Thanks for the reviews.
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
I backed out the changes to nsHTMLSelectElement.cpp ... they was responsible for comboboxes simply not working at all. (The COMarray changes are still there.)
Maybe a simpler yet still performant approach would be to just blow away the option list whenever an option is inserted or removed, and recreate it the next time we need it.
Comment 11•19 years ago
|
||
Reopening, since this got backed out.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 12•19 years ago
|
||
I forgot that the callpaths hasn't merged at this point. Sort of ugly to call InsertChildAt, but it's what nsGenericElement is doing anyway.
I also added some extra safety since we want to be able to deal with the array being wrong. This was the only unsafe place i could find that was using ObjectAt.
Attachment #213703 -
Flags: superreview?(bzbarsky)
Attachment #213703 -
Flags: review?(bzbarsky)
Comment 13•19 years ago
|
||
Comment on attachment 213703 [details] [diff] [review]
missing bits
r+sr=bzbasky. Should we just inline AppendChildTo on nsINode? Or even non-inline, but still implement in terms of InsertChildAt? I'm not sure how these GetChildCount() calls play with the EnsureContentsGenerated crap that nsXULElement does, btw... I'd hope we could rip out some of that...
Attachment #213703 -
Flags: superreview?(bzbarsky)
Attachment #213703 -
Flags: superreview+
Attachment #213703 -
Flags: review?(bzbarsky)
Attachment #213703 -
Flags: review+
Comment 14•19 years ago
|
||
I'm not sure if this bug is causing my trunk to crash visiting http://www.ryanair.com/ but at line 2294 of nsHTMLSelectElement.cpp it calls into ReplaceChild, which by the time that reaches the end of nsGenericElement::doReplaceOrInsertBefore has managed to drop the last reference so that the NS_ADDREF is on a stale pointer. Switching line 2299 to an nsCOMPtr seems to resolve the problem for me.
Assignee | ||
Comment 15•19 years ago
|
||
That's bug 330084
Depends on: 330917
Updated•19 years ago
|
Flags: blocking1.8.0.3? → blocking1.8.0.3+
Assignee | ||
Updated•19 years ago
|
Attachment #212198 -
Flags: approval1.8.0.3?
Attachment #212198 -
Flags: approval-branch-1.8.1?(jst)
Assignee | ||
Updated•19 years ago
|
Attachment #213703 -
Flags: approval1.8.0.3?
Attachment #213703 -
Flags: approval-branch-1.8.1?(jst)
Updated•19 years ago
|
Attachment #212198 -
Flags: approval-branch-1.8.1?(jst) → approval-branch-1.8.1+
Updated•19 years ago
|
Attachment #213703 -
Flags: approval-branch-1.8.1?(jst) → approval-branch-1.8.1+
Assignee | ||
Updated•19 years ago
|
Keywords: fixed1.8.1
Assignee | ||
Comment 16•19 years ago
|
||
Comment 17•19 years ago
|
||
Comment on attachment 212198 [details] [diff] [review]
Patch to fix
approved for 1.8.0 branch, a=dveditz for drivers
Attachment #212198 -
Flags: approval1.8.0.3? → approval1.8.0.3+
Comment 18•19 years ago
|
||
Comment on attachment 213703 [details] [diff] [review]
missing bits
approved for 1.8.0 branch, a=dveditz for drivers
Attachment #213703 -
Flags: approval1.8.0.3? → approval1.8.0.3+
Assignee | ||
Comment 19•19 years ago
|
||
The last one wasn't quite right, i had merged the wrong version of the patch.
Attachment #219119 -
Attachment is obsolete: true
Assignee | ||
Comment 20•19 years ago
|
||
Marking fixed since this relanded on trunk.
Status: REOPENED → RESOLVED
Closed: 19 years ago → 19 years ago
Keywords: fixed1.8.0.3
Resolution: --- → FIXED
Comment 21•19 years ago
|
||
verified with Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.0.4) Gecko/20060508 Firefox/1.5.0.4
Keywords: fixed1.8.0.4 → verified1.8.0.4
Updated•19 years ago
|
Whiteboard: [sg:investigate]
Updated•18 years ago
|
Group: security
Comment 22•18 years ago
|
||
Jesse,
on 1.0.x branch I only get the runtime-checked assertion when running the attached testcase
###!!! ASSERTION: nsVoidArray::ElementAt(index past end array) - note on bug 96108: 'aIndex < Count()', file ../ds/nsVoidArray.h, line 72
... so no problem I guess?
You need to log in
before you can comment on or make changes to this bug.
Description
•