348511 - XSLTProcessor.setParameter("","f3ck",array) causes heap trouble

Status: RESOLVED FIXED

(Core :: XSLT, defect)

Description

[georgi - hopefully not receiving bugspam]

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]

Attachment: this is a testcase - glibc whining

Comment 2

[Peter Van der Beken [:peterv]]

Attachment: v1

Comment 3

[Peter Van der Beken [:peterv]]

Attachment: v1 (branch version)

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 233495 [details] [diff] [review]
v1

Yeah, makes sense.

Comment 5

[Peter Van der Beken [:peterv]]

Georgi, could you verify whether this fixes the problems?

Comment 6

[georgi - hopefully not receiving bugspam]

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]]

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]

(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]

in comment #6 i meant that grep doesn't catch other misuses of NS_RELEASE(++/--)

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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]]

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]]

fix checked into 1.8 and 1.8.0 branches

Comment 13

[Jay Patel [:jay]]

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]

(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]

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]]

(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]

as per comment #14 there is testcase attached in this bug.

just renamed the testcase, removing "need testcase"