Closed Bug 348511 Opened 15 years ago Closed 15 years ago

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

Categories

(Core :: XSLT, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9alpha1

People

(Reporter: guninski, Assigned: peterv)

Details

(Keywords: verified1.8.0.7, verified1.8.1, Whiteboard: [sg:critical?] not in 1.7/aviary)

Attachments

(3 files)

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 ***
Product: Firefox → Core
Component: General → Web Services
Assignee: nobody → xslt
Component: Web Services → XSLT
QA Contact: general → keith
Attached patch v1Splinter Review
Assignee: xslt → peterv
Status: NEW → ASSIGNED
Attachment #233495 - Flags: superreview?(bzbarsky)
Attachment #233495 - Flags: review?(bzbarsky)
Comment on attachment 233495 [details] [diff] [review]
v1

Yeah, makes sense.
Attachment #233495 - Flags: superreview?(bzbarsky)
Attachment #233495 - Flags: superreview+
Attachment #233495 - Flags: review?(bzbarsky)
Attachment #233495 - Flags: review+
Georgi, could you verify whether this fixes the problems?
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 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.
Attachment #233496 - Flags: approval1.8.1?
Attachment #233496 - Flags: approval1.8.0.7?
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
OS: Linux → All
Hardware: PC → All
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9alpha
(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.

in comment #6 i meant that grep doesn't catch other misuses of NS_RELEASE(++/--)
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.
Attachment #233496 - Flags: approval1.8.1? → approval1.8.1+
Comment on attachment 233496 [details] [diff] [review]
v1 (branch version)

approved for 1.8.0 branch, a=dveditz for drivers
Attachment #233496 - Flags: approval1.8.0.7? → approval1.8.0.7+
Flags: blocking1.8.1?
Flags: blocking1.8.0.7?
Whiteboard: [sg:critical?]
Flags: blocking1.8.0.7? → blocking1.8.0.7+
fix checked into 1.8 and 1.8.0 branches
v.fixed on both branches by code inspection, this was a simple fix.  If anyone has a testcase, we can retest.
Whiteboard: [sg:critical?] → [sg:critical?][need testcase]
(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"
the affected code appears not to exist on 1.0.x ... Peter, can you confirm that aviary 1.0.1 branch is not affected?
(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.
Flags: blocking1.7.14-
Flags: blocking-aviary1.0.9-
Whiteboard: [sg:critical?][need testcase] → [sg:critical?][need testcase] not in 1.7/aviary
Attachment #233448 - Attachment description: glibc whining → this is a testcase - glibc whining
as per comment #14 there is testcase attached in this bug.

just renamed the testcase, removing "need testcase"
Whiteboard: [sg:critical?][need testcase] not in 1.7/aviary → [sg:critical?] not in 1.7/aviary
Group: security
Flags: in-testsuite?
Flags: blocking1.8.1?
You need to log in before you can comment on or make changes to this bug.