1149915 - Visiting kangax.github.io/compat-table/es6 crashes the browser

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Georg Koppen]

A vanilla ESR 31.6.0 is crashing reliably if I visit kangax.github.io/compat-table/es6.

The regression range is: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=e86b84998b18&tochange=a19e0434ea52

I have at least a stacktrace from a Tor Browser build:

Program received signal SIGSEGV, Segmentation fault.
DecompileExpressionFromStack(JSContext*, int, int, JS::Handle<JS::Value>, char**) () at /home/ubuntu/build/tor-browser/js/src/jsopcode.cpp:1696
1696	/home/ubuntu/build/tor-browser/js/src/jsopcode.cpp: No such file or directory.
(gdb) bt
#0  DecompileExpressionFromStack(JSContext*, int, int, JS::Handle<JS::Value>, char**) () at /home/ubuntu/build/tor-browser/js/src/jsopcode.cpp:1696
#1  0x00007ffff4938480 in js::DecompileValueGenerator(JSContext*, int, JS::Handle<JS::Value>, JS::Handle<JSString*>, int) ()
    at /home/ubuntu/build/tor-browser/js/src/jsopcode.cpp:1785
#2  0x00007ffff48b14c3 in js_ReportValueErrorFlags(JSContext*, unsigned int, unsigned int, int, JS::Handle<JS::Value>, JS::Handle<JSString*>, char const*, char const*) () at /home/ubuntu/build/tor-browser/js/src/jscntxt.cpp:978
#3  0x00007ffff49f4a65 in js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) () at /home/ubuntu/build/tor-browser/js/src/vm/Interpreter.cpp:353
#4  0x00007ffff49e798a in Interpret(JSContext*, js::RunState&) ()
    at /home/ubuntu/build/tor-browser/js/src/vm/Interpreter.cpp:2620
#5  0x00007ffff49f4739 in js::RunScript(JSContext*, js::RunState&) ()
    at /home/ubuntu/build/tor-browser/js/src/vm/Interpreter.cpp:422
#6  0x00007ffff48fdab0 in bool NativeMethod<js::StarGeneratorObject, &(star_generator_throw(JSContext*, JS::CallArgs))>(JSContext*, unsigned int, JS::Value*)
    () at /home/ubuntu/build/tor-browser/js/src/jsiter.cpp:1775
#7  0x00007ffff49f4aae in js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) () at /home/ubuntu/build/tor-browser/js/src/jscntxtinlines.h:239
#8  0x00007ffff49e798a in Interpret(JSContext*, js::RunState&) ()
    at /home/ubuntu/build/tor-browser/js/src/vm/Interpreter.cpp:2620
#9  0x00007ffff49f4739 in js::RunScript(JSContext*, js::RunState&) ()
    at /home/ubuntu/build/tor-browser/js/src/vm/Interpreter.cpp:422

The original bug report is: https://trac.torproject.org/projects/tor/ticket/15526

Not sure how severe this problem actually is, thus embargoing this bug for now.

Comment 1

[Georg Koppen]

Oh, "regression range" might be the wrong term here as this problem actually got fixed between those two changesets on trunk.

Comment 2

[Boris Zbarsky [:bzbarsky]]

[Tracking Requested - why for this release]:

Just tried this in a debug ESR31 build and I get:

Assertion failure: codeArray_[offset], at ../../../mozilla/js/src/jsopcode.cpp:398

(anonymous namespace)::BytecodeParser::getCode (this=0x7fff5fbf4858, offset=57) at ../../../mozilla/js/src/jsopcode.cpp:398
398             JS_ASSERT(codeArray_[offset]);

"offset" is 57.  The stack has, in addition to the comment 0 bits:

#0  (anonymous namespace)::BytecodeParser::getCode (this=0x7fff5fbf4858, offset=57) at ../../../mozilla/js/src/jsopcode.cpp:398
#1  0x0000000106415fbb in (anonymous namespace)::BytecodeParser::stackDepthAtPC (this=0x7fff5fbf4858, offset=57) at ../../../mozilla/js/src/jsopcode.cpp:357
#2  0x000000010640dd90 in (anonymous namespace)::BytecodeParser::stackDepthAtPC (this=0x7fff5fbf4858, pc=0x112e14862 ":") at ../../../mozilla/js/src/jsopcode.cpp:359
#3  0x00000001064158d3 in FindStartPC (cx=0x1270cd8b0, iter=@0x7fff5fbf4ab8, spindex=-3, skipStackHits=0, v={data = {asBits = 18444773748872577024, debugView = {payload47 = 0, tag = JSVAL_TAG_UNDEFINED}, s = {payload = {i32 = 0, u32 = 0, why = JS_ELEMENTS_HOLE}}, asDouble = -nan(0x9000000000000), asPtr = 0xfff9000000000000, asWord = 18444773748872577024, asUIntPtr = 18444773748872577024}}, valuepc=0x7fff5fbf4a70) at ../../../mozilla/js/src/jsopcode.cpp:1696
#4  0x0000000106410dd4 in DecompileExpressionFromStack (cx=0x1270cd8b0, spindex=-3, skipStackHits=0, v={<js::HandleBase<JS::Value>> = {<js::ValueOperations<JS::Handle<JS::Value> >> = {<No data fields>}, <No data fields>}, ptr = 0x1254e4bc8}, res=0x7fff5fbf5050) at ../../../mozilla/js/src/jsopcode.cpp:1764

The script running at this point is:

  var closed = false;
  var iter = __createIterableObject(1, 2, 3);
  iter['throw'] = undefined;
  iter['return'] = function(){
    closed = true;
    return {done: true};
  }
  var gen = (function*(){
    try {
      yield *iter;
    } catch(e){}
  })();
  gen.next();
  gen['throw']();
  return closed;

In any case, I bet somehow the Symbol changes in the changelog fixed this, though they don't seem directly related...

Comment 3

[Boris Zbarsky [:bzbarsky]]

Oh, and the pc is pointing to JSOP_CALL.  Presumably the issue is that pcToOffset(pc) screwed up?  script->length() is 152, so 57 is not out of bounds or anything, but codeArray_ sure has a lot of null pointers in it...

Comment 4

[Jason Orendorff [:jorendorff]]

Bisecting.

Comment 5

[Jason Orendorff [:jorendorff]]

Maybe I misunderstood comment 0. Both endpoints of the range given seem to have the bug, so I'm extending my search (forward in time) to try to find a good revision. Here's the shell testcase:

function *__createIterableObject() {
    yield 1;
}
function f() {
  var iter = __createIterableObject();
  iter['throw'] = undefined;
  var gen = (function*(){
    try {
      yield *iter;
    } catch(e){}
  })();
  gen.next();
  gen.throw();
}
f();

Comment 6

[Jason Orendorff [:jorendorff]]

I suspect this was fixed by bug 987560, a large patch. In that case there's no backporting it. I'll just have to debug it.

Comment 7

[Jason Orendorff [:jorendorff]]

(In reply to Jason Orendorff [:jorendorff] from comment #6)
> I suspect this was fixed by bug 987560, a large patch.

Confirmed. I'll debug this tomorrow.

Comment 8

[Jason Orendorff [:jorendorff]]

Sequence of events leading to the crash:

1.  `gen.throw()` sends an exception to a generator that is suspended at `yield* iter;`.

    Per spec, this is supposed to call `iter.throw()`.

2.  `iter.throw` is undefined.
    -> ReportIsNotFunction
    -> DecompileValueGenerator
    -> FindStartPC

3.  FindStartPC creates a BytecodeParser which walks the script's bytecode.
    The intent of this code is to collect info on every opcode in the script.

    It reaches a JSOP_TRY op, part of the desugaring of `yield* iter`.

    Now, normally each JSOP_TRY op has a corresponding "try note" which the BytecodeParser uses
    to figure out where the catch block begins. In this case, the try note has an off-by-one error.
    BytecodeParser therefore does not find it, and thus doesn't parse any of the
    bytecode in the catch block.

    Which is where the `iter.throw()` call is.

4.  So when FindStartPC asks for the stack depth at the pc where the error occurred,
    the BytecodeParser has no record that there was an opcode at that pc.
    -> crash

The crash is at a fixed small offset from NULL, so it's not exploitable.

Now for the cause:

0.  I think this is because we emit JSOP_GENERATOR in the generator script's prologue (spelled
    "prolog" at the time).

    Bug 987560 (the large patch which fixes this bug) changed it so that generators no longer have
    a script prologue.

    Note that FinishTakingSrcNotes adjusts the offset of every srcnote in the main section,
    adding a number of bytes equal to the size of the prologue. I don't see corresponding code
    for try notes. So all bytecode offsets in main-section try notes, following a non-empty prologue,
    will be off by some amount.

    The code in EmitYieldStar has a counter-bug, probably inserted during testing,
    so that the try note is accurate enough for the interpreter's try-catch mechanism. :(

We may still have this bug, in scripts that have a prologue.

The easiest fix to backport would be to bail out if the BytecodeParser failed to find the current op in the script. We already have fallback code for cases where the expression decompiler doesn't work.

Comment 9

[Georg Koppen]

(In reply to Jason Orendorff [:jorendorff] from comment #5)
> Maybe I misunderstood comment 0. Both endpoints of the range given seem to

Not sure if we are talking about the same thing then but using mozregression gives me the "regression" range above: the nightly from 2014-06-25 is the first that is *not* crashing anymore on my 64bit Linux test system (just tested it again).

Comment 10

[Andrew McCreight [:mccr8]]

(In reply to Jason Orendorff [:jorendorff] from comment #8)
> The crash is at a fixed small offset from NULL, so it's not exploitable.

Setting to sec-low.

Comment 11

[Jason Orendorff [:jorendorff]]

(In reply to Georg Koppen from comment #9)
> Not sure if we are talking about the same thing then but using mozregression
> gives me the "regression" range above: the nightly from 2014-06-25 is the
> first that is *not* crashing anymore on my 64bit Linux test system (just
> tested it again).

Partly figured this out. This generator test, delightfully, feature-tests to see if Symbol() is supported, and if so, it runs some different code. The symbols-supported code apparently does not trigger the bug, though I'm still not sure why not. Hope to get back to this today.

Comment 12

[Al Billings [:abillings - ex-MoCo]]

Benjamin, why is this tracking for 38+ for ESR31? We don't normally take sec-low issues on ESR branches.

Comment 13

[Benjamin Kerensa [:bkerensa]]

(In reply to Al Billings [:abillings] from comment #12)
> Benjamin, why is this tracking for 38+ for ESR31? We don't normally take
> sec-low issues on ESR branches.

If this was going to be a stability issue for Tor considering our recent
support for their project I would have asked us to make an exception. But
looking at the downstream bug it seems this will be sorted out on its own.

Comment 14

[Georg Koppen]

(In reply to Benjamin Kerensa [:bkerensa] from comment #13)
> (In reply to Al Billings [:abillings] from comment #12)
> > Benjamin, why is this tracking for 38+ for ESR31? We don't normally take
> > sec-low issues on ESR branches.
> 
> If this was going to be a stability issue for Tor considering our recent
> support for their project I would have asked us to make an exception. But
> looking at the downstream bug it seems this will be sorted out on its own.

Well, sorting out means in this case just crossing our fingers and wait until we switch to ESR 38 (which is supposed to happen in August) as we don't have the capacity to fix that on our own. I was just echoing Al Billings in our bug anticipating no official fix as this is "just "sec-low" (the possibility to crash a Tor Browser maybe as part of a targeted attack bothers me though).

That said if there were a fix for this even if it would not make it into an official ESR 31 I guess we would gladly ship it. Jason, could you think of one?

Benjamin: Thanks for this consideration it is really appreciated.

Comment 15

[Jason Orendorff [:jorendorff]]

Some of the analysis I gave in comment 8 is wrong. This has nothing to do with prologues.

Here's the real story. Steps 1 and 2 were correct...

(In reply to Jason Orendorff [:jorendorff] from comment #8)
> Sequence of events leading to the crash:
> 
> 1.  `gen.throw()` sends an exception to a generator that is suspended at
> `yield* iter;`.
> 
>     Per spec, this is supposed to call `iter.throw()`.
> 
> 2.  `iter.throw` is undefined.
>     -> ReportIsNotFunction
>     -> DecompileValueGenerator
>     -> FindStartPC
> 
> 3.  FindStartPC creates a BytecodeParser which walks the script's bytecode.
>     The intent of this code is to collect info on every opcode in the script.

      So far so good, but the problem is that the bytecode looks like this:

          00018:  goto 66 (+48)
          00023:  try
          00024:  yield
          ...
          00081:  getprop "done"
          00086:  ifeq 24 (-62)

      This bytecode sequence means that opcode 23, the JSOP_TRY opcode, is unreachable.

      It's not an interpreter bug because JSOP_TRY is a no-op in the interpreter.
      From almost every possible angle, it does not matter at all whether the
      JSOP_IFEQ instruction at the bottom jumps to offset 23 or 24.

      But BytecodeParser needs to see that JSOP_TRY, because that's the only way
      it can know that it's possible for the interpreter to resume execution at
      the start of the catch block.

      Here the JSOP_TRY op really is unreachable.
>     BytecodeParser therefore does not find it, and thus doesn't parse any of
> the
>     bytecode in the catch block.
> 
>     Which is where the `iter.throw()` call is.
> 
> 4.  So when FindStartPC asks for the stack depth at the pc where the error
> occurred,
>     the BytecodeParser has no record that there was an opcode at that pc.
>     -> crash
> 
> The crash is at a fixed small offset from NULL, so it's not exploitable.
> 
> Now for the cause:

The bytecode emitter was generating code that was buggy, but in a really obscure way. Bug 987560 fixed this, by doing:

         // Try prologue.                                             // ITER RESULT
         StmtInfoBCE stmtInfo(cx);
         PushStatementBCE(bce, &stmtInfo, STMT_TRY, bce->offset());
         ptrdiff_t noteIndex = NewSrcNote(cx, bce, SRC_TRY);
    +    ptrdiff_t tryStart = bce->offset();                          // tryStart:
         if (noteIndex < 0 || Emit1(cx, bce, JSOP_TRY) < 0)
             return false;
    -    ptrdiff_t tryStart = bce->offset();                          // tryStart:

After that, the bytecode looks like:

          00024:  goto 78 (+54)
          00029:  try 41 (+12)
          00030:  getaliasedvar ".generator" (hops = 0, slot = 2)
          ...
          00093:  getprop "done"
          00098:  ifeq 29 (-69)

Note the "ifeq 29", which jumps to instruction 29, the JSOP_TRY instruction, which is thus reachable.

> The easiest fix to backport would be to bail out if the BytecodeParser
> failed to find the current op in the script. We already have fallback code
> for cases where the expression decompiler doesn't work.

This is still a better way to fix the issue than taking the above patch, I think. We don't want subtle issues like this to lead to crashes. We should assert in debug mode and fail gracefully in release mode.

Comment 16

[Jason Orendorff [:jorendorff]]

I tried it, and the bit of patch I quoted in comment 15 is not by itself sufficient to make a patch for this. I don't think I can spend any more time on this, unfortunately, as other bugs are more pressing.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

I can't reproduce a crash on this page. Is there anything left to do here?