357398 - js_ExpandErrorArguments can crash in OOM conditions

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Gavin Reaney]

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.

Comment 1

[Gavin Reaney]

Attachment: 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

[timeless]

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>

Comment 3

[Brendan Eich [:brendan]]

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

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

/be

Comment 4

[Daniel Veditz [:dveditz]]

Do we want this fix in 1.8.0.x as well?

Comment 5

[Reed Loden [:reed]]

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.

Comment 6

[Gavin Reaney]

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.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

mozilla/js/src/jscntxt.c 	3.93

Comment 8

[Daniel Veditz [:dveditz]]

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

approved for 1.8 branch, a=dveditz for drivers

Comment 9

[:Gavin Sharp [email: [e-mail]]]

mozilla/js/src/jscntxt.c 	3.60.4.9

Comment 10

[Gavin Reaney]

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

Comment 11

[timeless]

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

no. it doesn't have approval for that.

Comment 12

[Jay Patel [:jay]]

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

Approved for 1.8.0 branch, a=jay for drivers.

Comment 13

[Gavin Reaney]

(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 :-)).

Comment 14

[Jeff Walden [:Waldo]]

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

Comment 15

[:Gavin Sharp [email: [e-mail]]]

mozilla/js/src/jscntxt.c 	3.60.4.2.2.3

Comment 16

[Tony Chung [:tchung]]

gavin, can you list some testcases so QA can verify this fix?  Thanks.

Comment 17

[Gavin Reaney]

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

Comment 18

[Gavin Reaney]

Attachment: Test code

Test code as described in comment 17.