GC hazards in constructor functions

VERIFIED FIXED

Status

()

Core
JavaScript Engine
--
critical
VERIFIED FIXED
11 years ago
10 years ago

People

(Reporter: shutdown, Assigned: Igor Bukanov)

Tracking

({crash, verified1.8.0.5, verified1.8.1})

Trunk
crash, verified1.8.0.5, verified1.8.1
Points:
---
Bug Flags:
blocking1.7.14 ?
blocking-aviary1.0.9 ?
blocking1.9a1 +
blocking1.8.1 +
blocking1.8.0.5 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical?])

Attachments

(7 attachments, 2 obsolete attachments)

(Reporter)

Description

11 years ago
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
(Reporter)

Comment 1

11 years ago
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

11 years ago
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

11 years ago
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?

Updated

11 years ago
Flags: in-testsuite+
Assignee: general → mrbkap
Blocks: 256195
Flags: blocking1.9a1+
Flags: blocking1.8.1+
Flags: blocking1.8.0.5?
Flags: blocking1.7.14?
Flags: blocking-aviary1.0.9?
Whiteboard: [sg:critical?]
Igor, could you take this one?  I think Blake is booked for this week, traveling Wednesday back to New York, and stuff.  Thanks,

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

Updated

11 years ago
Assignee: mrbkap → igor.bukanov
(Assignee)

Comment 6

11 years ago
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.
(Assignee)

Comment 7

11 years ago
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.
Attachment #222975 - Attachment is obsolete: true
(Assignee)

Comment 8

11 years ago
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.
Attachment #222977 - Flags: superreview?(brendan)
Attachment #222977 - Flags: review?(mrbkap)
Comment on attachment 222977 [details] [diff] [review]
Fix

Thanks for taking this.
Attachment #222977 - Flags: review?(mrbkap) → review+
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
Attachment #222977 - Flags: superreview?(brendan) → superreview+
(Assignee)

Comment 11

11 years ago
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.
Attachment #222977 - Attachment is obsolete: true
(Assignee)

Comment 12

11 years ago
I committed the patch from comment 11.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
(Assignee)

Comment 13

11 years ago
Created attachment 223006 [details] [diff] [review]
Patch for branch 1.8.1
Attachment #223006 - Flags: approval-branch-1.8.1?(brendan)
(Assignee)

Comment 14

11 years ago
Comment on attachment 223006 [details] [diff] [review]
Patch for branch 1.8.1

The patch applies to MOZILLA_1_8_0_BRANCH as is.
Attachment #223006 - Flags: approval1.8.0.5?
(Assignee)

Comment 15

11 years ago
Created attachment 223007 [details] [diff] [review]
Patch for AVIARY_1_0_1 and MOZILLA_1_7 branches
Attachment #223007 - Flags: approval1.7.14?
Attachment #223007 - Flags: approval-aviary1.0.9?

Updated

11 years ago
Attachment #223006 - Flags: approval-branch-1.8.1?(brendan) → approval-branch-1.8.1+

Comment 16

11 years ago
verified fixed trunk 1.9a1 win/mac/linux
Status: RESOLVED → VERIFIED
(Assignee)

Comment 17

11 years ago
I committed the patch from comment 13 to MOZILLA_1_8_BRANCH
Keywords: fixed1.8.1
We'll want this on the 1.8.0 branch as soon as the tree reopens after 1.8.0.4 ships.
Flags: blocking1.8.0.5? → blocking1.8.0.5+

Comment 19

11 years ago
verified fixed 1.8.1a3_2006052713 win/mac(intel|ppc)/linux
Keywords: fixed1.8.1 → verified1.8.1
Comment on attachment 223006 [details] [diff] [review]
Patch for branch 1.8.1

approved for 1.8.0 branch, a=dveditz for drivers
Attachment #223006 - Flags: approval1.8.0.5? → approval1.8.0.5+
(Assignee)

Comment 21

11 years ago
I committed the patch from comment 13 to MOZILLA_1_8_0_BRANCH.
Keywords: fixed1.8.0.5

Comment 22

11 years ago
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.
Keywords: fixed1.8.0.5 → verified1.8.0.5

Updated

11 years ago
Keywords: fixed1.8.1

Updated

11 years ago
Keywords: fixed1.8.1
Group: security
(Assignee)

Updated

10 years ago
Attachment #223007 - Attachment is obsolete: true
Attachment #223007 - Flags: approval1.7.14?
Attachment #223007 - Flags: approval-aviary1.0.9?
(Assignee)

Updated

10 years ago
Attachment #223007 - Attachment is obsolete: false

Comment 23

10 years ago
/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()
You need to log in before you can comment on or make changes to this bug.