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

VERIFIED FIXED in mozilla10

Status

()

Core
JavaScript Engine
--
critical
VERIFIED FIXED
6 years ago
4 years ago

People

(Reporter: gkw, Assigned: jbramley)

Tracking

(Blocks: 1 bug, {assertion, regression, testcase})

Trunk
mozilla10
ARM
Linux
assertion, regression, testcase
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

6 years ago
Created attachment 520494 [details]
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))
(Reporter)

Comment 1

6 years ago
This is likely to have occurred since changeset f569d49576bb when -a was first introduced.
(Reporter)

Updated

6 years ago
Summary: "ASSERTION FAILED: ((op2 & ~OP2_IMM) <= 0xfff) || (((op2 & ~OP2_IMMh) <= 0xfff))" on ARM → "Assertion failure: ((op2 & ~OP2_IMM) <= 0xfff) || (((op2 & ~OP2_IMMh) <= 0xfff))" on ARM
(Reporter)

Comment 2

6 years ago
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)
(Assignee)

Comment 3

6 years ago
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.
Assignee: general → Jacob.Bramley
(Assignee)

Comment 4

6 years ago
(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.
(Assignee)

Comment 5

6 years ago
Created attachment 543729 [details] [diff] [review]
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.
Attachment #543729 - Flags: review?(gary)
(Assignee)

Comment 6

6 years ago
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.
(Assignee)

Comment 7

6 years ago
(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.
(Assignee)

Updated

6 years ago
Attachment #543729 - Flags: review?(gary) → review?(mrosenberg)
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?
Attachment #543729 - Flags: review?(mrosenberg) → review+
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?
(Assignee)

Comment 10

6 years ago
(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.
(Assignee)

Comment 11

6 years ago
Created attachment 551474 [details] [diff] [review]
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.
Attachment #543729 - Attachment is obsolete: true
Attachment #551474 - Flags: review?(mrosenberg)
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.
Attachment #551474 - Flags: review?(mrosenberg) → review+
(Assignee)

Updated

6 years ago
Whiteboard: [inbound]
(Reporter)

Comment 13

6 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/77c924963f36
Backed out because of test failures on Android:
https://hg.mozilla.org/integration/mozilla-inbound/rev/29e59859d415
Whiteboard: [inbound]
(Assignee)

Comment 15

6 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/746e7c151de2
Whiteboard: [inbound]
https://hg.mozilla.org/mozilla-central/rev/746e7c151de2
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla10
(Reporter)

Comment 17

4 years ago
A type of test for this bug has already been landed because it is already marked in-testsuite+ -> VERIFIED.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.