630377 - Assertion failure: obj->isDenseArray() with evil sharp-variable use, array comprehension

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

function f(x) {
    x.q = 1;
}
#1=[f(#1#) for (i in [0])];

Assertion failure: obj->isDenseArray(), at js/src/jsarray.cpp:2032

The Right Way to fix this would be to make sharp-variable uses that expose the object before it's done SyntaxErrors. #1# should be legal only as a value in a plain ArrayLiteral or ObjectLiteral.

I would love to get that changed, but we don't know what it'll break. For now it'll be easier to weaken that assertion to a runtime check.

Comment 1

[Jason Orendorff [:jorendorff]]

Making this s-s for now. I'm not sure it's exploitable.

Comment 2

[Johnny Stenback (:jst)]

If this truly is a sg:critical bug, we need to fix it for 2.0. There's a testcase, seems it reproduces.

Comment 3

[Jason Orendorff [:jorendorff]]

I'm going to take a shot at making this a SyntaxError. Leaking objects to script before they're ready is just wrong. And scary.

If I can't make it work before lunch tomorrow, I'll give up and weaken the assertion to a runtime check.

Comment 4

[Jason Orendorff [:jorendorff]]

LOL, you can't
  var x = [#1="", #1#];
but you can
  var x = #1=Math.sin(#1#);

Clearly this should return the least fixed point of the function.

Comment 5

[Jason Orendorff [:jorendorff]]

Ah. The precedence there is (#1=Math).sin(#1#). If I parenthesize:

  var x = #1=(Math.sin(#1#));

then it throws an error. Not a SyntaxError; the program still parses and starts to execute. The bytecode is like this:

  00010:  getgname "Math"
  00013:  callprop "sin"
  00016:  usesharp 0 1
  00021:  call 1
  00024:  defsharp 0 1

At runtime, JSOP_USESHARP throws an exception since the corresponding JSOP_DEFSHARP hasn't executed.

Comment 6

[Jason Orendorff [:jorendorff]]

Here are the syntax changes I am implementing:

1. Sharp variable uses (#1#) will only be legal in two contexts:
       [HERE] and {x: HERE}
   These are the only two places where obj_toSource can emit them,
   in the absence of custom .toSource methods, as far as I can tell.

   So all these are errors:
       var v = #1#;                   // SyntaxError
       var v = #1=[], w = #1#;        // SyntaxError
       var v = #1=[(#1#)];            // SyntaxError
       var v = #1={x: 1, y: #1#.x};   // SyntaxError
       var v = #1=[f(#1#)];           // SyntaxError
       v = #1=['x', #1#[0] + 'y'];    // SyntaxError

2. A sharp variable def (#1=) must be immediately followed by an
   ArrayLiteral or ObjectLiteral.
       var v = #1=([#1#]);            // SyntaxError
       var v = #1=[#1#];              // OK
       var v = #1=(Math.sin(#1#));    // SyntaxError
       v = [#1=function () {}, #1#];  // SyntaxError
   Array comprehensions are excluded.
       var v = #1=[#1# for (x in a)]; // SyntaxError

3. Sharp variable defs (#1=[]) are AssignmentExpressions,
   not PrimaryExpressions. So:
       x = #1=[#1#];                  // OK
       uneval(#1={next: #1#});        // OK
       print(#1=[].length);           // SyntaxError
       print((#1=[]).length);         // OK
       x = #1=a == 0 ? [] : [#1#];    // SyntaxError
       x = #1=(a == 0 ? [] : [#1#]);  // SyntaxError
       x = a == 0 ? [] : #1=[#1#];    // OK
       Math.#1=PI;                    // SyntaxError and go directly to jail

I confidently claim nobody is using the syntax I'm banning. Maybe a fuzzer somewhere. We have nothing to lose but some attack surface.

If I had time, I would implement additional restrictions to make these SyntaxErrors too:

       x = [#42#];                    // wrong use - runtime Error for now
       x = #1=[function () [#1#]];    // wrong use - function throws for now
       [#1=["a"], #1#, #1=["b"], #1#] // multiple defs - OK for now
       print(#1={}, [#1#]);           // unrelated exprs - OK for now
       x = #1=[f([#1#])];             // #1# escapes! - OK for now

The last one is evil and I really wish I could kill it off, but it looks like that would require more invasive changes.

Comment 7

[Jason Orendorff [:jorendorff]]

Attachment: v1

Comment 8

[Jason Orendorff [:jorendorff]]

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

Brendan asked me to look into a narrower fix. I'll try weakening the assertion to an if-statement.

Comment 9

[Jason Orendorff [:jorendorff]]

Attachment: simpler

Comment 10

[Brendan Eich [:brendan]]

Comment on attachment 512318 [details] [diff] [review]
simpler

Looks like the v1 patch from bug 633637.

/be

Comment 11

[Brendan Eich [:brendan]]

(In reply to comment #8)
> Comment on attachment 511795 [details] [diff] [review]
> v1
> 
> Brendan asked me to look into a narrower fix.

The idea is to avoid regressing bug 335051 and making overlarge incompatible changes so late in the cycle.

Bug 486643 is about "deprecating sharp variables". Deprecation precedes removal (obsolescence) so that'll have to be later. Harmony WeakMaps give users the ability to implement toSource and uneval efficiently in JS itself. The Common Lisp inspired sharp syntax could be ditched in favor of something else that preserves alpha equivalence. The parsing extensions to match could perhaps come about via the quasis extension mechanism.

But this is all way, wayyyyy beyond this bug, and it arguably wouldn't entail any restrictions on sharp def parsing, _per se_.

/be

Comment 12

[Jason Orendorff [:jorendorff]]

Attachment: simpler

Sigh, how'd that get in there. Here's the right patch.

Comment 13

[Brendan Eich [:brendan]]

Comment on attachment 512488 [details] [diff] [review]
simpler

>+    uint32_t length = obj->getArrayLength();

uint32 not uint32_t -- match the return type and avoid the _t tax (I see it was pre-existing below).

>+    if (obj->isSlowArray()) {
>+        /* This can happen in one evil case. See bug 630377. */
>+        jsid id;
>+        return js_IndexToId(cx, length, &id) &&
>+               js_DefineProperty(cx, obj, id, &v, NULL, NULL, JSPROP_ENUMERATE);
>+    }
>+
>     JS_ASSERT(obj->isDenseArray());
>-    uint32_t length = obj->getArrayLength();
>     JS_ASSERT(length <= obj->getDenseArrayCapacity());
> 

It looks like jstracer.cpp needs to change as well.

/be

Comment 14

[Jason Orendorff [:jorendorff]]

Attachment: v3

Comment 15

[Johnathan Nightingale [:johnath]]

Is this [can land]?

Comment 16

[Jason Orendorff [:jorendorff]]

It went in last night, sorry.

http://hg.mozilla.org/tracemonkey/rev/a4594bca8dd2

Comment 17

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/a4594bca8dd2