854462 - [ARM]IonMonkey: Assertion failure: rejoinLabel.offset() == initialJump.offset() + REJOIN_LABEL_OFFSET, at ../ion/IonCaches.h:210

Status: RESOLVED INCOMPLETE

(Core :: JavaScript Engine: JIT, defect)

Description

[Christian Holler (:decoder)]

The following testcase asserts on mozilla-central revision 3acbf951b3b1 (run with --ion-eager):


var SECTION = "";
function TestCase( d, e, a) {
  this.bugnumber = typeof(BUGNUMER) != 'undefined' ? BUGNUMBER : '';
}
function reportCompare (expected, actual, description) {
  var testcase = new TestCase("unknown-test-name", description, expected, actual);
}
parseInt("0");
reportCompare (TestCase("Math.exp", "Number.NEGATIVE_INFINITY", 0), (100));
evaluate('\
new TestCase( SECTION, "1 * Number.NaN",             Number.NaN,     1 * Number.NaN );\
new TestCase( SECTION, "Number.POSITIVE_INFINITY * 0",   Number.NaN, Number.POSITIVE_INFINITY * 0 );\
new TestCase( SECTION, "Number.NEGATIVE_INFINITY * 0",   Number.NaN, Number.NEGATIVE_INFINITY * 0 );\
new TestCase( SECTION, "0 * Number.POSITIVE_INFINITY",   Number.NaN, 0 * Number.POSITIVE_INFINITY );\
new TestCase( SECTION, "-0 * Number.POSITIVE_INFINITY",  Number.NaN,   -0 * Number.POSITIVE_INFINITY );\
new TestCase( SECTION, "-0 * Number.NEGATIVE_INFINITY",  Number.NaN,   -0 * Number.NEGATIVE_INFINITY );\
new TestCase( SECTION, "Number.POSITIVE_INFINITY * -0",  Number.NaN,   Number.POSITIVE_INFINITY * -0 );\
new TestCase( SECTION, "Number.NEGATIVE_INFINITY * -0",  Number.NaN,   Number.NEGATIVE_INFINITY * -0 );\
', { noScriptRval : true });

Comment 1

[Christian Holler (:decoder)]

S-s because this looks like a dangerous assertion.

Comment 2

[Sean Stangl [:sstangl]]

I looked into this. This is security sensitive: the internal offset in |initialJump| is based on an invalid read from the stack on OOM, which allows writing specific data to arbitrary memory.

The problem is that jumpWithPatch() is fallible on ARM (and only on ARM). Internally, it's using some extremely convoluted branch pool logic in as_BranchPool().

as_BranchPool() needs to add an entry to some ARMBuffer, but on OOM, appending fails and a bogus struct with random data from the stack is returned. This bogus struct is then treated as gospel.

So either we need to make jumpWithPatch() explicitly fallible with a boolean return, or ARM needs to not return a garbage struct on OOM.

Marty, can we return a reasonable value in this function? Are there any other ARMAssembler functions that are assumed infallible but actually return random data on OOM?

Comment 3

[Sean Stangl [:sstangl]]

It's actually not as critical a security flaw -- the offset in the failure case is hardcoded to INT_MIN.

Comment 4

[Daniel Veditz [:dveditz]]

I wouldn't rule it out, but yeah sounds less than immediately sec-critical.

Comment 5

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

Dan mean setting sstangl as the *assignee*.

Comment 6

[Andrew McCreight [:mccr8]]

Sean, have you had a chance to look at this further?  Should we downgrade its severity?

Comment 7

[Andrew McCreight [:mccr8]]

Jandem, is there somebody who can look at this bug?  This and bug 809509 have been drifting along for months.  Thanks.

Comment 8

[Jan de Mooij [:jandem]]

Marty can you take this? According to Sean's analysis in comment 2 this is an ARM-only problem.

Comment 9

[Sean Stangl [:sstangl]]

Continuing the long chain of needinfo?, in order to attract attention.

Comment 10

[Sean Stangl [:sstangl]]

Note that even MOZ_CRASH() would be preferable to the current random behavior.

Comment 11

[Jan de Mooij [:jandem]]

Marty any update here? Based on comment 2 it shouldn't be too hard to fix for somebody familiar with the code.

Comment 12

[Marty Rosenberg [:mjrosenb]]

It looks like x86 can also fail in a similar way, but it won't trip assertions, and it'll probably pass most sanity checks before we do the final oom check at the end.  I'm also not entirely convinced that this is actually a security issue on arm.  we write an invalid pointer into a structure, but nothing uses that value before we return control flow to js::ion::CodeGenerator::generateBody and bail due to the masm.oom() check there.

Comment 13

[Marty Rosenberg [:mjrosenb]]

Attachment: bypasAssert-r0.patch

Since it looks like nothing bad can actually happen because an invalid struct is returned, just silence the assertion failure when we run out of memory.

Comment 14

[Jan de Mooij [:jandem]]

Comment on attachment 818839 [details] [diff] [review]
bypasAssert-r0.patch

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

::: js/src/ion/CodeGenerator.cpp
@@ +99,5 @@
>          return false;
>  
>      CodeOffsetJump jump = masm.jumpWithPatch(ool->repatchEntry());
>      CodeOffsetLabel label = masm.labelForPatch();
>      masm.bind(ool->rejoin());

I don't see this in CodeGeneratorShared::addCache, are you on an old revision? Also, I don't see this assert in IonCaches.h so is this no longer a problem?

Comment 15

[Marty Rosenberg [:mjrosenb]]

oh, well that may be the reason that I wasn't able to reproduce it on nightly.  Sure looks like it isn't a problem anymore.

Comment 16

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to Marty Rosenberg [:mjrosenb] from comment #15)
> oh, well that may be the reason that I wasn't able to reproduce it on
> nightly.  Sure looks like it isn't a problem anymore.

Sounds like we can downgrade the severity?