Skip to content

Commit 35bb178

Browse files
legendecaslegendecas
committed
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>
1 parent 4d3198c commit 35bb178

2 files changed

Lines changed: 92 additions & 34 deletions

File tree

‎src/node_contextify.cc‎

Lines changed: 36 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -89,6 +89,36 @@ using v8::Symbol;
8989
using v8::UnboundScript;
9090
using v8::Value;
9191

92+
// This is a helper function mediates deleted v8::PropertyDescriptor copy/move.
93+
// The `Fn` receives a mutable reference of a stack allocated
94+
// PropertyDescriptor, copied from the passed in const reference of
95+
// PropertyDescriptor.
96+
template <typename Fn>
97+
auto WithPropertyDescriptorCopied(Isolate* isolate,
98+
const PropertyDescriptor& desc,
99+
Fn&& callback) {
100+
auto apply_attrs = [&](PropertyDescriptor& d) {
101+
if (desc.has_enumerable()) d.set_enumerable(desc.enumerable());
102+
if (desc.has_configurable()) d.set_configurable(desc.configurable());
103+
return callback(d);
104+
};
105+
if (desc.has_get() || desc.has_set()) {
106+
PropertyDescriptor d(
107+
desc.has_get() ? desc.get() : Undefined(isolate).As<Value>(),
108+
desc.has_set() ? desc.set() : Undefined(isolate).As<Value>());
109+
return apply_attrs(d);
110+
} else if (desc.has_value()) {
111+
if (desc.has_writable()) {
112+
PropertyDescriptor d(desc.value(), desc.writable());
113+
return apply_attrs(d);
114+
}
115+
PropertyDescriptor d(desc.value());
116+
return apply_attrs(d);
117+
}
118+
PropertyDescriptor d;
119+
return apply_attrs(d);
120+
}
121+
92122
// The vm module executes code in a sandboxed environment with a different
93123
// global object than the rest of the code. This is achieved by applying
94124
// every call that changes or queries a property on the global `this` in the
@@ -692,41 +722,13 @@ Intercepted ContextifyContext::PropertyDefinerCallback(
692722

693723
Local<Object> sandbox = ctx->sandbox();
694724

695-
auto define_prop_on_sandbox =
696-
[&] (PropertyDescriptor* desc_for_sandbox) {
697-
if (desc.has_enumerable()) {
698-
desc_for_sandbox->set_enumerable(desc.enumerable());
699-
}
700-
if (desc.has_configurable()) {
701-
desc_for_sandbox->set_configurable(desc.configurable());
702-
}
703-
return sandbox->DefineProperty(context, property, *desc_for_sandbox);
704-
};
725+
auto result =
726+
WithPropertyDescriptorCopied(isolate, desc, [&](PropertyDescriptor& d) {
727+
return sandbox->DefineProperty(context, property, d);
728+
});
705729

706-
if (desc.has_get() || desc.has_set()) {
707-
PropertyDescriptor desc_for_sandbox(
708-
desc.has_get() ? desc.get() : Undefined(isolate).As<Value>(),
709-
desc.has_set() ? desc.set() : Undefined(isolate).As<Value>());
710-
711-
if (define_prop_on_sandbox(&desc_for_sandbox).FromMaybe(false))
712-
return Intercepted::kYes;
713-
return Intercepted::kNo;
714-
} else {
715-
Local<Value> value =
716-
desc.has_value() ? desc.value() : Undefined(isolate).As<Value>();
717-
718-
Maybe<bool> result;
719-
if (desc.has_writable()) {
720-
PropertyDescriptor desc_for_sandbox(value, desc.writable());
721-
result = define_prop_on_sandbox(&desc_for_sandbox);
722-
} else {
723-
PropertyDescriptor desc_for_sandbox(value);
724-
result = define_prop_on_sandbox(&desc_for_sandbox);
725-
}
726-
727-
if (result.FromMaybe(false)) return Intercepted::kYes;
728-
return Intercepted::kNo;
729-
}
730+
if (result.FromMaybe(false)) return Intercepted::kYes;
731+
return Intercepted::kNo;
730732
}
731733

732734
// static

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

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,56 @@
1+
'use strict';
2+
3+
require('../common');
4+
const vm = require('vm');
5+
const assert = require('assert');
6+
7+
// Object.defineProperty should only modify specified attributes and preserve
8+
// unspecified ones. Regression test for https://github.com/nodejs/node/issues/64008
9+
{
10+
const context = {};
11+
vm.createContext(context);
12+
13+
// Changing enumerable should preserve value.
14+
vm.runInContext(`
15+
globalThis.a1 = 1;
16+
Object.defineProperty(globalThis, 'a1', { enumerable: false });
17+
Object.defineProperty(globalThis, 'a2', {
18+
value: 2, enumerable: false, configurable: true,
19+
});
20+
Object.defineProperty(globalThis, 'a2', { enumerable: true });
21+
`, context);
22+
assert.strictEqual(context.a1, 1);
23+
assert.strictEqual(
24+
Object.getOwnPropertyDescriptor(context, 'a1').enumerable, false);
25+
assert.strictEqual(context.a2, 2);
26+
assert.strictEqual(
27+
Object.getOwnPropertyDescriptor(context, 'a2').enumerable, true);
28+
29+
// Changing writable should preserve value.
30+
vm.runInContext(`
31+
globalThis.b1 = 1;
32+
Object.defineProperty(globalThis, 'b1', { writable: false });
33+
Object.defineProperty(globalThis, 'b2', {
34+
value: 2, writable: false, configurable: true,
35+
});
36+
Object.defineProperty(globalThis, 'b2', { writable: true });
37+
`, context);
38+
assert.strictEqual(context.b1, 1);
39+
assert.strictEqual(context.b2, 2);
40+
41+
// Changing value should preserve enumerable.
42+
vm.runInContext(`
43+
globalThis.c1 = 1;
44+
Object.defineProperty(globalThis, 'c1', { value: 2 });
45+
Object.defineProperty(globalThis, 'c2', {
46+
value: 1, enumerable: false, configurable: true,
47+
});
48+
Object.defineProperty(globalThis, 'c2', { value: 2 });
49+
`, context);
50+
assert.strictEqual(context.c1, 2);
51+
assert.strictEqual(
52+
Object.getOwnPropertyDescriptor(context, 'c1').enumerable, true);
53+
assert.strictEqual(context.c2, 2);
54+
assert.strictEqual(
55+
Object.getOwnPropertyDescriptor(context, 'c2').enumerable, false);
56+
}

0 commit comments

Comments
 (0)