Closed Bug 1019413 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: JavaScript Engine: JIT, defect)

ARM
All
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32
Tracking Status
firefox29 --- wontfix
firefox30 --- wontfix
firefox31 --- fixed
firefox32 --- fixed
firefox-esr24 --- wontfix

People

(Reporter: dougc, Assigned: dougc)

References

Details

(Whiteboard: [qa-] )

Attachments

(1 file)

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: 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+
Keywords: checkin-needed
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.
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.
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
https://hg.mozilla.org/mozilla-central/rev/766912d43305
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Attachment #8433286 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.