624439 - Assertion failure: isS32(target - next) (./nanojit/NativeX64.cpp:2012)

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

Attachment: Test case for shell (x86_64)

The attached shell testcase causes one of the following assertions (randomly) on x86_64 architectures:

Assertion failure: isS32(target - next) (./nanojit/NativeX64.cpp:2012)
Assertion failure: isS32(offset) (./nanojit/NativeX64.cpp:224)

The test runs for 2-20 seconds, then the assertion should appear.

Due to the use of "evalcx" in the testcase, I don't know if it is valid or what the problem is, but if it is reproducable without that function, then it might be a serious bug.

Comment 1

[David Anderson [:dvander] - inactive, e-mail if emergency]

Bugs like this point to possible page mismanagement or buffer/range checking bugs in nanojit. This should block on investigation, at the least.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: patch (against TM 60026:0ccd15d19393)

The problem is simple:  various functions in the X64 back-end assume offsets fit into int32.  Hence the assertions.

The attached patch removes this assumption for JMPl(), which fixes this particular test case;  with it applied I can't reproduce the problem (and I tried 100 times in a row).  One example offset I saw for JMPl() was -2608348876.

That leaves the following functions that assume offsets fit into int32:
JO, JE, JL, ..., JNO, JNE, JNL, ..., CALL.  Do we need to adjust these to handle non-int32 offsets as well?  I don't even quite understand how JMPl can be given non-int32 offsets, but I know that the attached test case causes TM to generate bazillions of identical fragments.  (Perhaps this should be limited somehow...)

Comment 3

[David Anderson [:dvander] - inactive, e-mail if emergency]

there are no 64-bit wide jumps for Jcc, unfortunately. early JM (and the early NJ x64 port) had the ability to direct the jump to the end of the page and put an unconditional jump there. now JM just stops compiling if it hits this case :|

Comment 4

[Nicholas Nethercote [inactive]]

The X64 back-end already has a ConditionalBranchTooFar error code which is used is asm_branch_ov().  It could be used more extensively. 

That leaves CALL, which can take 64-bit operands AFAICT.

Comment 5

[Nicholas Nethercote [inactive]]

W.r.t. security implications, this bug is on X64 only, but it will cause a jump to a totally bogus address to occur.  It seems like it would be very difficult to control where that bogus jump goes to, but stranger things have happened.

Comment 6

[Nicholas Nethercote [inactive]]

Ed, do you think CALL needs to be fixed as well?  And the conditional branches?  Maybe I'll make them abort just to be on the safe side.

Comment 7

[Nicholas Nethercote [inactive]]

Attachment: patch v2 (against TM 60218:c8cb1ab7612a)

This patch aborts codegen if a too-big offset is encountered in emit_target32().  Thus it protects us if we ever have a too-big offset in CALL or a conditional branch.

Comment 8

[Edwin Smith]

(In reply to comment #6)
> Ed, do you think CALL needs to be fixed as well?  And the conditional branches?
>  Maybe I'll make them abort just to be on the safe side.

I think a too-big offset in emit_target32() should assert, and the callers of it should check for large offsets and emit different code.  It looks like CALL is already protected - the only call site I see is in asm_call:

    if (isTargetWithinS32(target)) {
        CALL(8, target);
    } else {
        // can't reach target from here, load imm64 and do an indirect jump
        CALLRAX();
        asm_immq(RAX, (uint64_t)target, /*canClobberCCs*/true);
    }

Setting the error code and asserting seems fine.

For JMPl(), it is meant to emit a patch-able jump; if there is any chance the target is changed later, there needs to be enough room for JMP64.  So, unconditionally emitting JMP64 might be best, or if the JMP32 case is used, pad with some space to allow for a JMP64 later.  The padding will never execute.

That reminds me, I wish we had a way to limit CodeAlloc to only serve up blocks within a +/- 2GB address range.  Then, the only 64-bit offsets we'd have to worry about would be references to code outside the assembled fragments.  I think that means, just calls.  Other compiled fragments would be in the same CodeAlloc instance and thus nearby.

Comment 9

[Edwin Smith]

Comment on attachment 503723 [details] [diff] [review]
patch v2 (against TM 60218:c8cb1ab7612a)

Hesitant R+  -- I'm a bit concerned about the interaction of JMPl() and nPatchBranch(); if a JMP32 can be patched later to a farther away address, then JMPl needs to leave room for a JMP64.  It looks like nPatchBranch already asserts if the patch target is too far;  do we need it to support far branches?  or, should it set the BranchToFar error?

Comment 10

[Nicholas Nethercote [inactive]]

Good point.  I think patching far branches isn't necessary, they're so rare, so I'll just set BranchTooFar there as well.  Thanks for the review!

Comment 11

[Rick Reitmaier]

Can we fix up the CALL?   

It should be the only entity triggering the assert, since JO, JE, JL, ..., JNO, JNE, JNL do not require 64b range.  See asm_branch_helper; The compare is inverted and a unconditional branch is inserted. 

Unless I'm missing something.

Comment 12

[Rick Reitmaier]

(In reply to comment #11)

> Unless I'm missing something.

Well I was...a little too quick on the submit button so please excuse the above comment.

I think the following are all we need to worry about:

- re JMPl()    How about emitting a 64b unconditional branch and then nPatchBranch() can squeeze it down to a 32b jump when its ok to do so.

- call is ok 

- conditionals are ok

- asm_branch_ov () - can we use code similar to asm_branch_helper() ?

Comment 13

[Nicholas Nethercote [inactive]]

http://hg.mozilla.org/projects/nanojit-central/rev/c3c9fa2b440f

Comment 14

[Nicholas Nethercote [inactive]]

http://hg.mozilla.org/tracemonkey/rev/6179074a3fa5

Comment 15

[Chris Leary [:cdleary] (not checking bugmail)]

cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/6179074a3fa5

Comment 16

[Daniel Veditz [:dveditz]]

(In reply to comment #5)
> It seems like it would be very difficult to control where that bogus
> jump goes to, but stranger things have happened.

You don't need great control, you just need to fill memory with lots of copies of the attack and hope with a big enough barn door you can hit it.

Comment 18

[Christian Holler (:decoder)]

Old tracer bug, and 1.9.2. unaffected, opening and marking verified.