338804 - GC hazards in constructor functions

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[shutdown]

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]

Attachment: 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] (inactive)]

Attachment: 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] (inactive)]

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

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]]

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

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

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

Attachment: 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) (inactive)]

Comment on attachment 222977 [details] [diff] [review]
Fix

Thanks for taking this.

Comment 10

[Brendan Eich [:brendan]]

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]

Attachment: patch to commit

The commit for bug 326466 affected the patch in jsexn.c so here is an updated version.

Comment 12

[Igor Bukanov]

I committed the patch from comment 11.

Comment 13

[Igor Bukanov]

Attachment: Patch for branch 1.8.1

Comment 14

[Igor Bukanov]

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]

Attachment: Patch for AVIARY_1_0_1 and MOZILLA_1_7 branches

Comment 16

[Bob Clary [:bc] (inactive)]

verified fixed trunk 1.9a1 win/mac/linux

Comment 17

[Igor Bukanov]

I committed the patch from comment 13 to MOZILLA_1_8_BRANCH

Comment 18

[Daniel Veditz [:dveditz]]

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] (inactive)]

verified fixed 1.8.1a3_2006052713 win/mac(intel|ppc)/linux

Comment 20

[Daniel Veditz [:dveditz]]

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]

I committed the patch from comment 13 to MOZILLA_1_8_0_BRANCH.

Comment 22

[Bob Clary [:bc] (inactive)]

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] (inactive)]

/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()