519805 - TM: arm fails if sti/stqi have displacements > 12 bits [nanojit]

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Andreas Gal :gal]

This might affect loads too. As per discussion with Ed the frontend should check what the permissible maximum displacement is (which the backend exports as a symbol) and then split up the store into a sti(add(base, disp), 0). This could be done in the LirWriter. We already have a bunch of helpers there that do similar things.

Comment 1

[Andreas Gal :gal]

Ed points out that we have to make sure that constant folding doesn't generate too large displacements again after we split them.

Comment 2

[Brian Crowder]

If we're right about the consequences of this, it should be security-sensitive.

Comment 3

[Brian Crowder]

Is there a wallpaper/band-aid approach on this that can correct our issue on ARM Windows Mobile until a proper solution is offered?

Comment 4

[Brian Crowder]

Gal proposed this as a testcase for this issue, but I am not able to hit the same assertion (or any crash, for that matter), running this testcase:

for (var i = 0; i < 1950; ++i)
    this["x" + i] = 1;
for (var j = 0; j < 10; ++j)
    ;

Comment 5

[Brian Crowder]

Attachment: fixes the assert on WINCE

This patch is by no means "the patch", but it fixes the assertion on WINCE, perhaps "the patch" for now is to simply ifdef WINCE this?

Comment 6

[Brian Crowder]

Errr, woops.  512, not 2048 (was playing w/ other sizes, but 512 is the only one that works).

Comment 7

[Brian Crowder]

Attachment: hg qref BEFORE uploading the patch next time.

Comment 8

[Andreas Gal :gal]

Attachment: patch

Comment 9

[Andreas Gal :gal]

Attachment: patch

Comment 10

[Andreas Gal :gal]

Attachment: patch

Comment 11

[Andreas Gal :gal]

Attachment: patch

Comment 12

[Graydon Hoare :graydon]

Comment on attachment 404379 [details] [diff] [review]
patch

good enough, thanks.

Comment 13

[Andreas Gal :gal]

http://hg.mozilla.org/tracemonkey/rev/102a83c28bd0

Comment 14

[Andreas Gal :gal]

Why is this bug hidden? We have never shipped an ARM product, have we?

Comment 15

[Brian Crowder]

I dunno, I figured it might be on at least a few thousand phones, etc., but perhaps I am overly optimistic.  (or pessimistic)  Feel free to open it, if you like.

Comment 16

[Andreas Gal :gal]

I recommend opening up the bug. Usually someone from the security group makes that call though so cc'ing Jesse.

Comment 17

[Andreas Gal :gal]

Follow-up fix for ARM.

http://hg.mozilla.org/tracemonkey/rev/9adc8fa19990

Comment 18

[Jesse Ruderman]

> I recommend opening up the bug.

Are there any nightly or beta users on ARM?  If there are, we should be nice to them and hold bugs private until they've had a chance to update.

Comment 19

[Brad Lassey [:blassey] (use needinfo?)]

yea, we have maemo users on beta and winmo users on alpha.  Both also have nightly users without a working update channel.

Comment 20

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/102a83c28bd0

Comment 21

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/635bba671d90

Comment 22

[Daniel Veditz [:dveditz]]

Does this bug affect the 1.9.1 branch, and if so would we bother to fix it there? If not we can probably unhide this bug now.