563221 - "Assertion failure: ss->top >= nuses," or "Assertion failure: top != 0,"

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

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

function f(code) {
  code = code.replace(/\/\*DUPTRY\d+/,
  function(k) {
    n = (k.substr(8))
    return g("try{}catch(e){}", n)
  })
  var f = new Function(code)
  uneval(f)
}
function g(s, n) {
  if (n == 1) return s
  s2 = s + s
  r = n % 2
  d = (n - r) / 2
  m = g(s2, d)
  return r ? s: m
}
f("if([]){}else if(\"\"()){}else{/*DUPTRY2721}")


asserts js debug shell on TM tip without -j at Assertion failure: ss->top >= nuses, at ../jsopcode.cpp:1974

Comment 1

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

(Not the smallest) regression window:

01012006 js shell does not assert.
A 07 Nov 2008 js shell build (changeset http://hg.mozilla.org/tracemonkey/rev/04c360f123e5 ) seems to assert.

Comment 2

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

function f(code) {
  code = code.replace(/\/\*DUPTRY\d+/,
  function(k) {
    n = (k.substr(8))
    return g("try{}catch(e){}", n)
  })
  var f = new Function(code)
  uneval(f)
}
function g(s, n) {
  if (n == 1) return s
  s2 = s + s
  r = n % 2
  d = (n - r) / 2
  m = g(s2, d)
  return r ? s: m
}
f("if(x);else if(\"\".*){(n(\"\"()))}else{/*DUPTRY5683}")

asserts at Assertion failure: top != 0, at ../jsopcode.cpp:1030 instead

Comment 3

[Jason Orendorff [:jorendorff]]

"" + eval("(function () { if (x) ; else if (y) n(); else { " + Array(5684).join("try{}catch(e){}") + " } });");

flags: LAMBDA NULL_CLOSURE
main:
00000:  trace
00001:  name "x"
00004:  ifeq 12 (8)
00007:  gotox 102325 (102318)
00012:  name "y"
00015:  ifeq 31 (16)
00018:  callname "n"
00021:  call 0
00024:  trace
00025:  pop
00026:  gotox 102325 (102299)
00031:  try
....
102325:  stop

Comment 4

[Jason Orendorff [:jorendorff]]

As usual with these, it's not try/catch-specific. Any large chunk of bytecode will do.

"" + eval("(function () { if (x) ; else if (y) n(); else { " +
          Array(55000).join("e;") +
          "}});");

Comment 5

[Andreas Gal :gal]

Hiding for further analysis whether this can be exploited.

Comment 6

[Jason Orendorff [:jorendorff]]

Likely exploitable. The decompiler jumps to a bad bytecode offset and continues decompiling.

Comment 7

[Jason Orendorff [:jorendorff]]

Attachment: v1

I think this is the right fix.

Comment 8

[Andreas Gal :gal]

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

Can you explain the fix? This happens because oplen is unsigned? Or what goes on here?

Comment 9

[Brendan Eich [:brendan]]

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

This leaves oplen possibly wrong, although oplen2 does avoid setting it to an also-possibly-wrong value with which to goto if_again.

One line fix is to set oplen = js_CodeSpec[*pc].length; right before the goto if_again (best if immediately after pc += cond; to be precise).

/be

Comment 10

[Brendan Eich [:brendan]]

For DecompileDestructuring, I added LOAD_OP_DATA to get every local depending on an updated pc fetched and up-to-date. But this was back when we used C and hunted with spears and snowshoes (so if the snow was too light we'd fail to catch up to the Moose, and then we'd starve :-/).

/be

Comment 11

[Brendan Eich [:brendan]]

I'm pretty sure this is not exploitable.

/be

Comment 12

[Jason Orendorff [:jorendorff]]

Attachment: v2

Yes, that's much better.

Comment 13

[Jason Orendorff [:jorendorff]]

(In reply to comment #8)
> (From update of attachment 443325 [details] [diff] [review])
> Can you explain the fix? This happens because oplen is unsigned? Or what goes
> on here?

oplen is always supposed to be the length of the current opcode. The existing code changes pc (to point to an IFNE/IFEQ opcode) without updating oplen, which still points to a GOTOX opcode.

GOTO and IFNE/IFEQ are all the same length (3 bytes), which is why this bug hasn't surfaced before; but GOTOX is 5 bytes.

The result was that the decompiler jumped into a bad offset between opcodes and started decompiling. Brendan: Andreas and I have been treating this kind of bug as exploitable for the past few days, since the latest batch of decompiler fuzzbugs. It seems like you'd have to analyze the decompiler pretty closely to make sure it's not possible to use it as an interpreter to do whatever you want to the heap at least.

Comment 14

[Brendan Eich [:brendan]]

The decompiler reads and writes memory, so perhaps there's an exploit, but unlike the rest of the codebase, it has always-on sanity checks that cause it to bail out. We can restrict this bug since it will be hard to prove non-exploitability, but looking at how the optimized shell fails can be illuminating:

$ ./Darwin_OPT.OBJ/js 
js> "" + eval("(function () { if (x) ; else if (y) n(); else { " + Array(10000).join("e;") + " } });");
js> 

No crash.

/be

Comment 15

[Brendan Eich [:brendan]]

Comment on attachment 443408 [details] [diff] [review]
v2

Note how the decompiler *writes* only by pushing, and pushing beyond budgeted stack depth fails with OOM:

    JS_ASSERT(top < StackDepth(ss->printer->script));
    if (top >= StackDepth(ss->printer->script)) {
        JS_ReportOutOfMemory(ss->sprinter.context);
        return JS_FALSE;
    }

Likewise, underflowing the stack fails:

    /* ss->top points to the next free slot; be paranoid about underflow. */
    top = ss->top;
    JS_ASSERT(top != 0);
    if (top == 0)
        return 0;

The sprinter autogrows and checks for malloc failure, so the return 0 may cause overwriting, i.e., invalid decompiler output, but not exploitable memory corruption.

I continue to believe that this bug is not exploitable.

/be

Comment 16

[Brendan Eich [:brendan]]

(In reply to comment #15)
> Likewise, underflowing the stack fails:
> 
>     /* ss->top points to the next free slot; be paranoid about underflow. */
>     top = ss->top;
>     JS_ASSERT(top != 0);
>     if (top == 0)
>         return 0;

This means OFF2STR returns the empty string at the base of the sprinter. IOW, pop is a read, not a write.

Reading off the end of bytecode could be a privacy leak, although in the case of this bug the bytecode limit pointer (endpc) stops that. Other decompiler bugs have miscalculated endpc, so I thought I'd mention it.

/be

Comment 17

[Jason Orendorff [:jorendorff]]

Thanks for the explanation.

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

Comment 18

[Robert Sayre]

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

Comment 19

[Daniel Veditz [:dveditz]]

1.9.1/1.9.2 branches also assert. Given the simple patch and checked-in testcase we might as well fix this on branches.

Comment 20

[Christian Holler (:decoder)]

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

Comment 21

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

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