Closed Bug 631082 Opened 14 years ago Closed 14 years ago

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

Categories

(Core :: JavaScript Engine, defect)

Other Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: jorendorff, Assigned: dmandelin)

References

(Blocks 1 open bug)

Details

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

Attachments

(1 file, 1 obsolete file)

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.
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) (deleted) — 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.
Attached patch v1 (just adds test case) (deleted) — Splinter Review
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)
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. /be
Attachment #509654 - Flags: review?(jorendorff) → review+
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]
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Group: core-security
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: