FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

src: perform integrity checks on built-in code cache by joyeecheung · Pull Request #22152 · nodejs/node · GitHub

/ node Public

src: perform integrity checks on built-in code cache - #22152

Closed
joyeecheung wants to merge 1 commit into
nodejs:masterfrom
joyeecheung:hash-cache-check
ConversationCommits (1)ChecksFiles changed
Closed

src: perform integrity checks on built-in code cache#22152
joyeecheung wants to merge 1 commit into
nodejs:masterfrom
joyeecheung:hash-cache-check

Conversation

joyeecheung commented Aug 6, 2018

Copy link
Copy Markdown
Member

Currently V8 only checks that the length of the source code is the
same as the code used to generate the hash, so we add an additional
check here:

  1. During compile time, when generating node_javascript.cc and
    node_code_cache.cc, we compute and include the hash of the
    (unwrapped) JavaScript source in both.
  2. At runtime, we check that the hash of the code being compiled
    and the hash of the code used to generate the cache
    (inside the wrapper) is the same.

This is based on the assumptions:

  1. internalBinding('code_cache_hash') must be in sync with
    internalBinding('code_cache') (same C++ file)
  2. internalBinding('natives_hash') must be in sync with
    process.binding('natives') (same C++ file)
  3. If internalBinding('natives_hash') is in sync with
    internalBinding('natives_hash'), then the (unwrapped)
    code used to generate internalBinding('code_cache')
    should be in sync with the (unwrapped) code in
    process.binding('natives')

There will be, however, false positives if the wrapper used
to generate the cache is different from the one used at run time,
and the length of the wrapper somehow stays the same.
But that should be rare and can be eased once we make the
two bootstrappers cached and checked as well.

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

nodejs-github-bot commented Aug 6, 2018

Copy link
Copy Markdown
Collaborator

@joyeecheung build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/443/pipeline

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Aug 6, 2018

joyeecheung commented Aug 6, 2018

Copy link
Copy Markdown
Member Author

cc @addaleax @hashseed @devsnek @TimothyGu

devsnek reviewed Aug 6, 2018
View reviewed changes
Comment thread lib/internal/bootstrap/cache.js Outdated

devsnek Aug 6, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

getSource: NativeModule.getSource

Comment thread lib/internal/bootstrap/loaders.js Outdated

devsnek Aug 6, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

id

targos commented Aug 6, 2018

Copy link
Copy Markdown
Member

The wrapper issue should disappear once we switch to CompileFunctionInContext (enabled by #21571)

addaleax approved these changes Aug 6, 2018
View reviewed changes

addaleax left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Looks good!

Comment thread tools/js2c.py Outdated

addaleax Aug 6, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Both are fixed one-byte strings, right?

joyeecheung commented Aug 9, 2018

Copy link
Copy Markdown
Member Author

Thanks for the reviews. Addressed the comments.

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

joyeecheung commented Aug 10, 2018

Copy link
Copy Markdown
Member Author

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

joyeecheung mentioned this pull request Aug 10, 2018
Closed

jasnell commented Aug 10, 2018

Copy link
Copy Markdown
Member

CI again on custom-suites: https://ci.nodejs.org/job/node-test-commit-custom-suites/622/
and linux: https://ci.nodejs.org/job/node-test-commit-linux/20616/

joyeecheung commented Aug 11, 2018

Copy link
Copy Markdown
Member Author

Rebuild with freestyle custom suites: https://ci.nodejs.org/job/node-test-pull-request/16368/

joyeecheung commented Aug 13, 2018

Copy link
Copy Markdown
Member Author

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

joyeecheung mentioned this pull request Aug 15, 2018
Closed
src: perform integrity checks on built-in code cache
42e79aa 
Currently V8 only checks that the length of the source code is the
same as the code used to generate the hash, so we add an additional
check here:

1. During compile time, when generating node_javascript.cc and
   node_code_cache.cc, we compute and include the hash of the
  (unwrapped) JavaScript source in both.
2. At runtime, we check that the hash of the code being compiled
  and the hash of the code used to generate the cache
  (inside the wrapper) is the same.

This is based on the assumptions:

1. `internalBinding('code_cache_hash')` must be in sync with
   `internalBinding('code_cache')` (same C++ file)
2. `internalBinding('natives_hash')` must be in sync with
   `process.binding('natives')` (same C++ file)
3. If `internalBinding('natives_hash')` is in sync with
   `internalBinding('natives_hash')`, then the (unwrapped)
   code used to generate `internalBinding('code_cache')`
   should be in sync with the (unwrapped) code in
   `process.binding('natives')`

There will be, however, false positives if the wrapper used
to generate the cache is different from the one used at run time,
and the length of the wrapper somehow stays the same.
But that should be rare and can be eased once we make the
two bootstrappers cached and checked as well.

joyeecheung commented Aug 17, 2018

Copy link
Copy Markdown
Member Author

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

joyeecheung commented Aug 17, 2018

Copy link
Copy Markdown
Member Author

CI is green

joyeecheung added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Aug 17, 2018

joyeecheung commented Aug 17, 2018

Copy link
Copy Markdown
Member Author

Landed in 7cbbb27, thanks!

joyeecheung added a commit that referenced this pull request Aug 17, 2018
src: perform integrity checks on built-in code cache
7cbbb27 
Currently V8 only checks that the length of the source code is the
same as the code used to generate the hash, so we add an additional
check here:

1. During compile time, when generating node_javascript.cc and
   node_code_cache.cc, we compute and include the hash of the
  (unwrapped) JavaScript source in both.
2. At runtime, we check that the hash of the code being compiled
  and the hash of the code used to generate the cache
  (inside the wrapper) is the same.

This is based on the assumptions:

1. `internalBinding('code_cache_hash')` must be in sync with
   `internalBinding('code_cache')` (same C++ file)
2. `internalBinding('natives_hash')` must be in sync with
   `process.binding('natives')` (same C++ file)
3. If `internalBinding('natives_hash')` is in sync with
   `internalBinding('natives_hash')`, then the (unwrapped)
   code used to generate `internalBinding('code_cache')`
   should be in sync with the (unwrapped) code in
   `process.binding('natives')`

There will be, however, false positives if the wrapper used
to generate the cache is different from the one used at run time,
and the length of the wrapper somehow stays the same.
But that should be rare and can be eased once we make the
two bootstrappers cached and checked as well.

PR-URL: #22152
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
targos pushed a commit that referenced this pull request Aug 19, 2018
src: perform integrity checks on built-in code cache
bfe7db9 
Currently V8 only checks that the length of the source code is the
same as the code used to generate the hash, so we add an additional
check here:

1. During compile time, when generating node_javascript.cc and
   node_code_cache.cc, we compute and include the hash of the
  (unwrapped) JavaScript source in both.
2. At runtime, we check that the hash of the code being compiled
  and the hash of the code used to generate the cache
  (inside the wrapper) is the same.

This is based on the assumptions:

1. `internalBinding('code_cache_hash')` must be in sync with
   `internalBinding('code_cache')` (same C++ file)
2. `internalBinding('natives_hash')` must be in sync with
   `process.binding('natives')` (same C++ file)
3. If `internalBinding('natives_hash')` is in sync with
   `internalBinding('natives_hash')`, then the (unwrapped)
   code used to generate `internalBinding('code_cache')`
   should be in sync with the (unwrapped) code in
   `process.binding('natives')`

There will be, however, false positives if the wrapper used
to generate the cache is different from the one used at run time,
and the length of the wrapper somehow stays the same.
But that should be rare and can be eased once we make the
two bootstrappers cached and checked as well.

PR-URL: #22152
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
targos pushed a commit that referenced this pull request Sep 3, 2018
src: perform integrity checks on built-in code cache
4638ce6 
Currently V8 only checks that the length of the source code is the
same as the code used to generate the hash, so we add an additional
check here:

1. During compile time, when generating node_javascript.cc and
   node_code_cache.cc, we compute and include the hash of the
  (unwrapped) JavaScript source in both.
2. At runtime, we check that the hash of the code being compiled
  and the hash of the code used to generate the cache
  (inside the wrapper) is the same.

This is based on the assumptions:

1. `internalBinding('code_cache_hash')` must be in sync with
   `internalBinding('code_cache')` (same C++ file)
2. `internalBinding('natives_hash')` must be in sync with
   `process.binding('natives')` (same C++ file)
3. If `internalBinding('natives_hash')` is in sync with
   `internalBinding('natives_hash')`, then the (unwrapped)
   code used to generate `internalBinding('code_cache')`
   should be in sync with the (unwrapped) code in
   `process.binding('natives')`

There will be, however, false positives if the wrapper used
to generate the cache is different from the one used at run time,
and the length of the wrapper somehow stays the same.
But that should be rare and can be eased once we make the
two bootstrappers cached and checked as well.

PR-URL: #22152
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
targos mentioned this pull request Sep 5, 2018
Merged
This was referenced Sep 6, 2018
Closed
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jasnell jasnell approved these changes
addaleax addaleax approved these changes
+2 more reviewers
TimothyGu TimothyGu approved these changes
devsnek devsnek approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

Back | FazBrowse Home | New Git URL