js_ExpandErrorArguments can crash in OOM conditions

RESOLVED FIXED

Status

()

Core
JavaScript Engine
--
critical
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: Gavin Reaney, Assigned: Gavin Reaney)

Tracking

({crash, fixed1.8.0.10, fixed1.8.1.1})

1.8 Branch
crash, fixed1.8.0.10, fixed1.8.1.1
Points:
---
Bug Flags:
blocking1.8.1.1 +
blocking1.8.0.10 +
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [need testcase])

Attachments

(2 attachments)

(Assignee)

Description

11 years ago
If js_ExpandErrorArguments is passed unicode arguments, it can crash in the error handling block (this only happens if OOM occurs earlier in the function):

error:
    if (reportp->messageArgs) {
        i = 0;
        while (reportp->messageArgs[i])
            JS_free(cx, (void *)reportp->messageArgs[i++]); <-- crash
    }

This is similar to bug 319264 but the case here was missed when that bug was fixed. The arguments should only be free'd if they were allocated by this function.
(Assignee)

Comment 1

11 years ago
Created attachment 242871 [details] [diff] [review]
Fix for double free

I'm not sure if I should assign the bug to me or not. Here's a patch anyway.

Comment 2

11 years ago
Comment on attachment 242871 [details] [diff] [review]
Fix for double free

looks right to me.

you might consider adding -p to your diff flags (not that it really helps here).

there's nothing wrong with assigning the bug to yourself. please ask for a review from a spidermonkey owner or peer <http://www.mozilla.org/owners.html#javascript>
Attachment #242871 - Flags: review?(brendan)
(Assignee)

Updated

11 years ago
Assignee: general → gavin
Comment on attachment 242871 [details] [diff] [review]
Fix for double free

Redirect to mrbkap, the last patch here was his IIRC.

/be
Attachment #242871 - Flags: review?(brendan) → review?(mrbkap)

Updated

11 years ago
Blocks: 355044
Flags: blocking1.8.1.1?

Updated

11 years ago
Attachment #242871 - Flags: review?(mrbkap) → review+
Do we want this fix in 1.8.0.x as well?
Flags: wanted1.8.0.x?
Flags: blocking1.8.1.1?
Flags: blocking1.8.1.1+
As this is blocking1.8.1.1+, please either request approval1.8.1.1 on the current patch or, if needed, attach a branch version of the patch and request approval1.8.1.1 on it.

Has this patch been committed to the trunk yet? If it needs to, please do so ASAP.
Status: NEW → ASSIGNED
(Assignee)

Comment 6

11 years ago
Comment on attachment 242871 [details] [diff] [review]
Fix for double free

mrbkap, can you commit this when you have a chance (or anyone else if mrbkap is not available)?

Thanks in advance.
Attachment #242871 - Flags: approval1.8.1.1?
mozilla/js/src/jscntxt.c 	3.93
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
OS: Windows XP → All
Hardware: PC → All
Resolution: --- → FIXED
Version: Trunk → 1.8 Branch

Updated

11 years ago
Flags: in-testsuite-
Comment on attachment 242871 [details] [diff] [review]
Fix for double free

approved for 1.8 branch, a=dveditz for drivers
Attachment #242871 - Flags: approval1.8.1.1? → approval1.8.1.1+
Whiteboard: [checkin needed (1.8 branch)]
mozilla/js/src/jscntxt.c 	3.60.4.9
Keywords: fixed1.8.1.1
Whiteboard: [checkin needed (1.8 branch)]
Flags: wanted1.8.0.x? → blocking1.8.0.10+
(Assignee)

Comment 10

11 years ago
Can someone with cvs commit access please commit this fix to the 1.8.0 branch? Thanks in advance.

Comment 11

11 years ago
Comment on attachment 242871 [details] [diff] [review]
Fix for double free

no. it doesn't have approval for that.
Attachment #242871 - Flags: approval1.8.0.10?

Comment 12

11 years ago
Comment on attachment 242871 [details] [diff] [review]
Fix for double free

Approved for 1.8.0 branch, a=jay for drivers.
Attachment #242871 - Flags: approval1.8.0.10? → approval1.8.0.10+
(Assignee)

Comment 13

11 years ago
(In reply to comment #11)
> (From update of attachment 242871 [details] [diff] [review])
> no. it doesn't have approval for that.
> 

It has 1.8.0.10 approval now so it's ready for landing (/me pleads again :-)).
(In reply to comment #13)
> It has 1.8.0.10 approval now so it's ready for landing (/me pleads again :-)).

I'll pick up the checkin in a few hours if no one has already by that time.
mozilla/js/src/jscntxt.c 	3.60.4.2.2.3
Keywords: fixed1.8.0.10

Updated

11 years ago
Whiteboard: [need testcase]

Comment 16

11 years ago
gavin, can you list some testcases so QA can verify this fix?  Thanks.
(Assignee)

Comment 17

11 years ago
(In reply to comment #16)
> gavin, can you list some testcases so QA can verify this fix?  Thanks.
> 

This isn't a bug that can be verified with a scripted test case since we require OOM to happen at a specific point. We also need to call the error reporter with utf16 strings - the engine itself doesn't do this, but the facility is available to embedding applications.

On that basis I've tested as follows:

- call the error reporter with a utf16 string from the JS shell
- fake a malloc failure in js_ExpandErrorArguments

Prior to the bug fix the engine would crash, but it now behaves correctly (the error report is correctly discarded). I'll upload the test code I used.
(Assignee)

Comment 18

11 years ago
Created attachment 254567 [details] [diff] [review]
Test code

Test code as described in comment 17.
You need to log in before you can comment on or make changes to this bug.