Closed Bug 357398 Opened 14 years ago Closed 13 years ago

js_ExpandErrorArguments can crash in OOM conditions

Categories

(Core :: JavaScript Engine, defect, critical)

1.8 Branch
defect
Not set
critical

Tracking

()

RESOLVED FIXED

People

(Reporter: gavin.reaney, Assigned: gavin.reaney)

References

Details

(Keywords: crash, fixed1.8.0.10, fixed1.8.1.1, Whiteboard: [need testcase])

Attachments

(2 files)

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.
I'm not sure if I should assign the bug to me or not. Here's a patch anyway.
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: 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)
Blocks: js1.7src
Flags: blocking1.8.1.1?
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
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
Closed: 13 years ago
OS: Windows XP → All
Hardware: PC → All
Resolution: --- → FIXED
Version: Trunk → 1.8 Branch
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+
Can someone with cvs commit access please commit this fix to the 1.8.0 branch? Thanks in advance.
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 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+
(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
Whiteboard: [need testcase]
gavin, can you list some testcases so QA can verify this fix?  Thanks.
(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.
Attached patch Test codeSplinter Review
Test code as described in comment 17.
You need to log in before you can comment on or make changes to this bug.