★ wanayoo — archive 1999 https://github.com/nodejs/node/pull/22152Nouvelle recherche | Portail wanayoo
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

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

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
7 participants
@joyeecheung
Copy link
Member

joyeecheung commented Aug 6, 2018

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
@joyeecheung

This comment has been minimized.

Copy link
Member Author

joyeecheung commented Aug 6, 2018

@@ -48,6 +52,7 @@ module.exports = {
),
builtinSource: Object.assign({}, NativeModule._source),
getCodeCache,
getSource,

This comment has been minimized.

Copy link
@devsnek

devsnek Aug 6, 2018

Member

getSource: NativeModule.getSource

// But that should be rare and can be eased once we make the
// two bootstrappers cached and checked as well.
const cache = codeCacheHash[id] && (codeCacheHash[id] === sourceHash[id]) ?
codeCache[this.id] : undefined;

This comment has been minimized.

Copy link
@devsnek
@targos

This comment has been minimized.

Copy link
Member

targos commented Aug 6, 2018

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

@addaleax
Copy link
Member

addaleax left a comment

Looks good!

HASH_INITIALIZER = """\
CHECK(target->Set(env->context(),
FIXED_ONE_BYTE_STRING(env->isolate(), "{key}"),
OneByteString(env->isolate(), "{value}")).FromJust());

This comment has been minimized.

Copy link
@addaleax

addaleax Aug 6, 2018

Member

Both are fixed one-byte strings, right?

@joyeecheung

This comment has been minimized.

Copy link
Member Author

joyeecheung commented Aug 9, 2018

Thanks for the reviews. Addressed the comments.

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

@jasnell

jasnell approved these changes Aug 9, 2018

@devsnek

devsnek approved these changes Aug 9, 2018

@joyeecheung

This comment has been minimized.

Copy link
Member Author

joyeecheung commented Aug 10, 2018

@jasnell

This comment has been minimized.

@joyeecheung

This comment has been minimized.

Copy link
Member Author

joyeecheung commented Aug 11, 2018

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

@joyeecheung

This comment has been minimized.

Copy link
Member Author

joyeecheung commented Aug 13, 2018

@joyeecheung joyeecheung referenced this pull request Aug 15, 2018

Closed

CI failures: 20180815 #13

src: perform integrity checks on built-in code cache
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 joyeecheung force-pushed the joyeecheung:hash-cache-check branch from 6c4e775 to 42e79aa Aug 17, 2018

@joyeecheung

This comment has been minimized.

Copy link
Member Author

joyeecheung commented Aug 17, 2018

@joyeecheung

This comment has been minimized.

Copy link
Member Author

joyeecheung commented Aug 17, 2018

CI is green

@joyeecheung

This comment has been minimized.

Copy link
Member Author

joyeecheung commented Aug 17, 2018

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
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 added a commit that referenced this pull request Aug 19, 2018

src: perform integrity checks on built-in code cache
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 added a commit that referenced this pull request Sep 3, 2018

src: perform integrity checks on built-in code cache
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 targos referenced this pull request Sep 5, 2018

Merged

Release proposal: v10.10.0 #22716

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.