From 1ac7d56d47d1ff1c2c1f4cc621704a25b86dec11 Mon Sep 17 00:00:00 2001 From: Divy Srivastava Date: Wed, 12 Jun 2024 22:32:54 +0530 Subject: [PATCH] fix(ext/node): fix vm memory usage and context initialization (#23976) Fixes https://github.com/denoland/deno/issues/22441 Fixes https://github.com/denoland/deno/issues/23913 Fixes https://github.com/denoland/deno/issues/23852 Fixes https://github.com/denoland/deno/issues/23917 --- .github/workflows/ci.generate.ts | 12 +++- .github/workflows/ci.yml | 8 +-- ext/node/global.rs | 2 +- ext/node/lib.rs | 63 ++++++++++++++++- ext/node/ops/vm.rs | 17 +++++ ext/node/ops/vm_internal.rs | 115 +++++++++++++++++++++---------- runtime/snapshot.rs | 10 ++- tests/unit_node/vm_test.ts | 31 +++++++++ 8 files changed, 213 insertions(+), 45 deletions(-) diff --git a/.github/workflows/ci.generate.ts b/.github/workflows/ci.generate.ts index e86e13971e179f..95218dae79b93b 100755 --- a/.github/workflows/ci.generate.ts +++ b/.github/workflows/ci.generate.ts @@ -5,7 +5,7 @@ import { stringify } from "jsr:@std/yaml@^0.221/stringify"; // Bump this number when you want to purge the cache. // Note: the tools/release/01_bump_crate_versions.ts script will update this version // automatically via regex, so ensure that this line maintains this format. -const cacheVersion = 95; +const cacheVersion = 96; const ubuntuX86Runner = "ubuntu-22.04"; const ubuntuX86XlRunner = "ubuntu-22.04-xl"; @@ -688,6 +688,16 @@ const ci = { ].join("\n"), env: { CARGO_PROFILE_DEV_DEBUG: 0 }, }, + // Uncomment for remote debugging + // { + // name: "Setup tmate session", + // if: [ + // "(matrix.job == 'test' || matrix.job == 'bench') &&", + // "matrix.profile == 'release' && (matrix.use_sysroot ||", + // "github.repository == 'denoland/deno')", + // ].join("\n"), + // uses: "mxschmitt/action-tmate@v3", + // }, { name: "Build release", if: [ diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 81af5dc28dbca2..d084528ec73286 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -367,8 +367,8 @@ jobs: path: |- ~/.cargo/registry/index ~/.cargo/registry/cache - key: '95-cargo-home-${{ matrix.os }}-${{ matrix.arch }}-${{ hashFiles(''Cargo.lock'') }}' - restore-keys: '95-cargo-home-${{ matrix.os }}-${{ matrix.arch }}' + key: '96-cargo-home-${{ matrix.os }}-${{ matrix.arch }}-${{ hashFiles(''Cargo.lock'') }}' + restore-keys: '96-cargo-home-${{ matrix.os }}-${{ matrix.arch }}' if: '!(matrix.skip)' - name: Restore cache build output (PR) uses: actions/cache/restore@v4 @@ -380,7 +380,7 @@ jobs: !./target/*/*.zip !./target/*/*.tar.gz key: never_saved - restore-keys: '95-cargo-target-${{ matrix.os }}-${{ matrix.arch }}-${{ matrix.profile }}-${{ matrix.job }}-' + restore-keys: '96-cargo-target-${{ matrix.os }}-${{ matrix.arch }}-${{ matrix.profile }}-${{ matrix.job }}-' - name: Apply and update mtime cache if: '!(matrix.skip) && (!startsWith(github.ref, ''refs/tags/''))' uses: ./.github/mtime_cache @@ -669,7 +669,7 @@ jobs: !./target/*/gn_out !./target/*/*.zip !./target/*/*.tar.gz - key: '95-cargo-target-${{ matrix.os }}-${{ matrix.arch }}-${{ matrix.profile }}-${{ matrix.job }}-${{ github.sha }}' + key: '96-cargo-target-${{ matrix.os }}-${{ matrix.arch }}-${{ matrix.profile }}-${{ matrix.job }}-${{ github.sha }}' publish-canary: name: publish canary runs-on: ubuntu-22.04 diff --git a/ext/node/global.rs b/ext/node/global.rs index 5c95b2f8fba02a..e01fca95e1dc55 100644 --- a/ext/node/global.rs +++ b/ext/node/global.rs @@ -275,7 +275,7 @@ fn current_mode(scope: &mut v8::HandleScope) -> Mode { }; let mut buffer = [MaybeUninit::uninit(); 2048]; let str = v8_string.to_rust_cow_lossy(scope, &mut buffer); - if node_resolver.in_npm_package_with_cache(str) { + if str.starts_with("node:") || node_resolver.in_npm_package_with_cache(str) { Mode::Node } else { Mode::Deno diff --git a/ext/node/lib.rs b/ext/node/lib.rs index 094bea3aeec753..8a20b3109b87a4 100644 --- a/ext/node/lib.rs +++ b/ext/node/lib.rs @@ -35,6 +35,10 @@ mod resolution; pub use ops::ipc::ChildPipeFd; pub use ops::ipc::IpcJsonStreamResource; pub use ops::v8::VM_CONTEXT_INDEX; +use ops::vm; +pub use ops::vm::create_v8_context; +pub use ops::vm::init_global_template; +pub use ops::vm::ContextInitMode; pub use package_json::PackageJson; pub use path::PathClean; pub use polyfill::is_builtin_node_module; @@ -641,7 +645,64 @@ deno_core::extension!(deno_node, global_template_middleware = global_template_middleware, global_object_middleware = global_object_middleware, customizer = |ext: &mut deno_core::Extension| { - let mut external_references = Vec::with_capacity(7); + let mut external_references = Vec::with_capacity(14); + + vm::GETTER_MAP_FN.with(|getter| { + external_references.push(ExternalReference { + named_getter: *getter, + }); + }); + vm::SETTER_MAP_FN.with(|setter| { + external_references.push(ExternalReference { + named_setter: *setter, + }); + }); + vm::DELETER_MAP_FN.with(|deleter| { + external_references.push(ExternalReference { + named_getter: *deleter, + },); + }); + vm::ENUMERATOR_MAP_FN.with(|enumerator| { + external_references.push(ExternalReference { + enumerator: *enumerator, + }); + }); + vm::DEFINER_MAP_FN.with(|definer| { + external_references.push(ExternalReference { + named_definer: *definer, + }); + }); + vm::DESCRIPTOR_MAP_FN.with(|descriptor| { + external_references.push(ExternalReference { + named_getter: *descriptor, + }); + }); + + vm::INDEXED_GETTER_MAP_FN.with(|getter| { + external_references.push(ExternalReference { + indexed_getter: *getter, + }); + }); + vm::INDEXED_SETTER_MAP_FN.with(|setter| { + external_references.push(ExternalReference { + indexed_setter: *setter, + }); + }); + vm::INDEXED_DELETER_MAP_FN.with(|deleter| { + external_references.push(ExternalReference { + indexed_getter: *deleter, + }); + }); + vm::INDEXED_DEFINER_MAP_FN.with(|definer| { + external_references.push(ExternalReference { + indexed_definer: *definer, + }); + }); + vm::INDEXED_DESCRIPTOR_MAP_FN.with(|descriptor| { + external_references.push(ExternalReference { + indexed_getter: *descriptor, + }); + }); global::GETTER_MAP_FN.with(|getter| { external_references.push(ExternalReference { diff --git a/ext/node/ops/vm.rs b/ext/node/ops/vm.rs index f18038f8f05ab2..a0a63cc6e5a2d6 100644 --- a/ext/node/ops/vm.rs +++ b/ext/node/ops/vm.rs @@ -7,6 +7,23 @@ use deno_core::v8; use super::vm_internal as i; +pub use i::create_v8_context; +pub use i::init_global_template; +pub use i::ContextInitMode; + +pub use i::DEFINER_MAP_FN; +pub use i::DELETER_MAP_FN; +pub use i::DESCRIPTOR_MAP_FN; +pub use i::ENUMERATOR_MAP_FN; +pub use i::GETTER_MAP_FN; +pub use i::SETTER_MAP_FN; + +pub use i::INDEXED_DEFINER_MAP_FN; +pub use i::INDEXED_DELETER_MAP_FN; +pub use i::INDEXED_DESCRIPTOR_MAP_FN; +pub use i::INDEXED_GETTER_MAP_FN; +pub use i::INDEXED_SETTER_MAP_FN; + pub struct Script { inner: i::ContextifyScript, } diff --git a/ext/node/ops/vm_internal.rs b/ext/node/ops/vm_internal.rs index 54b9aaa4716c89..4af1825954300e 100644 --- a/ext/node/ops/vm_internal.rs +++ b/ext/node/ops/vm_internal.rs @@ -69,9 +69,9 @@ impl ContextifyContext { scope: &mut v8::HandleScope, sandbox_obj: v8::Local, ) { - let tmp = init_global_template(scope); + let tmp = init_global_template(scope, ContextInitMode::UseSnapshot); - let context = create_v8_context(scope, tmp, None); + let context = create_v8_context(scope, tmp, ContextInitMode::UseSnapshot); Self::from_context(scope, context, sandbox_obj); } @@ -84,6 +84,8 @@ impl ContextifyContext { let context_state = main_context.get_aligned_pointer_from_embedder_data( deno_core::CONTEXT_STATE_SLOT_INDEX, ); + let module_map = main_context + .get_aligned_pointer_from_embedder_data(deno_core::MODULE_MAP_SLOT_INDEX); v8_context.set_security_token(main_context.get_security_token(scope)); // SAFETY: set embedder data from the creation context @@ -92,6 +94,10 @@ impl ContextifyContext { deno_core::CONTEXT_STATE_SLOT_INDEX, context_state, ); + v8_context.set_aligned_pointer_in_embedder_data( + deno_core::MODULE_MAP_SLOT_INDEX, + module_map, + ); } let context = v8::Global::new(scope, v8_context); @@ -106,7 +112,7 @@ impl ContextifyContext { // lives longer than the execution context, so this should be safe. unsafe { v8_context.set_aligned_pointer_in_embedder_data( - 2, + 3, ptr as *const ContextifyContext as _, ); } @@ -168,7 +174,10 @@ impl ContextifyContext { ) -> Option<&'c ContextifyContext> { let context = object.get_creation_context(scope)?; - let context_ptr = context.get_aligned_pointer_from_embedder_data(2); + let context_ptr = context.get_aligned_pointer_from_embedder_data(3); + if context_ptr.is_null() { + return None; + } // SAFETY: We are storing a pointer to the ContextifyContext // in the embedder data of the v8::Context during creation. Some(unsafe { &*(context_ptr as *const ContextifyContext) }) @@ -177,17 +186,31 @@ impl ContextifyContext { pub const VM_CONTEXT_INDEX: usize = 0; -fn create_v8_context<'a>( - scope: &mut v8::HandleScope<'a>, +#[derive(PartialEq)] +pub enum ContextInitMode { + ForSnapshot, + UseSnapshot, +} + +pub fn create_v8_context<'a>( + scope: &mut v8::HandleScope<'a, ()>, object_template: v8::Local, - snapshot_data: Option<&'static [u8]>, + mode: ContextInitMode, ) -> v8::Local<'a, v8::Context> { let scope = &mut v8::EscapableHandleScope::new(scope); - let context = if let Some(_snapshot_data) = snapshot_data { + let context = if mode == ContextInitMode::UseSnapshot { v8::Context::from_snapshot(scope, VM_CONTEXT_INDEX).unwrap() } else { - v8::Context::new_from_template(scope, object_template) + let ctx = v8::Context::new_from_template(scope, object_template); + // SAFETY: ContextifyContexts will update this to a pointer to the native object + unsafe { + ctx.set_aligned_pointer_in_embedder_data(1, std::ptr::null_mut()); + ctx.set_aligned_pointer_in_embedder_data(2, std::ptr::null_mut()); + ctx.set_aligned_pointer_in_embedder_data(3, std::ptr::null_mut()); + ctx.clear_all_slots(scope); + }; + ctx }; scope.escape(context) @@ -196,25 +219,31 @@ fn create_v8_context<'a>( #[derive(Debug, Clone)] struct SlotContextifyGlobalTemplate(v8::Global); -fn init_global_template<'a>( - scope: &mut v8::HandleScope<'a>, +pub fn init_global_template<'a>( + scope: &mut v8::HandleScope<'a, ()>, + mode: ContextInitMode, ) -> v8::Local<'a, v8::ObjectTemplate> { - let mut maybe_object_template_slot = + let maybe_object_template_slot = scope.get_slot::(); if maybe_object_template_slot.is_none() { - init_global_template_inner(scope); - maybe_object_template_slot = - scope.get_slot::(); + let global_object_template = init_global_template_inner(scope); + + if mode == ContextInitMode::UseSnapshot { + let contextify_global_template_slot = SlotContextifyGlobalTemplate( + v8::Global::new(scope, global_object_template), + ); + scope.set_slot(contextify_global_template_slot); + } + global_object_template + } else { + let object_template_slot = maybe_object_template_slot + .expect("ContextifyGlobalTemplate slot should be already populated.") + .clone(); + v8::Local::new(scope, object_template_slot.0) } - let object_template_slot = maybe_object_template_slot - .expect("ContextifyGlobalTemplate slot should be already populated.") - .clone(); - v8::Local::new(scope, object_template_slot.0) } -extern "C" fn c_noop(_: *const v8::FunctionCallbackInfo) {} - // Using thread_local! to get around compiler bug. // // See NOTE in ext/node/global.rs#L12 @@ -235,11 +264,11 @@ thread_local! { pub static INDEXED_DESCRIPTOR_MAP_FN: v8::IndexedPropertyGetterCallback<'static> = indexed_property_descriptor.map_fn_to(); } -fn init_global_template_inner(scope: &mut v8::HandleScope) { - let global_func_template = - v8::FunctionTemplate::builder_raw(c_noop).build(scope); - let global_object_template = global_func_template.instance_template(scope); - global_object_template.set_internal_field_count(2); +pub fn init_global_template_inner<'a>( + scope: &mut v8::HandleScope<'a, ()>, +) -> v8::Local<'a, v8::ObjectTemplate> { + let global_object_template = v8::ObjectTemplate::new(scope); + global_object_template.set_internal_field_count(3); let named_property_handler_config = { let mut config = v8::NamedPropertyHandlerConfiguration::new() @@ -279,10 +308,8 @@ fn init_global_template_inner(scope: &mut v8::HandleScope) { .set_named_property_handler(named_property_handler_config); global_object_template .set_indexed_property_handler(indexed_property_handler_config); - let contextify_global_template_slot = SlotContextifyGlobalTemplate( - v8::Global::new(scope, global_object_template), - ); - scope.set_slot(contextify_global_template_slot); + + global_object_template } fn property_getter<'s>( @@ -291,7 +318,9 @@ fn property_getter<'s>( args: v8::PropertyCallbackArguments<'s>, mut ret: v8::ReturnValue, ) -> v8::Intercepted { - let ctx = ContextifyContext::get(scope, args.this()).unwrap(); + let Some(ctx) = ContextifyContext::get(scope, args.this()) else { + return v8::Intercepted::No; + }; let sandbox = ctx.sandbox(scope); @@ -325,7 +354,9 @@ fn property_setter<'s>( args: v8::PropertyCallbackArguments<'s>, mut rv: v8::ReturnValue, ) -> v8::Intercepted { - let ctx = ContextifyContext::get(scope, args.this()).unwrap(); + let Some(ctx) = ContextifyContext::get(scope, args.this()) else { + return v8::Intercepted::No; + }; let (attributes, is_declared_on_global_proxy) = match ctx .global_proxy(scope) @@ -416,7 +447,9 @@ fn property_deleter<'s>( args: v8::PropertyCallbackArguments<'s>, mut rv: v8::ReturnValue, ) -> v8::Intercepted { - let ctx = ContextifyContext::get(scope, args.this()).unwrap(); + let Some(ctx) = ContextifyContext::get(scope, args.this()) else { + return v8::Intercepted::No; + }; let context = ctx.context(scope); let sandbox = ctx.sandbox(scope); @@ -434,7 +467,9 @@ fn property_enumerator<'s>( args: v8::PropertyCallbackArguments<'s>, mut rv: v8::ReturnValue, ) { - let ctx = ContextifyContext::get(scope, args.this()).unwrap(); + let Some(ctx) = ContextifyContext::get(scope, args.this()) else { + return; + }; let context = ctx.context(scope); let sandbox = ctx.sandbox(scope); @@ -455,7 +490,9 @@ fn property_definer<'s>( args: v8::PropertyCallbackArguments<'s>, _: v8::ReturnValue, ) -> v8::Intercepted { - let ctx = ContextifyContext::get(scope, args.this()).unwrap(); + let Some(ctx) = ContextifyContext::get(scope, args.this()) else { + return v8::Intercepted::No; + }; let context = ctx.context(scope); let (attributes, is_declared) = match ctx @@ -533,7 +570,9 @@ fn property_descriptor<'s>( args: v8::PropertyCallbackArguments<'s>, mut rv: v8::ReturnValue, ) -> v8::Intercepted { - let ctx = ContextifyContext::get(scope, args.this()).unwrap(); + let Some(ctx) = ContextifyContext::get(scope, args.this()) else { + return v8::Intercepted::No; + }; let context = ctx.context(scope); let sandbox = ctx.sandbox(scope); @@ -585,7 +624,9 @@ fn indexed_property_deleter<'s>( args: v8::PropertyCallbackArguments<'s>, mut rv: v8::ReturnValue, ) -> v8::Intercepted { - let ctx = ContextifyContext::get(scope, args.this()).unwrap(); + let Some(ctx) = ContextifyContext::get(scope, args.this()) else { + return v8::Intercepted::No; + }; let context = ctx.context(scope); let sandbox = ctx.sandbox(scope); diff --git a/runtime/snapshot.rs b/runtime/snapshot.rs index 7ae5e3ae5b861d..2144ff07a2927a 100644 --- a/runtime/snapshot.rs +++ b/runtime/snapshot.rs @@ -285,7 +285,15 @@ pub fn create_runtime_snapshot( let isolate = rt.v8_isolate(); let scope = &mut v8::HandleScope::new(isolate); - let ctx = v8::Context::new(scope); + let tmpl = deno_node::init_global_template( + scope, + deno_node::ContextInitMode::ForSnapshot, + ); + let ctx = deno_node::create_v8_context( + scope, + tmpl, + deno_node::ContextInitMode::ForSnapshot, + ); assert_eq!(scope.add_context(ctx), deno_node::VM_CONTEXT_INDEX); })), skip_op_registration: false, diff --git a/tests/unit_node/vm_test.ts b/tests/unit_node/vm_test.ts index 21e56ee94313f8..46e231c6ca72bf 100644 --- a/tests/unit_node/vm_test.ts +++ b/tests/unit_node/vm_test.ts @@ -148,3 +148,34 @@ reject().catch(() => {}) script.runInNewContext(); }, }); + +// https://github.com/denoland/deno/issues/22441 +Deno.test({ + name: "vm runInNewContext module loader", + fn() { + const code = "import('node:process')"; + const script = new Script(code); + script.runInNewContext(); + }, +}); + +// https://github.com/denoland/deno/issues/23913 +Deno.test({ + name: "vm memory leak crash", + fn() { + const script = new Script("returnValue = 2+2"); + + for (let i = 0; i < 1000; i++) { + script.runInNewContext({}, { timeout: 10000 }); + } + }, +}); + +// https://github.com/denoland/deno/issues/23852 +Deno.test({ + name: "vm runInThisContext global.foo", + fn() { + const result = runInThisContext(`global.foo = 1`); + assertEquals(result, 1); + }, +});