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>

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>
+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<Value>(),
+        desc.has_set() ? desc.set() : Undefined(isolate).As<Value>());
+    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<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');
+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);
+}