Last Comment Bug 357398 - js_ExpandErrorArguments can crash in OOM conditions
: js_ExpandErrorArguments can crash in OOM conditions
Status: RESOLVED FIXED
[need testcase]
: crash, fixed1.8.0.10, fixed1.8.1.1
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: 1.8 Branch
: All All
: -- critical (vote)
: ---
Assigned To: Gavin Reaney
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks: js1.7src
  Show dependency treegraph
 
Reported: 2006-10-20 05:30 PDT by Gavin Reaney
Modified: 2007-02-09 13:11 PST (History)
7 users (show)
dveditz: blocking1.8.1.1+
dveditz: blocking1.8.0.10+
bob: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Fix for double free (774 bytes, patch)
2006-10-20 05:33 PDT, Gavin Reaney
mrbkap: review+
dveditz: approval1.8.1.1+
jaymoz: approval1.8.0.10+
Details | Diff | Splinter Review
Test code (1.40 KB, patch)
2007-02-09 13:11 PST, Gavin Reaney
no flags Details | Diff | Splinter Review

Description Gavin Reaney 2006-10-20 05:30:18 PDT
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 2006-10-20 05:33:07 PDT
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 timeless 2006-10-20 06:22:36 PDT
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] 2006-10-20 08:25:39 PDT
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] 2006-11-10 16:30:59 PST
Do we want this fix in 1.8.0.x as well?
Comment 5 Reed Loden [:reed] (use needinfo?) 2006-11-22 20:06:02 PST
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 2006-11-23 13:28:08 PST
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: gavin@gavinsharp.com] 2006-11-23 20:55:30 PST
mozilla/js/src/jscntxt.c 	3.93
Comment 8 Daniel Veditz [:dveditz] 2006-11-27 16:57:17 PST
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: gavin@gavinsharp.com] 2006-11-27 17:01:26 PST
mozilla/js/src/jscntxt.c 	3.60.4.9
Comment 10 Gavin Reaney 2007-01-06 06:35:48 PST
Can someone with cvs commit access please commit this fix to the 1.8.0 branch? Thanks in advance.
Comment 11 timeless 2007-01-06 09:12:55 PST
Comment on attachment 242871 [details] [diff] [review]
Fix for double free

no. it doesn't have approval for that.
Comment 12 Jay Patel [:jay] 2007-01-08 15:46:50 PST
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 2007-01-15 02:33:57 PST
(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] (remove +bmo to email) 2007-01-15 08:15:02 PST
(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: gavin@gavinsharp.com] 2007-01-15 11:17:12 PST
mozilla/js/src/jscntxt.c 	3.60.4.2.2.3
Comment 16 Tony Chung [:tchung] 2007-02-08 14:30:02 PST
gavin, can you list some testcases so QA can verify this fix?  Thanks.
Comment 17 Gavin Reaney 2007-02-09 13:08:58 PST
(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 2007-02-09 13:11:02 PST
Created attachment 254567 [details] [diff] [review]
Test code

Test code as described in comment 17.

Note You need to log in before you can comment on or make changes to this bug.