Closed Bug 912534 Opened 11 years ago Closed 11 years ago

SpiderMonkey: SPS Profiler segfault

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla27
Tracking Status
firefox25 --- fixed
firefox26 --- fixed
firefox27 --- fixed
firefox-esr17 --- unaffected
firefox-esr24 --- wontfix
b2g18 --- verified disabled
b2g-v1.1hd --- verified disabled
b2g-v1.2 --- fixed

People

(Reporter: djvj, Assigned: djvj)

References

Details

(Keywords: crash, sec-moderate, Whiteboard: [adv-main25+])

Attachments

(1 file)

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
Attached patch Fix.Splinter Review
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 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)
> 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+
https://hg.mozilla.org/mozilla-central/rev/b17120deb5b4
Assignee: general → kvijayan
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Kannan, given that this seems to have fixed a crash or two, should we consider uplifting this to Aurora/Beta?
(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.
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+
Whiteboard: [adv-main25+]
Group: core-security
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: