Issue metadata
Sign in to add a comment
|
Security: Lazy bailout from TurboFan after CompareIC is wrong |
|||||||||||||||||||||||||
Issue descriptionVULNERABILITY DETAILS The CompareICStub returns to TurboFan with some numeric untagged value in the accumulator, and TurboFan immediately goes back to FCG/Ignition with that value via the lazy bailout, but those expect a boolean value. In the worst case we'd access arbitrary memory depending on the exact bit pattern in the accumulator. This is the Chromium mirror bug for https://bugs.chromium.org/p/v8/issues/detail?id=4788 VERSION Chrome Version: <=50 Operating System: All REPRODUCTION CASE See tests in CL: https://codereview.chromium.org/1738153002 The quickfix https://codereview.chromium.org/1738153002 should be merged back.
,
Feb 29 2016
I added you to the bug.
,
Mar 1 2016
,
Mar 1 2016
[Automated comment] Less than 2 weeks to go before stable on M49, manual review required.
,
Mar 1 2016
Your change meets the bar and is auto-approved for M50 (branch: 2661)
,
Mar 1 2016
[Automated comment] Less than 2 weeks to go before stable on M49, manual review required.
,
Mar 1 2016
Merge approved for M49 (branch 2623)
,
Mar 1 2016
Please merge this change to M49 branch 2623 before 5:00 PM PST today if it is a safe merge. We're cutting last M49 Beta build @ 5:00 PM PST today.
,
Mar 1 2016
Lowering severity because this is more severe only in combination with Ignition, which is not yet shipped.
,
Mar 1 2016
Based on info from hablich@ via email, this isn't stable enough for a last minute merge (and it won't land cleanly). sshruthi@ - suggest that you reject this merge and then re-add Merge-Triage label for tracking.
,
Mar 1 2016
Rejecting merge based on c#11
,
Mar 2 2016
,
Mar 3 2016
,
Mar 4 2016
,
Mar 7 2016
Merge was rejected for M49 as per c#12. Please merge to M50 branch 2661 ASAP if it is a safe merge.
,
Mar 8 2016
,
Mar 8 2016
mstarzinger@ has agreed to do the merges.
,
Mar 8 2016
In order for this change to apply, we also need to merge back the following CL: https://codereview.chromium.org/1738883002/ Please confirm that the merge approvals apply to the combination of both CLs as well.
,
Mar 8 2016
Ok, let's not merge it to 49: The delta is to big between 49 and the needed CLs.
,
Mar 8 2016
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/0843a173996f5f63eca749d6fe8c20d4813537d9 commit 0843a173996f5f63eca749d6fe8c20d4813537d9 Author: Michael Starzinger <mstarzinger@google.com> Date: Tue Mar 08 12:16:00 2016 Version 5.0.71.10 (cherry-pick) Merged 55b4df7357557eb16377ad9227e4e0a4224b7885 Merged d00da47b61462681b48e48bdff4a80a33da1a6d6 Merged 4da2e3dbcfc686f68e56c0ad5575257dc2d8ccf1 Merged c1507e158780b170f25f0f1da87cb5d78a56faee [runtime] Unify comparison operator runtime entries. [turbofan] Don't use the CompareIC in JSGenericLowering. PPC: [runtime] Unify comparison operator runtime entries. PPC: [turbofan] Don't use the CompareIC in JSGenericLowering. R=hablich@chromium.org BUG= chromium:590832 ,v8:4788 LOG=N Review URL: https://codereview.chromium.org/1777503002 . Cr-Commit-Position: refs/branch-heads/5.0@{#13} Cr-Branched-From: ad16e6c2cbd2c6b0f2e8ff944ac245561c682ac2-refs/heads/5.0.71@{#1} Cr-Branched-From: bd9df50d75125ee2ad37b3d92c8f50f0a8b5f030-refs/heads/master@{#34215} [modify] https://crrev.com/0843a173996f5f63eca749d6fe8c20d4813537d9/include/v8-version.h [modify] https://crrev.com/0843a173996f5f63eca749d6fe8c20d4813537d9/src/arm/code-stubs-arm.cc [modify] https://crrev.com/0843a173996f5f63eca749d6fe8c20d4813537d9/src/arm64/code-stubs-arm64.cc [modify] https://crrev.com/0843a173996f5f63eca749d6fe8c20d4813537d9/src/compiler/js-generic-lowering.cc [modify] https://crrev.com/0843a173996f5f63eca749d6fe8c20d4813537d9/src/compiler/js-generic-lowering.h [modify] https://crrev.com/0843a173996f5f63eca749d6fe8c20d4813537d9/src/ia32/code-stubs-ia32.cc [modify] https://crrev.com/0843a173996f5f63eca749d6fe8c20d4813537d9/src/interpreter/interpreter.cc [modify] https://crrev.com/0843a173996f5f63eca749d6fe8c20d4813537d9/src/mips/code-stubs-mips.cc [modify] https://crrev.com/0843a173996f5f63eca749d6fe8c20d4813537d9/src/mips64/code-stubs-mips64.cc [modify] https://crrev.com/0843a173996f5f63eca749d6fe8c20d4813537d9/src/objects-inl.h [modify] https://crrev.com/0843a173996f5f63eca749d6fe8c20d4813537d9/src/ppc/code-stubs-ppc.cc [modify] https://crrev.com/0843a173996f5f63eca749d6fe8c20d4813537d9/src/runtime/runtime-interpreter.cc [modify] https://crrev.com/0843a173996f5f63eca749d6fe8c20d4813537d9/src/runtime/runtime-object.cc [modify] https://crrev.com/0843a173996f5f63eca749d6fe8c20d4813537d9/src/runtime/runtime-operators.cc [modify] https://crrev.com/0843a173996f5f63eca749d6fe8c20d4813537d9/src/runtime/runtime.h [modify] https://crrev.com/0843a173996f5f63eca749d6fe8c20d4813537d9/src/x64/code-stubs-x64.cc [modify] https://crrev.com/0843a173996f5f63eca749d6fe8c20d4813537d9/test/mjsunit/mjsunit.status [add] https://crrev.com/0843a173996f5f63eca749d6fe8c20d4813537d9/test/mjsunit/regress/regress-4788-1.js [add] https://crrev.com/0843a173996f5f63eca749d6fe8c20d4813537d9/test/mjsunit/regress/regress-4788-2.js [modify] https://crrev.com/0843a173996f5f63eca749d6fe8c20d4813537d9/test/unittests/runtime/runtime-interpreter-unittest.cc
,
Mar 8 2016
Merge to M50 done. Merge to M49 rejected. Closing this issue.
,
Mar 8 2016
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/d5cf399450548164e69521ecd739ad9b6c613e67 commit d5cf399450548164e69521ecd739ad9b6c613e67 Author: mstarzinger <mstarzinger@chromium.org> Date: Tue Mar 08 14:53:04 2016 Revert of Version 5.0.71.10 (cherry-pick) (patchset #1 id:1 of https://codereview.chromium.org/1777503002/ ) Reason for revert: This makes "mjsunit/undetectable-compare" fail, some previous drive-by-fix might be missing from the branch that allows us to make the switch away from the CompareIC. This is no longer a quick-fix that can be merged back and we are essentially flying blind on the branch. I am no longer confident that the quick-fix can be merged back. Reverting. Original issue's description: > Version 5.0.71.10 (cherry-pick) > > Merged 55b4df7357557eb16377ad9227e4e0a4224b7885 > Merged d00da47b61462681b48e48bdff4a80a33da1a6d6 > Merged 4da2e3dbcfc686f68e56c0ad5575257dc2d8ccf1 > Merged c1507e158780b170f25f0f1da87cb5d78a56faee > > [runtime] Unify comparison operator runtime entries. > > [turbofan] Don't use the CompareIC in JSGenericLowering. > > PPC: [runtime] Unify comparison operator runtime entries. > > PPC: [turbofan] Don't use the CompareIC in JSGenericLowering. > > R=hablich@chromium.org > BUG= chromium:590832 ,v8:4788 > LOG=N > > Committed: https://chromium.googlesource.com/v8/v8/+/0843a173996f5f63eca749d6fe8c20d4813537d9 TBR=hablich@chromium.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG= chromium:590832 ,v8:4788 Review URL: https://codereview.chromium.org/1775883003 Cr-Commit-Position: refs/branch-heads/5.0@{#14} Cr-Branched-From: ad16e6c2cbd2c6b0f2e8ff944ac245561c682ac2-refs/heads/5.0.71@{#1} Cr-Branched-From: bd9df50d75125ee2ad37b3d92c8f50f0a8b5f030-refs/heads/master@{#34215} [modify] https://crrev.com/d5cf399450548164e69521ecd739ad9b6c613e67/include/v8-version.h [modify] https://crrev.com/d5cf399450548164e69521ecd739ad9b6c613e67/src/arm/code-stubs-arm.cc [modify] https://crrev.com/d5cf399450548164e69521ecd739ad9b6c613e67/src/arm64/code-stubs-arm64.cc [modify] https://crrev.com/d5cf399450548164e69521ecd739ad9b6c613e67/src/compiler/js-generic-lowering.cc [modify] https://crrev.com/d5cf399450548164e69521ecd739ad9b6c613e67/src/compiler/js-generic-lowering.h [modify] https://crrev.com/d5cf399450548164e69521ecd739ad9b6c613e67/src/ia32/code-stubs-ia32.cc [modify] https://crrev.com/d5cf399450548164e69521ecd739ad9b6c613e67/src/interpreter/interpreter.cc [modify] https://crrev.com/d5cf399450548164e69521ecd739ad9b6c613e67/src/mips/code-stubs-mips.cc [modify] https://crrev.com/d5cf399450548164e69521ecd739ad9b6c613e67/src/mips64/code-stubs-mips64.cc [modify] https://crrev.com/d5cf399450548164e69521ecd739ad9b6c613e67/src/objects-inl.h [modify] https://crrev.com/d5cf399450548164e69521ecd739ad9b6c613e67/src/ppc/code-stubs-ppc.cc [modify] https://crrev.com/d5cf399450548164e69521ecd739ad9b6c613e67/src/runtime/runtime-interpreter.cc [modify] https://crrev.com/d5cf399450548164e69521ecd739ad9b6c613e67/src/runtime/runtime-object.cc [modify] https://crrev.com/d5cf399450548164e69521ecd739ad9b6c613e67/src/runtime/runtime-operators.cc [modify] https://crrev.com/d5cf399450548164e69521ecd739ad9b6c613e67/src/runtime/runtime.h [modify] https://crrev.com/d5cf399450548164e69521ecd739ad9b6c613e67/src/x64/code-stubs-x64.cc [modify] https://crrev.com/d5cf399450548164e69521ecd739ad9b6c613e67/test/mjsunit/mjsunit.status [delete] https://crrev.com/0843a173996f5f63eca749d6fe8c20d4813537d9/test/mjsunit/regress/regress-4788-1.js [delete] https://crrev.com/0843a173996f5f63eca749d6fe8c20d4813537d9/test/mjsunit/regress/regress-4788-2.js [modify] https://crrev.com/d5cf399450548164e69521ecd739ad9b6c613e67/test/unittests/runtime/runtime-interpreter-unittest.cc
,
Mar 8 2016
I am no longer confident that this can be merged back as a "quick-fix". The "mjsunit/undetectable-compare" test started failing on our branch builders. There must be some other change that these fixes build upon. I suspect a particular drive-by-fix but am flying blind on the branch. Benedikt: If you feel confident that this can be merged back, then I am afraid you have to do it yourself. My foo is not strong enough.
,
Mar 10 2016
Please mark security bugs as fixed as soon as the fix lands, and before requesting merges. - Your friendly ClusterFuzz
,
Mar 10 2016
,
Mar 16 2016
,
Mar 24 2016
,
Mar 30 2016
,
Apr 27 2016
The following revision refers to this bug: https://chromium.googlesource.com/v8/v8.git/+/e64fd96a6c38ccc46b8bd99cc6adb83a978fbef0 commit e64fd96a6c38ccc46b8bd99cc6adb83a978fbef0 Author: bmeurer <bmeurer@chromium.org> Date: Wed Apr 27 05:40:37 2016 [turbofan] Don't use the CompareIC in JSGenericLowering. This is essentially a cherry-pick that was applied before plus the removal of a test that is now failing because it depends on more involved changes. The test case checks comparison of different undetectable JSReceivers, which is not relevant in practice, as there's only one of these at most, which is document.all. Merged 55b4df7357557eb16377ad9227e4e0a4224b7885 Merged d00da47b61462681b48e48bdff4a80a33da1a6d6 Merged 4da2e3dbcfc686f68e56c0ad5575257dc2d8ccf1 Merged c1507e158780b170f25f0f1da87cb5d78a56faee [runtime] Unify comparison operator runtime entries. [turbofan] Don't use the CompareIC in JSGenericLowering. PPC: [runtime] Unify comparison operator runtime entries. PPC: [turbofan] Don't use the CompareIC in JSGenericLowering. R=yangguo@chromium.org BUG= chromium:590832 ,v8:4788, chromium:606181 LOG=N NOTRY=true NOPRESUBMIT=true Cr-Commit-Position: refs/branch-heads/5.0@{#13} Cr-Branched-From: ad16e6c2cbd2c6b0f2e8ff944ac245561c682ac2-refs/heads/5.0.71@{#1} Cr-Branched-From: bd9df50d75125ee2ad37b3d92c8f50f0a8b5f030-refs/heads/master@{#34215} Review URL: https://codereview.chromium.org/1925463003 Cr-Commit-Position: refs/branch-heads/5.0@{#44} Cr-Branched-From: ad16e6c2cbd2c6b0f2e8ff944ac245561c682ac2-refs/heads/5.0.71@{#1} Cr-Branched-From: bd9df50d75125ee2ad37b3d92c8f50f0a8b5f030-refs/heads/master@{#34215} [modify] https://crrev.com/e64fd96a6c38ccc46b8bd99cc6adb83a978fbef0/include/v8-version.h [modify] https://crrev.com/e64fd96a6c38ccc46b8bd99cc6adb83a978fbef0/src/arm/code-stubs-arm.cc [modify] https://crrev.com/e64fd96a6c38ccc46b8bd99cc6adb83a978fbef0/src/arm64/code-stubs-arm64.cc [modify] https://crrev.com/e64fd96a6c38ccc46b8bd99cc6adb83a978fbef0/src/compiler/js-generic-lowering.cc [modify] https://crrev.com/e64fd96a6c38ccc46b8bd99cc6adb83a978fbef0/src/compiler/js-generic-lowering.h [modify] https://crrev.com/e64fd96a6c38ccc46b8bd99cc6adb83a978fbef0/src/ia32/code-stubs-ia32.cc [modify] https://crrev.com/e64fd96a6c38ccc46b8bd99cc6adb83a978fbef0/src/interpreter/interpreter.cc [modify] https://crrev.com/e64fd96a6c38ccc46b8bd99cc6adb83a978fbef0/src/mips/code-stubs-mips.cc [modify] https://crrev.com/e64fd96a6c38ccc46b8bd99cc6adb83a978fbef0/src/mips64/code-stubs-mips64.cc [modify] https://crrev.com/e64fd96a6c38ccc46b8bd99cc6adb83a978fbef0/src/objects-inl.h [modify] https://crrev.com/e64fd96a6c38ccc46b8bd99cc6adb83a978fbef0/src/ppc/code-stubs-ppc.cc [modify] https://crrev.com/e64fd96a6c38ccc46b8bd99cc6adb83a978fbef0/src/runtime/runtime-interpreter.cc [modify] https://crrev.com/e64fd96a6c38ccc46b8bd99cc6adb83a978fbef0/src/runtime/runtime-object.cc [modify] https://crrev.com/e64fd96a6c38ccc46b8bd99cc6adb83a978fbef0/src/runtime/runtime-operators.cc [modify] https://crrev.com/e64fd96a6c38ccc46b8bd99cc6adb83a978fbef0/src/runtime/runtime.h [modify] https://crrev.com/e64fd96a6c38ccc46b8bd99cc6adb83a978fbef0/src/x64/code-stubs-x64.cc [modify] https://crrev.com/e64fd96a6c38ccc46b8bd99cc6adb83a978fbef0/test/mjsunit/mjsunit.status [add] https://crrev.com/e64fd96a6c38ccc46b8bd99cc6adb83a978fbef0/test/mjsunit/regress/regress-4788-1.js [add] https://crrev.com/e64fd96a6c38ccc46b8bd99cc6adb83a978fbef0/test/mjsunit/regress/regress-4788-2.js [modify] https://crrev.com/e64fd96a6c38ccc46b8bd99cc6adb83a978fbef0/test/mjsunit/undetectable-compare.js [modify] https://crrev.com/e64fd96a6c38ccc46b8bd99cc6adb83a978fbef0/test/unittests/runtime/runtime-interpreter-unittest.cc
,
Jun 17 2016
This bug has been closed for more than 14 weeks. Removing security view restrictions. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Oct 1 2016
This bug has been closed for more than 14 weeks. Removing security view restrictions. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Oct 2 2016
This bug has been closed for more than 14 weeks. Removing security view restrictions. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Oct 2 2016
|
||||||||||||||||||||||||||
►
Sign in to add a comment |
||||||||||||||||||||||||||
Comment 1 by och...@chromium.org
, Feb 29 2016