Last Comment Bug 348511 - XSLTProcessor.setParameter("","f3ck",array) causes heap trouble
: XSLTProcessor.setParameter("","f3ck",array) causes heap trouble
Status: RESOLVED FIXED
[sg:critical?] not in 1.7/aviary
: verified1.8.0.7, verified1.8.1
Product: Core
Classification: Components
Component: XSLT (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla1.9alpha1
Assigned To: Peter Van der Beken [:peterv] - away till Aug 1st
: Keith Visco
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-08-13 03:56 PDT by georgi - hopefully not receiving bugspam
Modified: 2008-10-17 16:03 PDT (History)
3 users (show)
dveditz: blocking1.7.14-
dveditz: blocking‑aviary1.0.9-
dveditz: blocking1.8.0.7+
dveditz: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
this is a testcase - glibc whining (329 bytes, text/html)
2006-08-13 03:58 PDT, georgi - hopefully not receiving bugspam
no flags Details
v1 (1.15 KB, patch)
2006-08-13 12:41 PDT, Peter Van der Beken [:peterv] - away till Aug 1st
bzbarsky: review+
bzbarsky: superreview+
Details | Diff | Splinter Review
v1 (branch version) (849 bytes, patch)
2006-08-13 12:42 PDT, Peter Van der Beken [:peterv] - away till Aug 1st
dveditz: approval1.8.0.7+
dbaron: approval1.8.1+
Details | Diff | Splinter Review

Description georgi - hopefully not receiving bugspam 2006-08-13 03:56:55 PDT
XSLTProcessor.setParameter("","f3ck",array) causes heap trouble, 
probably either heap overflow or wild pointers.

on trunk, 2.0 and 1.5

*** glibc detected *** free(): invalid next size (fast): 0x08dd6ef8 ***
Comment 1 georgi - hopefully not receiving bugspam 2006-08-13 03:58:07 PDT
Created attachment 233448 [details]
this is a testcase - glibc whining
Comment 2 Peter Van der Beken [:peterv] - away till Aug 1st 2006-08-13 12:41:17 PDT
Created attachment 233495 [details] [diff] [review]
v1
Comment 3 Peter Van der Beken [:peterv] - away till Aug 1st 2006-08-13 12:42:22 PDT
Created attachment 233496 [details] [diff] [review]
v1 (branch version)
Comment 4 Boris Zbarsky [:bz] 2006-08-13 13:05:34 PDT
Comment on attachment 233495 [details] [diff] [review]
v1

Yeah, makes sense.
Comment 5 Peter Van der Beken [:peterv] - away till Aug 1st 2006-08-13 13:25:58 PDT
Georgi, could you verify whether this fixes the problems?
Comment 6 georgi - hopefully not receiving bugspam 2006-08-14 01:08:24 PDT
this seems fixed on today trunk.

if i understand correctly the side effects of macro arguments, NS_RELEASE doesn't seems misused in other places (according to grep)
Comment 7 Peter Van der Beken [:peterv] - away till Aug 1st 2006-08-14 01:37:51 PDT
Comment on attachment 233496 [details] [diff] [review]
v1 (branch version)

NS_RELEASE first releases and then overwrites the pointer with 0. Since it evaluates the macro argument twice that means we could end up writing a null just past the end of the array.
Comment 8 georgi - hopefully not receiving bugspam 2006-08-14 02:31:01 PDT
(In reply to comment #7)
> (From update of attachment 233496 [details] [diff] [review] [edit])
> NS_RELEASE first releases and then overwrites the pointer with 0. Since it
> evaluates the macro argument twice that means we could end up writing a null
> just past the end of the array.
> 

yes, that is what the macro definition is.

so it seems to allow writing zero past the last element.

and if the array.length > 1 it screws memory on the way to the end.

Comment 9 georgi - hopefully not receiving bugspam 2006-08-14 02:33:12 PDT
in comment #6 i meant that grep doesn't catch other misuses of NS_RELEASE(++/--)
Comment 10 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2006-08-14 16:19:10 PDT
Comment on attachment 233496 [details] [diff] [review]
v1 (branch version)

a=dbaron on behalf of drivers.  Please check in to MOZILLA_1_8_BRANCH and add the fixed1.8.1 keyword once you have done so.
Comment 11 Daniel Veditz [:dveditz] 2006-08-15 15:52:31 PDT
Comment on attachment 233496 [details] [diff] [review]
v1 (branch version)

approved for 1.8.0 branch, a=dveditz for drivers
Comment 12 Daniel Veditz [:dveditz] 2006-08-23 08:11:12 PDT
fix checked into 1.8 and 1.8.0 branches
Comment 13 Jay Patel [:jay] 2006-08-24 14:39:17 PDT
v.fixed on both branches by code inspection, this was a simple fix.  If anyone has a testcase, we can retest.
Comment 14 georgi - hopefully not receiving bugspam 2006-08-25 00:35:10 PDT
(In reply to comment #13)
> v.fixed on both branches by code inspection, this was a simple fix.  If anyone
> has a testcase, we can retest.
> 

the testcase is in this bug titled "glibc whining"
Comment 15 Alexander Sack 2006-09-15 12:45:04 PDT
the affected code appears not to exist on 1.0.x ... Peter, can you confirm that aviary 1.0.1 branch is not affected?
Comment 16 Peter Van der Beken [:peterv] - away till Aug 1st 2006-09-15 13:00:39 PDT
(In reply to comment #15)
> the affected code appears not to exist on 1.0.x ... Peter, can you confirm that
> aviary 1.0.1 branch is not affected?

It is not affected.
Comment 17 georgi - hopefully not receiving bugspam 2006-09-18 23:50:19 PDT
as per comment #14 there is testcase attached in this bug.

just renamed the testcase, removing "need testcase"

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