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

RESOLVED FIXED in Firefox 31

Status

()

Core
JavaScript Engine: JIT
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: dougc, Assigned: dougc)

Tracking

unspecified
mozilla32
ARM
All
Points:
---

Firefox Tracking Flags

(firefox29 wontfix, firefox30 wontfix, firefox31 fixed, firefox32 fixed, firefox-esr24 wontfix)

Details

(Whiteboard: [qa-] )

Attachments

(1 attachment)

(Assignee)

Description

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

Comment 1

4 years ago
Created attachment 8433286 [details] [diff] [review]
Do not apply actualOffset() to the safepoint stream offsets.
Assignee: nobody → dtc-moz
Attachment #8433286 - Flags: review?(jdemooij)
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.
Attachment #8433286 - Flags: review?(jdemooij) → review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
(Assignee)

Comment 3

4 years ago
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
Attachment #8433286 - Flags: approval-mozilla-beta?
Attachment #8433286 - Flags: approval-mozilla-aurora?
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
Attachment #8433286 - Flags: review+
(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.
status-firefox29: --- → affected
status-firefox30: --- → affected
status-firefox31: --- → affected
status-firefox32: --- → affected
status-firefox-esr24: --- → affected
Do we have a Try link for this? :)
Keywords: checkin-needed
(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
Wont fix for 29 and ESR (not a security issue). I guess this is going to have the same fate for 30.
status-firefox29: affected → wontfix
status-firefox-esr24: affected → wontfix
Idem for 30.
status-firefox30: affected → wontfix
Attachment #8433286 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8433286 [details] [diff] [review]
Do not apply actualOffset() to the safepoint stream offsets.

wrong way - meant to '-' for beta.
Attachment #8433286 - Flags: approval-mozilla-beta+ → approval-mozilla-beta-
See also comment 7 for try link (all oranges seem unrelated).
Keywords: checkin-needed
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/766912d43305
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Attachment #8433286 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/143f7c2f05df
status-firefox31: affected → fixed
status-firefox32: affected → fixed
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.