953104 - OdinMonkey: Assertion failure: !elems_.empty(), at jit/AsmJS.cpp:1213 or Crash on Heap

Status: VERIFIED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Christian Holler (:decoder)]

The following testcase asserts on mozilla-central revision cd3e9359fd64 (run with --fuzzing-safe --ion-eager --ion-compile-try-catch --ion-eager):


function m() {
    "use asm";
    function f() {
        var x = 0;
        var y = 0;
        y = y[(x + 1) & 3]() | 0;
    }
    return f;
}
m()();

Comment 1

[Christian Holler (:decoder)]

Attachment: [crash-signature] Machine-readable crash signature

Comment 2

[Christian Holler (:decoder)]

JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/63d537758124
user:        Luke Wagner
date:        Thu Jun 13 11:48:47 2013 -0700
summary:     Bug 880538 - OdinMonkey: make a single pass over the parse tree (r=bbouvier)

This iteration took 330.591 seconds to run.

Comment 3

[Christian Holler (:decoder)]

Needinfo from Luke based on comment 2.

Comment 4

[Luke Wagner [:luke]]

Attachment: fix-func-ptr-table-bug

Oops, there totally should have been a unit test for this.  Great find!

Comment 5

[Andrew McCreight [:mccr8]]

How bad of a security issue is this, Luke?  Thanks.

Comment 6

[Luke Wagner [:luke]]

I think this could be used to call a definitely-null pointer, so it'd be a crash, but not exploitable.  This won't happen in well-formed asm.js programs, so I'd be inclined to let the fix ride the trains.

Comment 7

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8355255 [details] [diff] [review]
fix-func-ptr-table-bug

Review of attachment 8355255 [details] [diff] [review]:
-----------------------------------------------------------------

Nice catch!

::: js/src/jit-test/tests/asm.js/testFunctionPtr.js
@@ +11,5 @@
>  assertAsmTypeFail(USE_ASM + "function f() {} function g(i) {i=i|0} var tbl=[f,g]; return f");
>  assertAsmTypeFail(USE_ASM + "function f() {} function g() {return 0} var tbl=[f,g]; return f");
>  assertAsmTypeFail(USE_ASM + "function f(i) {i=i|0} function g(i) {i=+i} var tbl=[f,g]; return f");
>  assertAsmTypeFail(USE_ASM + "function f() {return 0} function g() {return 0.0} var tbl=[f,g]; return f");
> +assertAsmTypeFail(USE_ASM + "var tbl=0; function g() {tbl[0&1]()|0} return g");

Ha, nice catch! I have been sometimes worried about creating assertAsmTypeFail tests and having them pass for another reason, like what was happening here.

Comment 8

[Luke Wagner [:luke]]

Yeah, I've thought it might be nice to match not just that there was an error, but part of the text of the message.  OTOH, this would make the tests more fragile, which would be annoying.

https://hg.mozilla.org/integration/mozilla-inbound/rev/567977c0c7da

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/567977c0c7da

Comment 10

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

(In reply to Luke Wagner [:luke] from comment #6)
> I think this could be used to call a definitely-null pointer, so it'd be a
> crash, but not exploitable.  This won't happen in well-formed asm.js
> programs, so I'd be inclined to let the fix ride the trains.

Sounds like a wontfix for B2G as well then too :)