From f14a61620fca2038d60a0cee48ed2c1b88569645 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Fri, 29 Sep 2023 14:38:20 +0200 Subject: [PATCH] vm: don't set host-defined options when it's not necessary This makes it possile to hit the in-isolate compilation cache when host-defined options are not necessary. --- .../vm/compile-script-in-isolate-cache.js | 35 +++++++++ lib/vm.js | 11 ++- src/crypto/crypto_context.cc | 76 +++++++++++++------ src/node_contextify.cc | 27 ++++--- 4 files changed, 111 insertions(+), 38 deletions(-) create mode 100644 benchmark/vm/compile-script-in-isolate-cache.js diff --git a/benchmark/vm/compile-script-in-isolate-cache.js b/benchmark/vm/compile-script-in-isolate-cache.js new file mode 100644 index 00000000000000..f8d0220040c94f --- /dev/null +++ b/benchmark/vm/compile-script-in-isolate-cache.js @@ -0,0 +1,35 @@ +'use strict'; + +// This benchmarks compiling scripts that hit the in-isolate compilation +// cache (by having the same source). +const common = require('../common.js'); +const fs = require('fs'); +const vm = require('vm'); +const fixtures = require('../../test/common/fixtures.js'); +const scriptPath = fixtures.path('snapshot', 'typescript.js'); + +const bench = common.createBenchmark(main, { + type: ['with-dynamic-import-callback', 'without-dynamic-import-callback'], + n: [100], +}); + +const scriptSource = fs.readFileSync(scriptPath, 'utf8'); + +function main({ n, type }) { + let script; + bench.start(); + let options = {}; + switch (type) { + case 'with-dynamic-import-callback': + // Use a dummy callback for now until we really need to benchmark it. + options.importModuleDynamically = async() => {}; + break; + case 'without-dynamic-import-callback': + break; + } + for (let i = 0; i < n; i++) { + script = new vm.Script(scriptSource, options); + } + bench.end(n); + script.runInThisContext(); +} diff --git a/lib/vm.js b/lib/vm.js index 4b9bedec3f4934..c616673f8fc0fb 100644 --- a/lib/vm.js +++ b/lib/vm.js @@ -86,6 +86,12 @@ class Script extends ContextifyScript { } validateBoolean(produceCachedData, 'options.produceCachedData'); + if (importModuleDynamically !== undefined) { + // Check that it's either undefined or a function before we pass + // it into the native constructor. + validateFunction(importModuleDynamically, + 'options.importModuleDynamically'); + } // Calling `ReThrow()` on a native TryCatch does not generate a new // abort-on-uncaught-exception check. A dummy try/catch in JS land // protects against that. @@ -96,14 +102,13 @@ class Script extends ContextifyScript { columnOffset, cachedData, produceCachedData, - parsingContext); + parsingContext, + importModuleDynamically); } catch (e) { throw e; /* node-do-not-add-exception-line */ } if (importModuleDynamically !== undefined) { - validateFunction(importModuleDynamically, - 'options.importModuleDynamically'); const { importModuleDynamicallyWrap } = require('internal/vm/module'); const { registerModule } = require('internal/modules/esm/utils'); registerModule(this, { diff --git a/src/crypto/crypto_context.cc b/src/crypto/crypto_context.cc index 3876adf7d72211..6e5bbe07d0c337 100644 --- a/src/crypto/crypto_context.cc +++ b/src/crypto/crypto_context.cc @@ -1052,34 +1052,60 @@ void SecureContext::LoadPKCS12(const FunctionCallbackInfo& args) { EVP_PKEY* pkey_ptr = nullptr; X509* cert_ptr = nullptr; STACK_OF(X509)* extra_certs_ptr = nullptr; - if (d2i_PKCS12_bio(in.get(), &p12_ptr) && - (p12.reset(p12_ptr), true) && // Move ownership to the smart pointer. - PKCS12_parse(p12.get(), pass.data(), - &pkey_ptr, - &cert_ptr, - &extra_certs_ptr) && - (pkey.reset(pkey_ptr), cert.reset(cert_ptr), - extra_certs.reset(extra_certs_ptr), true) && // Move ownership. - SSL_CTX_use_certificate_chain(sc->ctx_.get(), - std::move(cert), - extra_certs.get(), - &sc->cert_, - &sc->issuer_) && - SSL_CTX_use_PrivateKey(sc->ctx_.get(), pkey.get())) { - // Add CA certs too - for (int i = 0; i < sk_X509_num(extra_certs.get()); i++) { - X509* ca = sk_X509_value(extra_certs.get(), i); - - if (cert_store == GetOrCreateRootCertStore()) { - cert_store = NewRootCertStore(); - SSL_CTX_set_cert_store(sc->ctx_.get(), cert_store); - } - X509_STORE_add_cert(cert_store, ca); - SSL_CTX_add_client_CA(sc->ctx_.get(), ca); + + if (!d2i_PKCS12_bio(in.get(), &p12_ptr)) { + goto done; + } + + // Move ownership to the smart pointer: + p12.reset(p12_ptr); + + if (!PKCS12_parse( + p12.get(), pass.data(), &pkey_ptr, &cert_ptr, &extra_certs_ptr)) { + goto done; + } + + // Move ownership of the parsed data: + pkey.reset(pkey_ptr); + cert.reset(cert_ptr); + extra_certs.reset(extra_certs_ptr); + + if (!pkey) { + return THROW_ERR_CRYPTO_OPERATION_FAILED( + env, "Unable to load private key from PFX data"); + } + + if (!cert) { + return THROW_ERR_CRYPTO_OPERATION_FAILED( + env, "Unable to load certificate from PFX data"); + } + + if (!SSL_CTX_use_certificate_chain(sc->ctx_.get(), + std::move(cert), + extra_certs.get(), + &sc->cert_, + &sc->issuer_)) { + goto done; + } + + if (!SSL_CTX_use_PrivateKey(sc->ctx_.get(), pkey.get())) { + goto done; + } + + // Add CA certs too + for (int i = 0; i < sk_X509_num(extra_certs.get()); i++) { + X509* ca = sk_X509_value(extra_certs.get(), i); + + if (cert_store == GetOrCreateRootCertStore()) { + cert_store = NewRootCertStore(); + SSL_CTX_set_cert_store(sc->ctx_.get(), cert_store); } - ret = true; + X509_STORE_add_cert(cert_store, ca); + SSL_CTX_add_client_CA(sc->ctx_.get(), ca); } + ret = true; +done: if (!ret) { // TODO(@jasnell): Should this use ThrowCryptoError? unsigned long err = ERR_get_error(); // NOLINT(runtime/int) diff --git a/src/node_contextify.cc b/src/node_contextify.cc index b64faf812c6c47..da38602069c3a2 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -771,10 +771,13 @@ void ContextifyScript::New(const FunctionCallbackInfo& args) { bool produce_cached_data = false; Local parsing_context = context; + Local host_defined_options_id; + Local host_defined_options; if (argc > 2) { // new ContextifyScript(code, filename, lineOffset, columnOffset, - // cachedData, produceCachedData, parsingContext) - CHECK_EQ(argc, 7); + // cachedData, produceCachedData, parsingContext, + // needsHostDefinedOptions) + CHECK_EQ(argc, 8); CHECK(args[2]->IsNumber()); line_offset = args[2].As()->Value(); CHECK(args[3]->IsNumber()); @@ -793,6 +796,13 @@ void ContextifyScript::New(const FunctionCallbackInfo& args) { CHECK_NOT_NULL(sandbox); parsing_context = sandbox->context(); } + if (!args[7]->IsUndefined()) { + host_defined_options_id = Symbol::New(isolate, filename); + host_defined_options = + PrimitiveArray::New(isolate, loader::HostDefinedOptions::kLength); + host_defined_options->Set( + isolate, loader::HostDefinedOptions::kID, host_defined_options_id); + } } ContextifyScript* contextify_script = @@ -814,12 +824,6 @@ void ContextifyScript::New(const FunctionCallbackInfo& args) { data + cached_data_buf->ByteOffset(), cached_data_buf->ByteLength()); } - Local host_defined_options = - PrimitiveArray::New(isolate, loader::HostDefinedOptions::kLength); - Local id_symbol = Symbol::New(isolate, filename); - host_defined_options->Set( - isolate, loader::HostDefinedOptions::kID, id_symbol); - ScriptOrigin origin(isolate, filename, line_offset, // line offset @@ -865,8 +869,11 @@ void ContextifyScript::New(const FunctionCallbackInfo& args) { new_cached_data.reset(ScriptCompiler::CreateCodeCache(v8_script)); } - if (contextify_script->object() - ->SetPrivate(context, env->host_defined_option_symbol(), id_symbol) + if (!host_defined_options_id.IsEmpty() && + contextify_script->object() + ->SetPrivate(context, + env->host_defined_option_symbol(), + host_defined_options_id) .IsNothing()) { return; }