If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

SpiderMonkey: SPS Profiler segfault

RESOLVED FIXED in Firefox 25, Firefox OS v1.2

Status

()

Core
JavaScript Engine
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: djvj, Assigned: djvj)

Tracking

({crash, sec-moderate})

unspecified
mozilla27
x86
Mac OS X
crash, sec-moderate
Points:
---

Firefox Tracking Flags

(firefox25 fixed, firefox26 fixed, firefox27 fixed, firefox-esr17 unaffected, firefox-esr24 wontfix, b2g18 verified disabled, b2g-v1.1hd verified disabled, b2g-v1.2 fixed)

Details

(Whiteboard: [adv-main25+])

Attachments

(1 attachment)

(Assignee)

Description

4 years ago
The following test cases crashes on trunk JS shell with '--ion-eager':


function f() {}

enableSPSProfilingAssertions(false);
for (var i = 0; i < 10; i++)
    f();

What's happening is that we are bailing from Ion into Baseline, with no profile stack entry.  Then the code OSRs into a profiling-enabled Ion compilation, which uses the nonexistant stack entry.

We should not OSR from baseline to Ion when the presence or lack of an profiler entry for the baseline frame doesn't match up with the Ion code's expectations.
calling this sec-moderate on the theory that requiring people to profile stuff limits the effectiveness of any attack.
Keywords: sec-moderate
(Assignee)

Comment 2

4 years ago
Created attachment 802939 [details] [diff] [review]
Fix.

Fix.  Disable jumping from Baseline to Ion when the IonScript's profiling state disagrees with the baseline frame's profiling state.
Attachment #802939 - Flags: review?

Comment 3

4 years ago
Comment on attachment 802939 [details] [diff] [review]
Fix.

Fixing empty review request.
Attachment #802939 - Flags: review? → review?(nicolas.b.pierron)
Comment on attachment 802939 [details] [diff] [review]
Fix.

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

::: js/src/jit/BaselineIC.cpp
@@ +769,5 @@
> +        IonScript *ion = script->ionScript();
> +
> +        // If the baseline frame's SPS handling doesn't match up with the Ion code's SPS
> +        // handling, don't OSR.
> +        if (frame->hasPushedSPSFrame() != ion->hasSPSInstrumentation()) {

The current issue is that baseline has no SPS frame, while Ion tries to pop one, right?
Or is that the opposite?

If this is the opposite, then, Ion code should have been invalidated.
Could we just push an SPS frame from anywhere in baseline, when we switch the toggle?
Flags: needinfo?(kvijayan)
(Assignee)

Comment 5

4 years ago
> The current issue is that baseline has no SPS frame, while Ion tries to pop
> one, right?

The issue is that there is no SPS frame entry, but the Ion jitcode thinks there is, so we can't enter that Ion jitcode.  It arrives at this state through the following path:


SPS is off
We compile Ion without SPS, and enter the function frame without pusing an SPS entry for it.
SPS is turned on.
Ion code is invalidated and we bail out to baseline code.  Still no SPS frame exists.
We re-compile with Ion at an OSR point, this time with SPS instrumentation enabled in Ion.
We enter Ion at the OSR point, Ion believes that the SPS frame exists, and tries to use it, but crashes.
Flags: needinfo?(kvijayan)
CC jld.

Could we just instead of reusing a new frame just push a new one, even if this will duplicate the frame on old branches?
Or in such case we can just push a new one on bailouts instead of adding this condition in EnterJit.
If we fix the profiler stack during OSR (push or pop the frame as needed), that will guarantee that we enter Ion jitcode with things in the right state, but maybe we're concerned about the overhead of adding that check?

Alternately, we could fix it during bailout… but at the very least I'd want the OSR path to assert that things remain correct when we get back to it.
I would prefer if they are balanced and unconditional.  As mentioned on the JS mailing list, whe need to care about the entry cost, as event callback functions need to be entered with a fast path.  So, I would prefer if we add it as part of the bailout-to-baseline instead of as part of the entry path.
Flags: needinfo?(kvijayan)
Comment on attachment 802939 [details] [diff] [review]
Fix.

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

Sorry I did not noticed that this code was already only on the path of the OSR and not on the path on any function entry.

::: js/src/jit/BaselineIC.cpp
@@ +769,5 @@
> +        IonScript *ion = script->ionScript();
> +
> +        // If the baseline frame's SPS handling doesn't match up with the Ion code's SPS
> +        // handling, don't OSR.
> +        if (frame->hasPushedSPSFrame() != ion->hasSPSInstrumentation()) {

nit: Assert that the ion SPS instrumentation is the same as the runtime instrumentation.
Attachment #802939 - Flags: review?(nicolas.b.pierron) → review+
(Assignee)

Comment 10

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/b17120deb5b4
Flags: needinfo?(kvijayan)
https://hg.mozilla.org/mozilla-central/rev/b17120deb5b4
Assignee: general → kvijayan
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-firefox27: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Blocks: 920482
Kannan, given that this seems to have fixed a crash or two, should we consider uplifting this to Aurora/Beta?
status-b2g18: --- → verified disabled
status-firefox26: --- → affected
status-firefox-esr17: --- → unaffected
status-firefox-esr24: --- → wontfix
Keywords: crash
(Assignee)

Comment 13

4 years ago
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #12)
> Kannan, given that this seems to have fixed a crash or two, should we
> consider uplifting this to Aurora/Beta?

Given the simplicity of the change, I'd say it's a good candidate for uplift to both.
(Assignee)

Comment 14

4 years ago
Comment on attachment 802939 [details] [diff] [review]
Fix.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
N/A, longstanding

User impact if declined:
Crashes in some cases when enabling SPS profiler.
 
Testing completed (on m-c, etc.): 
Green on m-c for two weeks.

Risk to taking this patch (and alternatives if risky): 
Minor, just disables a certain jitcode entry path in some cases.

String or IDL/UUID changes made by this patch:
N/A
Attachment #802939 - Flags: approval-mozilla-beta?
Attachment #802939 - Flags: approval-mozilla-aurora?
Comment on attachment 802939 [details] [diff] [review]
Fix.

Still have enough time to take a sec-moderate on branches.
Attachment #802939 - Flags: approval-mozilla-beta?
Attachment #802939 - Flags: approval-mozilla-beta+
Attachment #802939 - Flags: approval-mozilla-aurora?
Attachment #802939 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/36c99527ce97
https://hg.mozilla.org/releases/mozilla-beta/rev/88dbb710851f
status-b2g-v1.1hd: --- → verified disabled
status-b2g-v1.2: --- → fixed
status-firefox25: --- → fixed
status-firefox26: affected → fixed
Whiteboard: [adv-main25+]
Group: core-security
You need to log in before you can comment on or make changes to this bug.