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

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
2 years ago

People

(Reporter: jorendorff, Assigned: dmandelin)

Tracking

(Blocks: 1 bug, {assertion, regression, testcase})

Other Branch
assertion, regression, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

(Whiteboard: [sg:critical?][hardblocker][fixed-in-tracemonkey][has patch])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

6 years ago
var t;
(function () {
    t = (function() {
            yield k();
        })();
    function h() {
    }
    function k() {
        return function() { h(); };
    }
})();
t.next();

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.
(Reporter)

Comment 1

6 years ago
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.

Updated

6 years ago
Blocks: 593743
(Reporter)

Comment 2

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

Updated

6 years ago
blocking2.0: --- → betaN+
Whiteboard: [sg:critical?][hardblocker]
(Assignee)

Comment 3

6 years ago
Created attachment 509635 [details] [diff] [review]
v(-1)

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

Comment 4

6 years ago
Created attachment 509654 [details] [diff] [review]
v1 (just adds test case)

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
Status: NEW → ASSIGNED
Attachment #509654 - Flags: review?(jorendorff)

Updated

6 years ago
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.
Blocks: 558451
Keywords: assertion, regression, testcase
Comment on attachment 509654 [details] [diff] [review]
v1 (just adds test case)

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

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

Comment 7

6 years ago
http://hg.mozilla.org/tracemonkey/rev/a46dd602c30d

(with added comment.)
Whiteboard: [sg:critical?][hardblocker][has patch] → [sg:critical?][hardblocker][fixed-in-tracemonkey]

Updated

6 years ago
Whiteboard: [sg:critical?][hardblocker][fixed-in-tracemonkey] → [sg:critical?][hardblocker][fixed-in-tracemonkey][has patch]
cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/a46dd602c30d
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Group: core-security
You need to log in before you can comment on or make changes to this bug.