Closed Bug 456196 Opened 12 years ago Closed 11 years ago

Crash [@ nsCSSValueList::~nsCSSValueList] with adding a lot of values in css property

Categories

(Core :: CSS Parsing and Computation, defect, critical)

defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla1.9.1b2

People

(Reporter: martijn.martijn, Assigned: mats)

Details

(Keywords: crash, testcase, Whiteboard: [sg:nse dos] too much stack recursion)

Crash Data

Attachments

(5 files, 2 obsolete files)

Attached file testcase
See testcase, which hangs for a while, but after the hang, when you do a reload, Mozilla crashes.

Stack from debug build:
 	kernel32.dll!_TlsGetValue@4()  + 0x2 bytes	
>	nspr4.dll!_MD_CURRENT_THREAD()  Line 298 + 0xc bytes	C
 	nspr4.dll!PR_GetCurrentThread()  Line 175	C
 	nspr4.dll!PR_GetThreadPrivate(unsigned int index=1)  Line 232 + 0x5 bytes	C
 	xpcom_core.dll!NS_LogDtor_P(void * aPtr=0x0df49d18, const char * aType=0x043f9058, unsigned int aInstanceSize=12)  Line 1073 + 0x15 bytes	C++
 	gklayout.dll!nsCSSValueList::~nsCSSValueList()  Line 97 + 0x11 bytes	C++
 	gklayout.dll!nsCSSValueList::`scalar deleting destructor'()  + 0xf bytes	C++
 	gklayout.dll!nsCSSValueList::~nsCSSValueList()  Line 98 + 0x28 bytes	C++
 	gklayout.dll!nsCSSValueList::`scalar deleting destructor'()  + 0xf bytes	C++
 	gklayout.dll!nsCSSValueList::~nsCSSValueList()  Line 98 + 0x28 bytes	C++
 	gklayout.dll!nsCSSValueList::`scalar deleting destructor'()  + 0xf bytes	C++
 	gklayout.dll!nsCSSValueList::~nsCSSValueList()  Line 98 + 0x28 bytes	C++
 	gklayout.dll!nsCSSValueList::`scalar deleting destructor'()  + 0xf bytes	C++
 	gklayout.dll!nsCSSValueList::~nsCSSValueList()  Line 98 + 0x28 bytes	C++
 	gklayout.dll!nsCSSValueList::`scalar deleting destructor'()  + 0xf bytes	C++
 	gklayout.dll!nsCSSValueList::~nsCSSValueList()  Line 98 + 0x28 bytes	C++
 	gklayout.dll!nsCSSValueList::`scalar deleting destructor'()  + 0xf bytes	C++
 	gklayout.dll!nsCSSValueList::~nsCSSValueList()  Line 98 + 0x28 bytes	C++
etc..

This is also crashing in Firefox 3.
This reminds me of bug 440230, btw.
OS: Windows XP → All
Hardware: PC → All
Whiteboard: [sg:dos?] too much stack recursion
Attached patch wip (obsolete) — Splinter Review
We have a few "list classes" with destructors that recursively calls
"delete mNext" which exhausts the stack space for long lists.

Something like this should fix it.  We could also put a limit in
CSSParserImpl::ParseBorderColors() and others to reject unreasonably
long lists of values.  Maybe we should do both?
Assignee: nobody → mats.palmgren
You need to fix the cloning functions as well.

Would it be possible to share the NS_IF_DEEP_CLONE and NS_IF_DEEP_DELETE macros from that patch?
Flags: wanted1.9.0.x+
Flags: blocking1.9.0.4?
Whiteboard: [sg:dos?] too much stack recursion → [sg:nse dos] too much stack recursion
Group: core-security
A fix would be nice, but if it's just a DoS we don't need to block on it.
Flags: blocking1.9.0.4?
Attached patch Patch rev. 1 (obsolete) — Splinter Review
Attachment #339769 - Attachment is obsolete: true
Attachment #341582 - Flags: superreview?(dbaron)
Attachment #341582 - Flags: review?(dbaron)
Not sure if we can use the attached testcase as a crashtest, it takes
about 20 seconds to load...
Using -moz-transform instead seems pretty quick. I didn't use that before, because that can only be used on trunk.
Comment on attachment 341582 [details] [diff] [review]
Patch rev. 1

Oops, I introduced an error...
Attachment #341582 - Flags: superreview?(dbaron)
Attachment #341582 - Flags: review?(dbaron)
Attachment #341582 - Attachment is obsolete: true
Attached patch crashtest.diffSplinter Review
Thanks Martijn, the 2nd one works fine as a crashtest.
Attached patch Patch rev. 2Splinter Review
Attachment #341641 - Flags: superreview?(dbaron)
Attachment #341641 - Flags: review?(dbaron)
Comment on attachment 341641 [details] [diff] [review]
Patch rev. 2

I don't like exposing the boolean argument to the caller; the only
option for callers on the outside should be to clone the whole list.
Clone just a single item should be internal-only.  So you should make
header changes to nsCSSValueList and nsCSSValuePairList to give them a
second public clone function like nsAtomList, and make your current one
private.  

I don't like your macro names with "ITER" rather than "DEEP".  How about
NS_CSS_CLONE_LIST and NS_CSS_DELETE_LIST?  (The use of "LIST" sort of
implies the null-safety that the old "IF" implied.)

NS_CSS_ITER_CLONE_ALL_OR_NONE should not be a separate macro; it has no
independent callers.  It should just be merged in, which will let you
replace the break and the delete before it with the code from the latter
macro for the delete and the early return.

+  NS_ASSERTION(!mNegations || !mNegations->mNext, "mNegations can't have non-null mNext");

This is an 80th column violation.

So is this:
+    NS_CSS_ITER_CLONE(nsCSSSelector, this, mNegations, result, (PR_TRUE, PR_FALSE));

The macros should also parenthesize all uses of their arguments, except
args_.


r+sr=dbaron with those changes
Attachment #341641 - Flags: superreview?(dbaron)
Attachment #341641 - Flags: superreview+
Attachment #341641 - Flags: review?(dbaron)
Attachment #341641 - Flags: review+
really, probably NS_CSS_CLONE_LIST_MEMBER and NS_CSS_DELETE_LIST_MEMBER.
(In reply to comment #10)
> I don't like exposing the boolean argument to the caller

Fixed. Also for nsBorderColors.

> I don't like your macro names

Fixed, NS_CSS_CLONE_LIST_MEMBER and NS_CSS_DELETE_LIST_MEMBER it is.

> NS_CSS_ITER_CLONE_ALL_OR_NONE should not be a separate macro

Fixed.

> This is an 80th column violation.

Fixed all of those.

> The macros should also parenthesize all uses of their arguments, except
> args_.

Fixed (except for type_ and member_ which would not compile).
http://hg.mozilla.org/mozilla-central/rev/44687f34236c
http://hg.mozilla.org/mozilla-central/rev/8d0e5b53857f

-> FIXED
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1b2
Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b2pre) Gecko/20081029 Minefield/3.1b2pre
Status: RESOLVED → VERIFIED
Attachment #343519 - Flags: approval1.9.0.4?
Attachment #343519 - Flags: approval1.9.0.4? → approval1.9.0.5?
Comment on attachment 343519 [details] [diff] [review]
Patch rev. 3 (nits fixed)

Moving approval request to 1.9.0.5 since 1.9.0.4 is code frozen.
Flags: wanted1.9.0.x+
Comment on attachment 343519 [details] [diff] [review]
Patch rev. 3 (nits fixed)

Unless we've mischaracterized the severity of this problem, a patch this big is more than we'd want to take to fix an unexploitable non-topcrash.
Attachment #343519 - Flags: approval1.9.0.5? → approval1.9.0.5-
(In reply to comment #16)
> more than we'd want to take 

... in a security update. Thank you for fixing this on the trunk.
Crash Signature: [@ nsCSSValueList::~nsCSSValueList]
You need to log in before you can comment on or make changes to this bug.