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.

Sign up for GitHub

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

Jump to bottom

deps: V8: cherry-pick e0a109c #27533

Closed
wants to merge 2 commits into from
Closed

deps: V8: cherry-pick e0a109c #27533

wants to merge 2 commits into from

Conversation

joyeecheung
Copy link
Member

Original commit message:

[api] Implement StartupData::CanBeRehashed() for the snapshot blob

This enables the embedder to check if the snapshot generated
from SnapshotCreator::CreateBlob() can be rehashed and the seed
can be recomputed during deserialization.

The lack of this functionality resulted in a temporary vunerability
in Node.js: https://github.com/nodejs/node/pull/27365

Change-Id: I88d52337217c40f79c26438be3c87d2db874d980
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1578661
Commit-Queue: Joyee Cheung <joyee@igalia.com>
Reviewed-by: Yang Guo <yangguo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#61175}

Refs: v8/v8@e0a109c

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot
Copy link
Collaborator

Lite-CI: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/3425

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. v8 engine Issues and PRs related to the V8 dependency. labels May 2, 2019
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/22889/

@joyeecheung
Copy link
Member Author

V8-CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/2266/

@joyeecheung joyeecheung requested a review from targos May 2, 2019 14:54
@joyeecheung
Copy link
Member Author

hmm, somehow on our branch the snapshots generated in the tests are not rehashable https://ci.nodejs.org/job/node-test-commit-v8-linux/2266/

There may be some other commits that we need to backport together? @hashseed

@joyeecheung
Copy link
Member Author

joyeecheung commented May 2, 2019 •
edited
Loading

Ah, the failures are all Check failed: !blob.CanBeRehashed(), which means the ExtractRehashability() on our branch may be bogus..(because having maps in the snapshot here does fail the pummel/test-hash-seed test)

@joyeecheung
deps: V8: cherry-pick e0a109c
2702abb 
Original commit message:

    [api] Implement StartupData::CanBeRehashed() for the snapshot blob

    This enables the embedder to check if the snapshot generated
    from SnapshotCreator::CreateBlob() can be rehashed and the seed
    can be recomputed during deserialization.

    The lack of this functionality resulted in a temporary vunerability
    in Node.js: nodejs#27365

    Change-Id: I88d52337217c40f79c26438be3c87d2db874d980
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1578661
    Commit-Queue: Joyee Cheung <joyee@igalia.com>
    Reviewed-by: Yang Guo <yangguo@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#61175}

Refs: v8/v8@e0a109c
@joyeecheung
fixup! deps: V8: cherry-pick e0a109c
0b06ddf
@joyeecheung
Copy link
Member Author

OK, looks like somehow the patch was applied incorrectly and the assertions went to the wrong places. I've moved them to be in the correct location.

@targos maybe you want to take another look? (though it's just correcting the patch to be the same as the one in the upstream)

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/23658/

@joyeecheung
Copy link
Member Author

V8-CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/2361/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/23723/

@joyeecheung
Copy link
Member Author

V8-CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/2366

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 9, 2019
@joyeecheung
Copy link
Member Author

Landed in d2634be

joyeecheung added a commit that referenced this pull request Jun 11, 2019
@joyeecheung
deps: V8: cherry-pick e0a109c
d2634be 
Original commit message:

    [api] Implement StartupData::CanBeRehashed() for the snapshot blob

    This enables the embedder to check if the snapshot generated
    from SnapshotCreator::CreateBlob() can be rehashed and the seed
    can be recomputed during deserialization.

    The lack of this functionality resulted in a temporary vunerability
    in Node.js: #27365

    Change-Id: I88d52337217c40f79c26438be3c87d2db874d980
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1578661
    Commit-Queue: Joyee Cheung <joyee@igalia.com>
    Reviewed-by: Yang Guo <yangguo@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#61175}

Refs: v8/v8@e0a109c

PR-URL: #27533
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Refael Ackermann (רפאל פלחי) <refack@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
BridgeAR pushed a commit that referenced this pull request Jun 17, 2019
@joyeecheung @BridgeAR
deps: V8: cherry-pick e0a109c
9382b3b 
Original commit message:

    [api] Implement StartupData::CanBeRehashed() for the snapshot blob

    This enables the embedder to check if the snapshot generated
    from SnapshotCreator::CreateBlob() can be rehashed and the seed
    can be recomputed during deserialization.

    The lack of this functionality resulted in a temporary vunerability
    in Node.js: #27365

    Change-Id: I88d52337217c40f79c26438be3c87d2db874d980
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1578661
    Commit-Queue: Joyee Cheung <joyee@igalia.com>
    Reviewed-by: Yang Guo <yangguo@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#61175}

Refs: v8/v8@e0a109c

PR-URL: #27533
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Refael Ackermann (רפאל פלחי) <refack@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@BridgeAR BridgeAR mentioned this pull request Jun 17, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@targos targos targos approved these changes

@Trott Trott Trott approved these changes

@refack refack refack approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. build Issues and PRs related to build files or the CI. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@joyeecheung @nodejs-github-bot @refack @Trott @targos