Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

deps: backport 2bd7464 from upstream V8 #10169

Closed

Conversation

cristiancavalli
Copy link

Checklist
  • make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

deps

Description of change

Backport of bugfix from upstream V8

Original commit message:
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

Review-Url: https://codereview.chromium.org/2444233004
Cr-Commit-Position: refs/heads/master@{#40592}

V8 commit: v8/v8@2bd7464

Original commit message:
  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

  Review-Url: https://codereview.chromium.org/2444233004
  Cr-Commit-Position: refs/heads/master@{nodejs#40592}
@nodejs-github-bot nodejs-github-bot added v6.x v8 engine Issues and PRs related to the V8 dependency. labels Dec 7, 2016
@cristiancavalli
Copy link
Author

cc @ofrobots

@ofrobots
Copy link
Contributor

ofrobots commented Dec 7, 2016

/cc @nodejs/v8

@mhdawson
Copy link
Member

mhdawson commented Dec 7, 2016

@ofrobots
Copy link
Contributor

ofrobots commented Dec 8, 2016

@mhdawson
Copy link
Member

mhdawson commented Dec 8, 2016

New CI run as seems to have failed a good part of the arm run : https://ci.nodejs.org/job/node-test-pull-request/5312/

@ofrobots
Copy link
Contributor

The arm-fanned issues in the latest CI seem unrelated infrastructure issues. Rest is green.

ofrobots pushed a commit that referenced this pull request Dec 13, 2016
Original commit message:
  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

  Review-Url: https://codereview.chromium.org/2444233004
  Cr-Commit-Position: refs/heads/master@{#40592}

PR-URL: #10169
Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: ofrobots - Ali Ijaz Sheikh <ofrobots@google.com>
@ofrobots
Copy link
Contributor

Landed on v6.x-staging as ee09828.

@cristiancavalli This doesn't cleanly apply to v4.x-staging, but is needed for that branch as well. Could you port the fix to v4.x-staging as well?

@ofrobots ofrobots closed this Dec 13, 2016
@MylesBorins
Copy link
Contributor

MylesBorins commented Dec 13, 2016

Landed in v4.x as 7bc76d66c9. The conflict was in v8-version.h

edit: I misread the above comment. While the patch lands cleanly there is a missing symbol

../deps/v8/src/hydrogen.cc:6723:39: error: no member named 'AssumeMapStable' in 'v8::internal::CompilationDependencies'
          top_info()->dependencies()->AssumeMapStable(cell_value_map);
          ~~~~~~~~~~~~~~~~~~~~~~~~~~  ^

this is no longer on v4.x-staging

@cristiancavalli
Copy link
Author

@ofrobots will do

MylesBorins pushed a commit that referenced this pull request Dec 21, 2016
Original commit message:
  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

  Review-Url: https://codereview.chromium.org/2444233004
  Cr-Commit-Position: refs/heads/master@{#40592}

PR-URL: #10169
Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: ofrobots - Ali Ijaz Sheikh <ofrobots@google.com>
@MylesBorins MylesBorins mentioned this pull request Dec 21, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants