1019413 - IonMonkey: do not apply actualOffset() to the safepoint stream offsets.

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Douglas Crosher [:dougc]]

The actualOffset() method is for converting assembler buffer offsets to their final position and is not relevant to the safepoint stream offsets and it could corrupt these offsets.

Comment 1

[Douglas Crosher [:dougc]]

Attachment: Do not apply actualOffset() to the safepoint stream offsets.

Comment 2

[Jan de Mooij [:jandem]]

Comment on attachment 8433286 [details] [diff] [review]
Do not apply actualOffset() to the safepoint stream offsets.

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

Awesome. I'm surprised the fuzzers didn't hit this yet.

Comment 3

[Douglas Crosher [:dougc]]

Comment on attachment 8433286 [details] [diff] [review]
Do not apply actualOffset() to the safepoint stream offsets.

[Approval Request Comment]

Bug caused by (feature/regressing bug #): This is a long standing bug. The last change to this code was some renaming in bug 726236.

User impact if declined: This has not caused any reported crashes, but it is incorrect code and there might be potential for a crash.

Testing completed (on m-c, etc.): passes jit-tests locally. Corrects an issue in extended testing.

Risk to taking this patch (and alternatives if risky): low.

String or IDL/UUID changes made by this patch: n/a

Comment 4

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8433286 [details] [diff] [review]
Do not apply actualOffset() to the safepoint stream offsets.

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

I confirm this patch is good, as the safepointOffset is coming from the CompactBuffer[1,2,3,4] and not the assembly buffer.
We *should* backport this patch to all branches.

[1] http://dxr.mozilla.org/mozilla-central/source/js/src/jit/Safepoints.h?from=SafepointWriter&case=true#23
[2] http://dxr.mozilla.org/mozilla-central/source/js/src/jit/Safepoints.cpp#31
[3] http://dxr.mozilla.org/mozilla-central/source/js/src/jit/Safepoints.cpp#320
[4] http://dxr.mozilla.org/mozilla-central/source/js/src/jit/LIR.h#1404

Comment 5

[Nicolas B. Pierron [:nbp]]

(In reply to Nicolas B. Pierron [:nbp] from comment #4)
> We *should* backport this patch to all branches.

Note, this mostly matters for ARM, as the fixup is a no-op on x86 / x64 architectures.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Do we have a Try link for this? :)

Comment 7

[Nicolas B. Pierron [:nbp]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #6)
> Do we have a Try link for this? :)

Now, we do:
https://tbpl.mozilla.org/?tree=Try&rev=9e217cf9f02a

Comment 8

[Sylvestre Ledru [:Sylvestre]]

Wont fix for 29 and ESR (not a security issue). I guess this is going to have the same fate for 30.

Comment 9

[Sylvestre Ledru [:Sylvestre]]

Idem for 30.

Comment 10

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 8433286 [details] [diff] [review]
Do not apply actualOffset() to the safepoint stream offsets.

wrong way - meant to '-' for beta.

Comment 11

[Benjamin Bouvier [:bbouvier] (inactive)]

See also comment 7 for try link (all oranges seem unrelated).

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/766912d43305

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/766912d43305

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/143f7c2f05df