From 35bb178fc6fa9deec07ada28d6455402e02e5fc9 Mon Sep 17 00:00:00 2001 From: Chengzhong Wu Date: Mon, 22 Jun 2026 14:35:42 -0400 Subject: [PATCH] vm: fix copying PropertyDescriptor This fixes a long-standing issue that `ContextifyContext::PropertyDefinerCallback` incorrectly copies the `const PropertyDescriptor& desc`, assigning value to be `undefined` when no value present on the `PropertyDescriptor`. This is revealed after https://github.com/nodejs/node/pull/63549 because returning `kYes` tells V8 that the definer handled it, and V8 no longer fixes it up. Signed-off-by: Chengzhong Wu --- src/node_contextify.cc | 70 ++++++++++--------- ...test-vm-property-definer-partial-update.js | 56 +++++++++++++++ 2 files changed, 92 insertions(+), 34 deletions(-) create mode 100644 test/parallel/test-vm-property-definer-partial-update.js diff --git a/src/node_contextify.cc b/src/node_contextify.cc index 40ed019a73d83b..dcfaf02c7e55f3 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -89,6 +89,36 @@ using v8::Symbol; using v8::UnboundScript; using v8::Value; +// This is a helper function mediates deleted v8::PropertyDescriptor copy/move. +// The `Fn` receives a mutable reference of a stack allocated +// PropertyDescriptor, copied from the passed in const reference of +// PropertyDescriptor. +template +auto WithPropertyDescriptorCopied(Isolate* isolate, + const PropertyDescriptor& desc, + Fn&& callback) { + auto apply_attrs = [&](PropertyDescriptor& d) { + if (desc.has_enumerable()) d.set_enumerable(desc.enumerable()); + if (desc.has_configurable()) d.set_configurable(desc.configurable()); + return callback(d); + }; + if (desc.has_get() || desc.has_set()) { + PropertyDescriptor d( + desc.has_get() ? desc.get() : Undefined(isolate).As(), + desc.has_set() ? desc.set() : Undefined(isolate).As()); + return apply_attrs(d); + } else if (desc.has_value()) { + if (desc.has_writable()) { + PropertyDescriptor d(desc.value(), desc.writable()); + return apply_attrs(d); + } + PropertyDescriptor d(desc.value()); + return apply_attrs(d); + } + PropertyDescriptor d; + return apply_attrs(d); +} + // The vm module executes code in a sandboxed environment with a different // global object than the rest of the code. This is achieved by applying // every call that changes or queries a property on the global `this` in the @@ -692,41 +722,13 @@ Intercepted ContextifyContext::PropertyDefinerCallback( Local sandbox = ctx->sandbox(); - auto define_prop_on_sandbox = - [&] (PropertyDescriptor* desc_for_sandbox) { - if (desc.has_enumerable()) { - desc_for_sandbox->set_enumerable(desc.enumerable()); - } - if (desc.has_configurable()) { - desc_for_sandbox->set_configurable(desc.configurable()); - } - return sandbox->DefineProperty(context, property, *desc_for_sandbox); - }; + auto result = + WithPropertyDescriptorCopied(isolate, desc, [&](PropertyDescriptor& d) { + return sandbox->DefineProperty(context, property, d); + }); - if (desc.has_get() || desc.has_set()) { - PropertyDescriptor desc_for_sandbox( - desc.has_get() ? desc.get() : Undefined(isolate).As(), - desc.has_set() ? desc.set() : Undefined(isolate).As()); - - if (define_prop_on_sandbox(&desc_for_sandbox).FromMaybe(false)) - return Intercepted::kYes; - return Intercepted::kNo; - } else { - Local value = - desc.has_value() ? desc.value() : Undefined(isolate).As(); - - Maybe result; - if (desc.has_writable()) { - PropertyDescriptor desc_for_sandbox(value, desc.writable()); - result = define_prop_on_sandbox(&desc_for_sandbox); - } else { - PropertyDescriptor desc_for_sandbox(value); - result = define_prop_on_sandbox(&desc_for_sandbox); - } - - if (result.FromMaybe(false)) return Intercepted::kYes; - return Intercepted::kNo; - } + if (result.FromMaybe(false)) return Intercepted::kYes; + return Intercepted::kNo; } // static diff --git a/test/parallel/test-vm-property-definer-partial-update.js b/test/parallel/test-vm-property-definer-partial-update.js new file mode 100644 index 00000000000000..b2291b00a2ba73 --- /dev/null +++ b/test/parallel/test-vm-property-definer-partial-update.js @@ -0,0 +1,56 @@ +'use strict'; + +require('../common'); +const vm = require('vm'); +const assert = require('assert'); + +// Object.defineProperty should only modify specified attributes and preserve +// unspecified ones. Regression test for https://github.com/nodejs/node/issues/64008 +{ + const context = {}; + vm.createContext(context); + + // Changing enumerable should preserve value. + vm.runInContext(` + globalThis.a1 = 1; + Object.defineProperty(globalThis, 'a1', { enumerable: false }); + Object.defineProperty(globalThis, 'a2', { + value: 2, enumerable: false, configurable: true, + }); + Object.defineProperty(globalThis, 'a2', { enumerable: true }); + `, context); + assert.strictEqual(context.a1, 1); + assert.strictEqual( + Object.getOwnPropertyDescriptor(context, 'a1').enumerable, false); + assert.strictEqual(context.a2, 2); + assert.strictEqual( + Object.getOwnPropertyDescriptor(context, 'a2').enumerable, true); + + // Changing writable should preserve value. + vm.runInContext(` + globalThis.b1 = 1; + Object.defineProperty(globalThis, 'b1', { writable: false }); + Object.defineProperty(globalThis, 'b2', { + value: 2, writable: false, configurable: true, + }); + Object.defineProperty(globalThis, 'b2', { writable: true }); + `, context); + assert.strictEqual(context.b1, 1); + assert.strictEqual(context.b2, 2); + + // Changing value should preserve enumerable. + vm.runInContext(` + globalThis.c1 = 1; + Object.defineProperty(globalThis, 'c1', { value: 2 }); + Object.defineProperty(globalThis, 'c2', { + value: 1, enumerable: false, configurable: true, + }); + Object.defineProperty(globalThis, 'c2', { value: 2 }); + `, context); + assert.strictEqual(context.c1, 2); + assert.strictEqual( + Object.getOwnPropertyDescriptor(context, 'c1').enumerable, true); + assert.strictEqual(context.c2, 2); + assert.strictEqual( + Object.getOwnPropertyDescriptor(context, 'c2').enumerable, false); +}