Closed
Bug 912534
Opened 11 years ago
Closed 11 years ago
SpiderMonkey: SPS Profiler segfault
Categories
(Core :: JavaScript Engine, defect)
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)
1.06 KB,
patch
|
nbp
:
review+
akeybl
:
approval-mozilla-aurora+
akeybl
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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.
Comment 1•11 years ago
|
||
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•11 years ago
|
||
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•11 years ago
|
||
Comment on attachment 802939 [details] [diff] [review] Fix. Fixing empty review request.
Attachment #802939 -
Flags: review? → review?(nicolas.b.pierron)
Comment 4•11 years ago
|
||
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?
Updated•11 years ago
|
Flags: needinfo?(kvijayan)
Assignee | ||
Comment 5•11 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)
Comment 6•11 years ago
|
||
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.
Comment 7•11 years ago
|
||
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.
Comment 8•11 years ago
|
||
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 9•11 years ago
|
||
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•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/b17120deb5b4
Flags: needinfo?(kvijayan)
Comment 11•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/b17120deb5b4
Assignee: general → kvijayan
Status: NEW → RESOLVED
Closed: 11 years ago
status-firefox27:
--- → fixed
Resolution: --- → FIXED
Updated•11 years ago
|
Target Milestone: --- → mozilla27
Comment 12•11 years ago
|
||
Kannan, given that this seems to have fixed a crash or two, should we consider uplifting this to Aurora/Beta?
Updated•11 years ago
|
status-b2g18:
--- → verified disabled
status-firefox26:
--- → affected
status-firefox-esr17:
--- → unaffected
status-firefox-esr24:
--- → wontfix
Keywords: crash
Assignee | ||
Comment 13•11 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•11 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 15•11 years ago
|
||
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+
Comment 16•11 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/36c99527ce97 https://hg.mozilla.org/releases/mozilla-beta/rev/88dbb710851f
status-b2g-v1.2:
--- → fixed
status-firefox25:
--- → fixed
Updated•11 years ago
|
Whiteboard: [adv-main25+]
Updated•9 years ago
|
Group: core-security
You need to log in
before you can comment on or make changes to this bug.
Description
•