Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(13)

Issue 2444233004: [compiler] Properly validate stable map assumption for globals. (Closed)

Created:
4 years, 1 month ago by Benedikt Meurer
Modified:
4 years, 1 month ago
Reviewers:
Yang
CC:
v8-reviews_googlegroups.com
Target Ref:
refs/pending/heads/master
Project:
v8
Visibility:
Public.

Description

[compiler] Properly validate stable map assumption for globals. For global object property cells, we did not check that the map on the previous object is still the same for which we actually optimized. So the optimized code was not in sync with the actual state of the property cell. When loading from such a global object property cell, Crankshaft optimizes away any map checks (based on the stable map assumption), leading to arbitrary memory access in the worst case. TurboFan has the same bug for stores, but is safe on loads because we do appropriate map checks there. However mixing TurboFan and Crankshaft still exposes the bug. R=yangguo@chromium.org BUG=chromium:659475 Committed: https://crrev.com/2bd7464ec1efc9eb24a38f7400119a5f2257f6e6 Cr-Commit-Position: refs/heads/master@{#40592}

Patch Set 1 #

Patch Set 2 : Fix the stupid CONVERT_LANGUAGE_MODE_ARG_CHECKED to not require a Smi. #

Patch Set 3 : Enforce HeapObject representation for cell value. #

Patch Set 4 : Unsmartify Crankshaft #

Unified diffs Side-by-side diffs Delta from patch set Stats (+86 lines, -9 lines) Patch
M src/bailout-reason.h View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M src/compiler/js-global-object-specialization.cc View 1 chunk +7 lines, -2 lines 0 comments Download
M src/crankshaft/hydrogen.cc View 1 2 3 1 chunk +12 lines, -4 lines 0 comments Download
M src/runtime/runtime-utils.h View 1 1 chunk +5 lines, -3 lines 0 comments Download
A test/mjsunit/regress/regress-crbug-659475-1.js View 1 chunk +30 lines, -0 lines 0 comments Download
A test/mjsunit/regress/regress-crbug-659475-2.js View 1 chunk +31 lines, -0 lines 0 comments Download

Messages

Total messages: 38 (25 generated)
Benedikt Meurer
4 years, 1 month ago (2016-10-26 07:03:36 UTC) #1
Yang
lgtm
4 years, 1 month ago (2016-10-26 07:10:46 UTC) #4
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2444233004/1
4 years, 1 month ago (2016-10-26 07:11:37 UTC) #7
commit-bot: I haz the power
Try jobs failed on following builders: v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/builds/6862) v8_linux64_gyp_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, ...
4 years, 1 month ago (2016-10-26 07:26:47 UTC) #9
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2444233004/20001
4 years, 1 month ago (2016-10-26 07:36:39 UTC) #15
commit-bot: I haz the power
Try jobs failed on following builders: v8_win_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/16841) v8_win_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, ...
4 years, 1 month ago (2016-10-26 07:54:33 UTC) #17
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2444233004/40001
4 years, 1 month ago (2016-10-26 08:27:41 UTC) #23
commit-bot: I haz the power
Committed patchset #3 (id:40001)
4 years, 1 month ago (2016-10-26 08:54:46 UTC) #24
Benedikt Meurer
A revert of this CL (patchset #3 id:40001) has been created in https://codereview.chromium.org/2454513003/ by bmeurer@chromium.org. ...
4 years, 1 month ago (2016-10-26 11:11:08 UTC) #25
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2444233004/60001
4 years, 1 month ago (2016-10-26 13:14:37 UTC) #32
commit-bot: I haz the power
Committed patchset #4 (id:60001)
4 years, 1 month ago (2016-10-26 13:43:57 UTC) #34
commit-bot: I haz the power
Patchset 4 (id:??) landed as https://crrev.com/3aa57eb920652746ba44ea5009a27a48900441ad Cr-Commit-Position: refs/heads/master@{#40578}
4 years, 1 month ago (2016-11-17 22:13:18 UTC) #36
commit-bot: I haz the power
4 years, 1 month ago (2016-11-17 22:14:05 UTC) #38
Message was sent while issue was closed.
Patchset 4 (id:??) landed as
https://crrev.com/2bd7464ec1efc9eb24a38f7400119a5f2257f6e6
Cr-Commit-Position: refs/heads/master@{#40592}

Powered by Google App Engine
This is Rietveld 408576698