Closed Bug 338804 Opened 13 years ago Closed 13 years ago

GC hazards in constructor functions

Categories

(Core :: JavaScript Engine, defect, critical)

defect
Not set
critical

Tracking

()

VERIFIED FIXED

People

(Reporter: sync2d, Assigned: igor)

References

Details

(Keywords: crash, verified1.8.0.5, verified1.8.1, Whiteboard: [sg:critical?])

Attachments

(7 files, 2 obsolete files)

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
Attached file 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.
scale 3000, modified to call exploit inline, doesn't reliably crash trunk.
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?
Flags: in-testsuite+
Assignee: general → mrbkap
Blocks: sbb?
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: mrbkap → igor.bukanov
This is a version of the test case to reliably expose the problem in jsshell/browser.
This is a test case version that checks for both Script and RegExp problems.
Attachment #222975 - Attachment is obsolete: true
Attached patch Fix (obsolete) — Splinter Review
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+
Attached patch patch to commitSplinter Review
The commit for bug 326466 affected the patch in jsexn.c so here is an updated version.
Attachment #222977 - Attachment is obsolete: true
I committed the patch from comment 11.
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Attachment #223006 - Flags: approval-branch-1.8.1?(brendan)
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?
Attachment #223007 - Flags: approval1.7.14?
Attachment #223007 - Flags: approval-aviary1.0.9?
Attachment #223006 - Flags: approval-branch-1.8.1?(brendan) → approval-branch-1.8.1+
verified fixed trunk 1.9a1 win/mac/linux
Status: RESOLVED → VERIFIED
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+
verified fixed 1.8.1a3_2006052713 win/mac(intel|ppc)/linux
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+
I committed the patch from comment 13 to MOZILLA_1_8_0_BRANCH.
Keywords: fixed1.8.0.5
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.1
Keywords: fixed1.8.1
Group: security
Attachment #223007 - Attachment is obsolete: true
Attachment #223007 - Flags: approval1.7.14?
Attachment #223007 - Flags: approval-aviary1.0.9?
Attachment #223007 - Attachment is obsolete: false
/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.