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 #63549 because
returning `kYes` tells V8 that the definer handled it, and V8 no longer
fixes it up.
Signed-off-by: Chengzhong Wu <cwu631@bloomberg.net>
PR-URL: #64073Fixes: #64008
Reviewed-By: Edy Silva <edigleyssonsilva@gmail.com>

Author: legendecas

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 <typename Fn>
+autoWithPropertyDescriptorCopied(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());
+returncallback(d);
+ };
+if (desc.has_get() || desc.has_set()) {
+ PropertyDescriptor d(
+ desc.has_get() ? desc.get() : Undefined(isolate).As<Value>(),
+ desc.has_set() ? desc.set() : Undefined(isolate).As<Value>());
+returnapply_attrs(d);
+ } elseif (desc.has_value()) {
+if (desc.has_writable()) {
+ PropertyDescriptor d(desc.value(), desc.writable());
+returnapply_attrs(d);
+ }
+ PropertyDescriptor d(desc.value());
+returnapply_attrs(d);
+ }
+ PropertyDescriptor d;
+returnapply_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<Object> 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<Value>(),
- desc.has_set() ? desc.set() : Undefined(isolate).As<Value>());
-
-if (define_prop_on_sandbox(&desc_for_sandbox).FromMaybe(false))
-return Intercepted::kYes;
-return Intercepted::kNo;
- } else {
- Local<Value> value =
- desc.has_value() ? desc.value() : Undefined(isolate).As<Value>();
-
- Maybe<bool> 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

test/parallel/test-vm-property-definer-partial-update.js

@@ -0,0 +1,56 @@
+'use strict';
+
+require('../common');
+constvm=require('vm');
+constassert=require('assert');
+
+// Object.defineProperty should only modify specified attributes and preserve
+// unspecified ones. Regression test for https://github.com/nodejs/node/issues/64008
+{
+constcontext={};
+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);
+}