643213 - "Assertion failure: ((op2 & ~OP2_IMM) <= 0xfff) || (((op2 & ~OP2_IMMh) <= 0xfff))" on ARM

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

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

Attachment: stack

(function(){w[268435456]})()

asserts js debug shell on TM changeset c811be25eaad with -m and -a at ASSERTION FAILED: ((op2 & ~OP2_IMM) <= 0xfff) || (((op2 & ~OP2_IMMh) <= 0xfff))

Comment 1

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

This is likely to have occurred since changeset f569d49576bb when -a was first introduced.

Comment 2

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

This assert still occurs. Can some ARM js folks pls check that this assert isn't dangerous? (esp. since Firefox Mobile is becoming more important)

Comment 3

[Jacob Bramley [:jbramley]]

Nasty! This is a simple signed arithmetic bug. I should have fixed this when you first filed it. I've just written a patch (which is now building), so I'll post it in a few minutes.

Comment 4

[Jacob Bramley [:jbramley]]

(In reply to comment #2)
> This assert still occurs. Can some ARM js folks pls check that this assert
> isn't dangerous? (esp. since Firefox Mobile is becoming more important)

Whilst a bad load can be generated, the engine will prevent the access at run-time anyway. The back-end trusts the engine to request only valid offsets, so there must be some filtering to check if the specified slot exists before an architectural load instruction is executed.

Only an offset of (-)2147483648 causes the code-generation problem. If some code is able to create an object with that many slots such that the JavaScript code can access it, the instruction that gets emitted will be a load with an offset of 0. Also, the top bit of the condition code will be set, though I think we always use AL (the default, 0xe), so this will have no effect.

I don't think the bug is exploitable. The buggy offset is half of the addressable memory space on a 32-bit machine. Even if it were possible for such an offset to be valid, the code would just access element 0 instead.

Comment 5

[Jacob Bramley [:jbramley]]

Attachment: Fix signed integer arithmetic used to construct ARM load instructions.

This fixes the arithmetic bug in dataTransfer32, as well as a similar one in dataTransfer8. The same bug is also present in doubleTransfer, but the code is unreachable so it's not a risk for now. I'll fix it as part of an optimization in bug 669132.

Comment 6

[Jacob Bramley [:jbramley]]

Actually, I think there's another bug in here. The offset argument to dataTransfer32 is signed, but the engine is treating it as unsigned. It actually meant to pass 268435456*8=2147483648 (0x80000000), but that is out of range for a signed integer.

The patch I've attached is still valid for the back-end as it includes a signed mathematics error, but the engine should have caught the out-of-range error anyway.

Comment 7

[Jacob Bramley [:jbramley]]

(In reply to comment #6)
> The patch I've attached is still valid for the back-end as it includes a
> signed mathematics error, but the engine should have caught the out-of-range
> error anyway.

Ah, ignore me. If the bad offset never gets executed, it doesn't matter.

Comment 8

[Marty Rosenberg [:mjrosenb]]

Comment on attachment 543729 [details] [diff] [review]
Fix signed integer arithmetic used to construct ARM load instructions.

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

Looks good to me?

Comment 9

[Marty Rosenberg [:mjrosenb]]

There was not supposed to be a question mark there.
I'd initially asked: "Can we get a processor mode that throws a signal on negating INT_MIN? That never does what you want."; decided that should go in a separate comment (this one), then deleted the wrong punctuation :(

Anyhow, can we get that?

Comment 10

[Jacob Bramley [:jbramley]]

(In reply to Marty Rosenberg [:Marty] from comment #9)
> I'd initially asked: "Can we get a processor mode that throws a signal on
> negating INT_MIN? That never does what you want."; decided that should go in
> a separate comment (this one), then deleted the wrong punctuation :(
> 
> Anyhow, can we get that?

The short answer: No.

The long answer:

No, there is no such mode. In fact, no integer arithmetic operation can throw an exception on ARM. The processor doesn't know if the register holds a signed or unsigned value, so in general it just sets the four condition flags and lets the software make the decision (if it cares). (There's a signed overflow flag as well as an unsigned one (carry).)

More importantly, the C compiler is not obliged to do anything special. "offset = -offset" might be optimized away (or into something else), for one thing, but the behaviour is undefined anyway. If the result of an operation is not in the range of representable values for the type, the behaviour is undefined.

Comment 11

[Jacob Bramley [:jbramley]]

Attachment: Rebase.

This is another rebase update, so it should be a quick review. It touches some code that's easy to get wrong and is poorly covered by tests, so I think it's worth having another set of eyes look at it. It's currently in try.

Comment 12

[Marty Rosenberg [:mjrosenb]]

Comment on attachment 551474 [details] [diff] [review]
Rebase.

># HG changeset patch
># Parent 5e946ec742d8da5a29ed3c412128bfe838177574
>Bug 643213: Fix signed integer arithmetic used to construct ARM load instructions. [r=cdleary]
>
>diff --git a/js/src/assembler/assembler/ARMAssembler.cpp b/js/src/assembler/assembler/ARMAssembler.cpp
>--- a/js/src/assembler/assembler/ARMAssembler.cpp
>+++ b/js/src/assembler/assembler/ARMAssembler.cpp
>@@ -311,41 +311,40 @@ void ARMAssembler::dataTransferN(bool is
>             // add upper bits of offset to the base, and store the result into the temp registe
*register
>             // for even bigger offsets, load the entire offset into a registe, then do an
*register-- I'm pretty sure I added all of these, and am kind of confused as to how I misspelled it so frequently and consistently.

> /* this is large, ugly and obsolete.  dataTransferN is superior.*/
> void ARMAssembler::dataTransfer8(bool isLoad, RegisterID srcDst, RegisterID base, int32_t offset, bool isSigned)
i'd say change srcDst to rt, but i'm about to rip out this function, and it isn't affecting correctness.

Comment 13

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

http://hg.mozilla.org/integration/mozilla-inbound/rev/77c924963f36

Comment 14

[Matt Brubeck (:mbrubeck)]

Backed out because of test failures on Android:
https://hg.mozilla.org/integration/mozilla-inbound/rev/29e59859d415

Comment 15

[Jacob Bramley [:jbramley]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/746e7c151de2

Comment 16

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

https://hg.mozilla.org/mozilla-central/rev/746e7c151de2

Comment 17

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

A type of test for this bug has already been landed because it is already marked in-testsuite+ -> VERIFIED.