546615 - Crash [@ BindNameToSlot] or "Assertion failure: cg->staticLevel >= level, at ../jsemit.cpp"

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

function spaces(n) {
  if (n == 0) return ""
  if (n == 1) return " "
  var w = spaces(Math.floor(n / 2));
  return w + w + spaces(n % 2)
}
s = "if(<y {l}={(0)[(*\n)]}/>)/**/if(x(function(){})(x for(x in x))){}";
s = s.replace("/**/", "/*" + spaces(228) + "*/");
eval(s);


crashes js opt build at BindNameToSlot near null on TM tip without -j, and asserts js debug build at Assertion failure: cg->staticLevel >= level, at ../jsemit.cpp:2171 on TM tip without -j.

Turning security-sensitive because I'm not sure how bad this is, though it's crashing near null.

autoBisect shows this is probably related to bug 452498:

The first bad revision is:
changeset:   26784:2cf0bbe3772a
user:        Brendan Eich
date:        Sun Apr 05 21:17:22 2009 -0700
summary:     upvar2, aka the big one take 2 (452498, r=mrbkap).


js shell opt crash stack:

Exception Type:  EXC_BAD_ACCESS (SIGBUS)
Exception Codes: KERN_PROTECTION_FAILURE at 0x000000000000000c
Crashed Thread:  0  Dispatch queue: com.apple.main-thread

Thread 0 Crashed:  Dispatch queue: com.apple.main-thread
0   js-opt-32-tm-darwin           	0x000325b7 BindNameToSlot(JSContext*, JSCodeGenerator*, JSParseNode*) + 663
1   js-opt-32-tm-darwin           	0x0003442c EmitNameOp(JSContext*, JSCodeGenerator*, JSParseNode*, int) + 28
2   js-opt-32-tm-darwin           	0x0003baeb js_EmitTree + 23707
3   js-opt-32-tm-darwin           	0x00038838 js_EmitTree + 10728
4   js-opt-32-tm-darwin           	0x00036bb6 js_EmitTree + 3430
5   js-opt-32-tm-darwin           	0x0003a4ea js_EmitTree + 18074
6   js-opt-32-tm-darwin           	0x000a35ae JSCompiler::compileScript(JSContext*, JSObject*, JSStackFrame*, JSPrincipals*, unsigned int, unsigned short const*, unsigned long, __sFILE*, char const*, unsigned int, JSString*, unsigned int) + 990
7   js-opt-32-tm-darwin           	0x00073e4a obj_eval(JSContext*, JSObject*, unsigned int, long*, long*) + 2330
8   js-opt-32-tm-darwin           	0x0005fdbc js_Invoke + 1180
9   js-opt-32-tm-darwin           	0x0004fe51 js_Interpret + 2209
10  js-opt-32-tm-darwin           	0x0005f5d1 js_Execute + 625
11  js-opt-32-tm-darwin           	0x0000d7bc JS_ExecuteScript + 60
12  js-opt-32-tm-darwin           	0x00004658 Process(JSContext*, JSObject*, char*, int) + 1336
13  js-opt-32-tm-darwin           	0x000085e6 main + 1734
14  js-opt-32-tm-darwin           	0x000024fd _start + 208
15  js-opt-32-tm-darwin           	0x0000242c start + 40

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Yes, this affects 1.9.1 and 1.9.2 js shell builds, and I could also use this to make a testcase that crashes Firefox 3.6 on WinXP SP3.

Comment 2

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: HTML testcase

bp-c64c56f9-44a4-40f6-aa6e-b658e2100217

Comment 3

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Comment on attachment 427322 [details]
HTML testcase

Seems to crash on load so switching to text/plain.

Comment 4

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

A similar assert is in bug 533862 but they might not be related.

Comment 5

[Jason Orendorff [:jorendorff]]

OK, it's not recursion or eval:

if(<y l={0
}/>)/*123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678*/if(x(function(){})(x
for(x in x)));

Comment 6

[Jason Orendorff [:jorendorff]]

How do you find this stuff?! It turns out that when we do this:

    oldflags = ts->flags;
    ts->flags = oldflags & ~TSF_XMLTAGMODE;
    pn2 = Expr(cx, ts, tc);
    if (!pn2)
        return NULL;

    MUST_MATCH_TOKEN(TOK_RC, JSMSG_CURLY_IN_XML_EXPR);
    ts->flags = oldflags;

the last line there can unintentionally clobber TSF_NLFLAG. The confusion about whether we saw a newline or not in turn causes the column numbers in token positions to be wrong for long lines, which in turn causes CompExprTransplanter to get confused and adjust the lexical depth of something that shouldn't be adjusted, which results in an assertion down the road.

Fix coming.

Comment 7

[Jason Orendorff [:jorendorff]]

Attachment: v1

It looks like this is the only place where we adjust ts->flags this way. Test case included.

Comment 8

[Jason Orendorff [:jorendorff]]

BTW, the spaces function can be implemented like this (unless you're just trying to test recursion).

    function spaces(n) {
        return Array(n+1).join(" ");
    }

Comment 10

[Brendan Eich [:brendan]]

Comment on attachment 427628 [details] [diff] [review]
v1

Thanks.

/be

Comment 11

[Jeff Walden [:Waldo]]

More spaces-like fun, for anyone who feels like arguing algorithms or code  (well, don't actually argue, I mention this only for pedagogical benefit ;-) ):

http://www.davidflanagan.com/2009/08/string-multipli.html
http://www.davidflanagan.com/2009/08/good-algorithms.html

Comment 12

[Jason Orendorff [:jorendorff]]

http://hg.mozilla.org/tracemonkey/rev/36487442aeb0

Comment 13

[Daniel Veditz [:dveditz]]

Since this has been deemed not a security issue it's not a security-update blocker, but if you want to fix this crash on the old branches please request approval on the patch.

Comment 14

[Bob Clary [:bc] (inactive)]

fixed manifest http://hg.mozilla.org/tracemonkey/rev/3970759ca2c1

Comment 16

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/36487442aeb0

Comment 17

[Christian Holler (:decoder)]

A testcase for this bug was automatically identified at js/src/tests/js1_8_5/regress/regress-546615.js.

Comment 18

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Testcases have been landed by virtue of being marked in-testsuite+ -> VERIFIED as well.