Closed Bug 631082 Opened 9 years ago Closed 9 years ago

Assertion failure: fp && fp->isScriptFrame(), at ../jscntxt.h:1830 @findFrameAtLevel with generator on the stack


(Core :: JavaScript Engine, defect)

Other Branch
Not set



Tracking Status
blocking2.0 --- betaN+


(Reporter: jorendorff, Assigned: dmandelin)


(Blocks 1 open bug)


(Keywords: assertion, regression, testcase, Whiteboard: [sg:critical?][hardblocker][fixed-in-tracemonkey][has patch])


(1 file, 1 obsolete file)

var t;
(function () {
    t = (function() {
            yield k();
    function h() {
    function k() {
        return function() { h(); };

Assertion failure: fp && fp->isScriptFrame(), at ../jscntxt.h:1830

The compiler believes that neither k nor its caller can escape the enclosing function, and therefore any time k is running, the enclosing function is on the stack. This is wrong because k's caller is a generator-function, and the generator-iterator does escape.
In an opt build, this test case would crash in findFrameAtLevel() near null. But you could easily arrange for findFrameAtLevel() to find a frame with the appropriate level. Having accomplished that you could probably use the code to read arbitrary slots from the js operand stack and possibly past the end of it. I'm not sure it's exploitable but it doesn't seem good; therefore setting the s-g bit.
(Btw, db48x ran across this while writing actual code to do something useful. The test case in comment 0 is reduced from about:startup, bug 593743, which this bug now blocks.)
blocking2.0: --- → betaN+
Whiteboard: [sg:critical?][hardblocker]
Attached patch v(-1) (obsolete) — Splinter Review
Well, this fixes this particular test case. I'm not sure it's a complete fix, though, partly because I don't understand the code I patched and partly because it seems to have relied on TCF_FUN_HEAVYWEIGHT being propagated outward, and I assume TCF_FUN_IS_GENERATOR is not similarly propagated.
I think the v(-1) patch is actually correct. This is just that patch plus the test case. Argument for why it works:

The basic idea is to assume that generator functions always escape, as a conservative approximation. (I figured it would be hard to teach the analysis to track down exactly where such functions are called, and then see exactly where that result flowed, so it would be better to say they always escape.)

From there, the existing analysis does the rest, given the point where I encoded that assumption. That point marks the generator function and all its kids (immediately enclosed functions) as escaping. In general, when the analysis marks any function |f| as escaping, it also marks as escaping any functions that are defined or referred to inside |f|.

In the test case, this plays out as follows: 

1. We the lambda inside k as escaping (it gets processed first) and enqueue it.
2. We mark the generator as escaping and enqueue it.
3. When we dequeue the lambda, we mark h as escaping because h is referred to inside the lambda.
4. When we dequeue the generator, we mark k as escaping because k is referred to inside the lambda.
Assignee: general → dmandelin
Attachment #509635 - Attachment is obsolete: true
Attachment #509654 - Flags: review?(jorendorff)
Whiteboard: [sg:critical?][hardblocker] → [sg:critical?][hardblocker][has patch]
Not sure how reliable this is, but autoBisect (on windows) pinpoints:

Due to skipped revisions, the first bad revision could be any of:
changeset:   51604:e5958cd4a135
user:        Brendan Eich
date:        Sun Aug 29 11:57:08 2010 -0700
summary:     Merge JSScope into JSObject and JSScopeProperty (now js::Shape; bug 558451, r=jorendorff).

changeset:   52694:d575f16c7f55
parent:      52693:ff1a35b9e9b8
parent:      51604:e5958cd4a135
user:        David Mandelin
date:        Mon Aug 30 15:13:32 2010 -0700
summary:     [JAEGER] Merge from Tracemonkey.

changeset:   52695:01a86ce240da
user:        David Anderson
date:        Mon Aug 30 15:17:18 2010 -0700
summary:     [JAEGER] Silence GCC warning about signed integer comparisons.
Comment on attachment 509654 [details] [diff] [review]
v1 (just adds test case)

Add a short para about generators to the comment, and r=me.

Attachment #509654 - Flags: review?(jorendorff) → review+

(with added comment.)
Whiteboard: [sg:critical?][hardblocker][has patch] → [sg:critical?][hardblocker][fixed-in-tracemonkey]
Whiteboard: [sg:critical?][hardblocker][fixed-in-tracemonkey] → [sg:critical?][hardblocker][fixed-in-tracemonkey][has patch]
Closed: 9 years ago
Resolution: --- → FIXED
Group: core-security
You need to log in before you can comment on or make changes to this bug.