Closed
Bug 1388045
Opened 7 years ago
Closed 7 years ago
Branch Pruning suggests to optimize away observable operands.
Categories
(Core :: JavaScript Engine: JIT, defect)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla57
People
(Reporter: nbp, Assigned: nbp)
References
Details
(Keywords: sec-moderate, Whiteboard: [adv-main56+][post-critsmash-triage])
Attachments
(1 file, 1 obsolete file)
1.63 KB,
patch
|
nbp
:
review+
gchang
:
approval-mozilla-beta+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
Branch Pruning checks if operands are observable or not, and if not we might not flag these operands as having removed uses. The problem here comes from the fact that we should check the slot corresponding to the frame encoded by the resume point, and the aliasing made in the second loop of Bug 1342016 breaks this rule by using the compile info of the inner most frame for flagging the outer-most one. as highlighted here: http://searchfox.org/mozilla-central/rev/b52285fffc13f36eca6b47de735d4e4403b3859e/js/src/jit/IonAnalysis.cpp#198,228,232 This can lead to bugs where the operands are replaced by OptimizedOut magic values, instead of objects, or arguments which might be expected. (function / this / …)
Assignee | ||
Comment 1•7 years ago
|
||
Attachment #8894503 -
Flags: review?(sstangl)
Updated•7 years ago
|
Assignee: nobody → nicolas.b.pierron
Comment 2•7 years ago
|
||
Comment on attachment 8894503 [details] [diff] [review] Branch Pruning: Check the compile info associated with the resume point. Review of attachment 8894503 [details] [diff] [review]: ----------------------------------------------------------------- Good catch. This is a real problem. I would suggest adding the following assertion above the other |info.isObservableSlot()|: > MOZ_ASSERT(&(rp->block()->info()) == &info); ::: js/src/jit/IonAnalysis.cpp @@ +224,5 @@ > if (mir->shouldCancel("FlagAllOperandsAsHavingRemovedUses loop 2")) > return false; > > for (size_t i = 0, e = rp->numOperands(); i < e; i++) { > + if (rp->isObservableOperand(i)) Instead of using `isObservableOperand()`, let's store `rp->block()->info()` into a temporary variable outside of the inner loop, and then use the same `isObservableSlot(i)`. Alternatively, it would be acceptable to move `isObservableOperand()` out of MIR.cpp and into MIR.h, so it can be inlined. Actually, that's probably not such a bad idea. If you wanted to land this bug without painting a target on the issue, you could just phrase it as a patch making the `isObserableOperand()` variants and `isRecoverableOperand()` inlineable.
Attachment #8894503 -
Flags: review?(sstangl) → review+
Updated•7 years ago
|
status-firefox55:
--- → wontfix
status-firefox56:
--- → affected
status-firefox57:
--- → affected
status-firefox-esr52:
--- → unaffected
tracking-firefox56:
--- → ?
tracking-firefox57:
--- → ?
Assignee | ||
Comment 3•7 years ago
|
||
Attachment #8894965 -
Flags: review+
Assignee | ||
Comment 4•7 years ago
|
||
Comment on attachment 8894965 [details] [diff] [review] Branch Pruning: Check the compile info associated with the resume point. > [Security approval request comment] > How easily could an exploit be constructed based on the patch? Getting all branch pruning requirements should be easy. Getting all the rest of the compiler requirements might be hard. > Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? Yes, which is why I am asking for approval, even if I rated it as sec-moderate. > Which older supported branches are affected by this flaw? Since Firefox 55. > If not all supported branches, which bug introduced the flaw? Bug 1342016 > Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? The same patch should apply cleanly, or this should be an easy fix. > How likely is this patch to cause regressions; how much testing does it need? In the worst case, this would be a performance regession on benchmark, as was attempted to be fixed by Bug 1342016. If any, it would be fixed by Bug 1377710.
Attachment #8894965 -
Flags: sec-approval?
Comment 5•7 years ago
|
||
Let's check this into trunk on 8/28, three weeks into the current cycle. At that point, we'll want a beta patch for 56 nominated as well to go in after this lands on trunk.
Whiteboard: [checkin on 8/28]
Updated•7 years ago
|
Attachment #8894965 -
Flags: sec-approval? → sec-approval+
Updated•7 years ago
|
Attachment #8894503 -
Attachment is obsolete: true
Assignee | ||
Comment 7•7 years ago
|
||
Pushed by npierron@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/2bf6d4f8f985 Branch Pruning: Check the compile info associated with the resume point. r=sstangl
Whiteboard: [checkin on 8/28]
Comment 8•7 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/2bf6d4f8f985 Please nominate this patch for Beta approval when you get a chance. It grafts cleanly.
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: needinfo?(nicolas.b.pierron)
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Updated•7 years ago
|
Group: javascript-core-security → core-security-release
Assignee | ||
Comment 9•7 years ago
|
||
Comment on attachment 8894965 [details] [diff] [review] Branch Pruning: Check the compile info associated with the resume point. Approval Request Comment [Feature/Bug causing the regression]: Bug 1342016 [User impact if declined]: Optimized out values where an object is expected. [Is this code covered by automated tests?]: Yes. [Has the fix been verified in Nightly?]: Yes. [Needs manual test from QE? If yes, steps to reproduce]: No. [List of other uplifts needed for the feature/fix]: {} [Is the change risky?]: Unlikely. [Why is the change risky/not risky?]: The change is small, and can be proof read again and again, and … again. [String changes made/needed]: None
Flags: needinfo?(nicolas.b.pierron)
Attachment #8894965 -
Flags: approval-mozilla-beta?
Comment 10•7 years ago
|
||
Comment on attachment 8894965 [details] [diff] [review] Branch Pruning: Check the compile info associated with the resume point. Fix a security issue. Beta56+.
Attachment #8894965 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 11•7 years ago
|
||
uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/5a73bb1978e8
Updated•7 years ago
|
Whiteboard: [adv-main56+]
Updated•7 years ago
|
Flags: qe-verify-
Whiteboard: [adv-main56+] → [adv-main56+][post-critsmash-triage]
Updated•6 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•