Last Comment Bug 338804 - GC hazards in constructor functions
: GC hazards in constructor functions
Status: VERIFIED FIXED
[sg:critical?]
: crash, verified1.8.0.5, verified1.8.1
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- critical (vote)
: ---
Assigned To: Igor Bukanov
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks: sbb?
  Show dependency treegraph
 
Reported: 2006-05-22 03:54 PDT by shutdown
Modified: 2007-02-08 16:41 PST (History)
6 users (show)
dveditz: blocking1.7.14?
dveditz: blocking‑aviary1.0.9?
dveditz: blocking1.9a1+
dveditz: blocking1.8.1+
dveditz: blocking1.8.0.5+
bob: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (912 bytes, text/html)
2006-05-22 04:09 PDT, shutdown
no flags Details
js1_5/GC/regress-338804-01.js (scale 3000) (2.92 KB, text/plain)
2006-05-22 05:51 PDT, Bob Clary [:bc:]
no flags Details
js1_5/GC/regress-338804-02.js (scale 2000) (2.93 KB, text/plain)
2006-05-22 05:54 PDT, Bob Clary [:bc:]
no flags Details
regress-338804-03.js to crash browser/jsshell 100% (2.24 KB, text/plain)
2006-05-22 17:35 PDT, Igor Bukanov
no flags Details
regress-338804-03.js to crash browser/jsshell 100% v2 (2.23 KB, text/plain)
2006-05-22 17:45 PDT, Igor Bukanov
no flags Details
Fix (4.29 KB, patch)
2006-05-22 17:56 PDT, Igor Bukanov
mrbkap: review+
brendan: superreview+
Details | Diff | Splinter Review
patch to commit (4.29 KB, patch)
2006-05-23 00:50 PDT, Igor Bukanov
no flags Details | Diff | Splinter Review
Patch for branch 1.8.1 (4.32 KB, patch)
2006-05-23 01:08 PDT, Igor Bukanov
brendan: approval‑branch‑1.8.1+
dveditz: approval1.8.0.5+
Details | Diff | Splinter Review
Patch for AVIARY_1_0_1 and MOZILLA_1_7 branches (3.27 KB, patch)
2006-05-23 01:50 PDT, Igor Bukanov
no flags Details | Diff | Splinter Review

Description shutdown 2006-05-22 03:54:59 PDT
The object allocated at jsscript.c:878 is not rooted properly.
Thus it might be GC-ed in the following script_compile() call.
File() and RegExp() constructors also have similar issues.

873 static JSBool
874 Script(JSContext *cx, JSObject *obj, uintN argc, jsval *argv, jsval *rval)
875 {
876     /* If not constructing, replace obj with a new Script object. */
877     if (!(cx->fp->flags & JSFRAME_CONSTRUCTING)) {
878         obj = js_NewObject(cx, &js_ScriptClass, NULL, NULL);
879         if (!obj)
880             return JS_FALSE;
881     }
882     return script_compile(cx, obj, argc, argv, rval);
883 }

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/js/src/jsfile.c&rev=3.42&mark=2170,2172,2179,2194#2170
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/js/src/jsregexp.c&rev=3.119&mark=4045,4059,4063#4045
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/js/src/jsscript.c&rev=3.99&mark=877,878,882#877
Comment 1 shutdown 2006-05-22 04:09:05 PDT
Created attachment 222842 [details]
testcase

Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8.0.4)
Gecko/20060521 Firefox/1.5.0.4
  TB18974351G

Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8.1a2)
Gecko/20060521 BonEcho/2.0a2
  TB18974375E

FIREFOX caused an exception 03H in
module unknown at 0000:03030480.
Registers:
EAX=deadfeed CS=017f EIP=03030480 EFLGS=00000206
EBX=deadfeed SS=0187 ESP=00d6f71c EBP=00d6f73c
ECX=deadfeed DS=0187 ESI=03227548 FS=556f
EDX=deadfeed ES=0187 EDI=03227538 GS=0000
Bytes at CS:EIP:
90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90
Stack dump:
600f8095 02877060 03227538 00000003 03227538 20202020 02877060 03227548
00d6f780 600d2da1 02877060 03227538 00000003 02877060 02955e00 03227538

exploitable.
Comment 2 Bob Clary [:bc:] 2006-05-22 05:51:22 PDT
Created attachment 222851 [details]
js1_5/GC/regress-338804-01.js (scale 3000)

scale 3000, modified to call exploit inline, doesn't reliably crash trunk.
Comment 3 Bob Clary [:bc:] 2006-05-22 05:54:34 PDT
Created attachment 222852 [details]
js1_5/GC/regress-338804-02.js (scale 2000)

scale 2000, crashes 1.5.0.4/trunk reliably.

Note, I added this and the previous test to a new GC subdirectory of js1_5.  I think I should probably move most of the GC related tests to a central location such as this so they can be easily selected out when you don't want to run the memory intensive GC tests. The e4x GC tests could live in e4x/GC. Brendan, is that ok?
Comment 4 Brendan Eich [:brendan] 2006-05-22 14:54:27 PDT
Igor, could you take this one?  I think Blake is booked for this week, traveling Wednesday back to New York, and stuff.  Thanks,

/be
Comment 5 Brendan Eich [:brendan] 2006-05-22 14:55:08 PDT
(In reply to comment #3)
> Created an attachment (id=222852) [edit]
> js1_5/GC/regress-338804-02.js (scale 2000)
> 
> scale 2000, crashes 1.5.0.4/trunk reliably.
> 
> Note, I added this and the previous test to a new GC subdirectory of js1_5.  I
> think I should probably move most of the GC related tests to a central location
> such as this so they can be easily selected out when you don't want to run the
> memory intensive GC tests. The e4x GC tests could live in e4x/GC. Brendan, is
> that ok?

Yes, please!

Thanks,

/be
Comment 6 Igor Bukanov 2006-05-22 17:35:48 PDT
Created attachment 222975 [details]
regress-338804-03.js to crash browser/jsshell 100%

This is a version of the test case to reliably expose the problem in jsshell/browser.
Comment 7 Igor Bukanov 2006-05-22 17:45:17 PDT
Created attachment 222976 [details]
regress-338804-03.js to crash browser/jsshell 100% v2

This is a test case version that checks for both Script and RegExp problems.
Comment 8 Igor Bukanov 2006-05-22 17:56:47 PDT
Created attachment 222977 [details] [diff] [review]
Fix

Besides fixing Script, RegExp and File cases, the patch also touches Exception() from jsexn.c. That code contains:

         ok = OBJ_GET_PROPERTY(cx, JSVAL_TO_OBJECT(argv[-2]),
                              ATOM_TO_JSID(cx->runtime->atomState
                                           .classPrototypeAtom),
                              &pval);
        if (!ok)
            goto out;
        obj = js_NewObject(cx, &ExceptionClass, JSVAL_TO_OBJECT(pval), NULL);

where pval is a local jsval. This is GC safe only because it is not possible to redefine a getter for "prototype" property of the Error object. But this too fragile so the patch uses rval for bulletproof rooting.
Comment 9 Blake Kaplan (:mrbkap) 2006-05-22 17:59:42 PDT
Comment on attachment 222977 [details] [diff] [review]
Fix

Thanks for taking this.
Comment 10 Brendan Eich [:brendan] 2006-05-22 18:08:11 PDT
Comment on attachment 222977 [details] [diff] [review]
Fix

When will someone save us from jsfile.c by rewriting it with a better API and implementation?

/be
Comment 11 Igor Bukanov 2006-05-23 00:50:26 PDT
Created attachment 223003 [details] [diff] [review]
patch to commit

The commit for bug 326466 affected the patch in jsexn.c so here is an updated version.
Comment 12 Igor Bukanov 2006-05-23 00:55:16 PDT
I committed the patch from comment 11.
Comment 13 Igor Bukanov 2006-05-23 01:08:55 PDT
Created attachment 223006 [details] [diff] [review]
Patch for branch 1.8.1
Comment 14 Igor Bukanov 2006-05-23 01:26:06 PDT
Comment on attachment 223006 [details] [diff] [review]
Patch for branch 1.8.1

The patch applies to MOZILLA_1_8_0_BRANCH as is.
Comment 15 Igor Bukanov 2006-05-23 01:50:59 PDT
Created attachment 223007 [details] [diff] [review]
Patch for AVIARY_1_0_1 and MOZILLA_1_7 branches
Comment 16 Bob Clary [:bc:] 2006-05-25 13:48:22 PDT
verified fixed trunk 1.9a1 win/mac/linux
Comment 17 Igor Bukanov 2006-05-27 02:35:59 PDT
I committed the patch from comment 13 to MOZILLA_1_8_BRANCH
Comment 18 Daniel Veditz [:dveditz] 2006-05-27 19:03:35 PDT
We'll want this on the 1.8.0 branch as soon as the tree reopens after 1.8.0.4 ships.
Comment 19 Bob Clary [:bc:] 2006-05-29 11:57:44 PDT
verified fixed 1.8.1a3_2006052713 win/mac(intel|ppc)/linux
Comment 20 Daniel Veditz [:dveditz] 2006-06-05 11:27:27 PDT
Comment on attachment 223006 [details] [diff] [review]
Patch for branch 1.8.1

approved for 1.8.0 branch, a=dveditz for drivers
Comment 21 Igor Bukanov 2006-06-06 05:05:08 PDT
I committed the patch from comment 13 to MOZILLA_1_8_0_BRANCH.
Comment 22 Bob Clary [:bc:] 2006-06-10 16:43:49 PDT
I haven't been running this test with shutdown's new -03 version yet. :-(. 01 and 02 pass 20060609 win/macppc/linux 1.8.0.5, 1.8.1 and trunk. I don't see a crash on windows 1.8.0.5, 1.8.1 or trunk in the browser so marking tentatively verified fixed.
Comment 23 Bob Clary [:bc:] 2007-02-08 16:41:24 PST
/cvsroot/mozilla/js/tests/js1_5/extensions/regress-338804-01.js,v  <--  regress-338804-01.js

/cvsroot/mozilla/js/tests/js1_5/extensions/regress-338804-02.js,v  <--  regress-338804-02.js

/cvsroot/mozilla/js/tests/js1_5/extensions/regress-338804-03.js,v  <--  regress-338804-03.js

moved to extensions/ due to Script()

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