Skip to content

Commit

Permalink
vm: properly handle defining props on any value
Browse files Browse the repository at this point in the history
While it was supposed to fix most of the remaining issues,
nodejs#46458 missed some in strict mode.

This PR adds some additional checks. It also clarifies what we are
really checking to execute or not the `GetReturnValue`.
  • Loading branch information
dubzzz committed Feb 11, 2023
1 parent a37c083 commit f5de052
Show file tree
Hide file tree
Showing 3 changed files with 171 additions and 11 deletions.
16 changes: 14 additions & 2 deletions src/node_contextify.cc
Original file line number Diff line number Diff line change
Expand Up @@ -527,10 +527,22 @@ void ContextifyContext::PropertySetterCallback(
!is_function)
return;

Local<Value> desc;
bool is_get_set_property = false;
if (is_declared_on_sandbox &&
ctx->sandbox()->GetOwnPropertyDescriptor(context, property).ToLocal(&desc) &&
desc->IsObject()) {
Local<Object> desc_obj = desc.As<Object>();
Isolate* isolate = context->GetIsolate();
is_get_set_property = desc_obj->HasOwnProperty(context, String::NewFromUtf8(isolate, "get").ToLocalChecked()).FromMaybe(false) ||
desc_obj->HasOwnProperty(context, String::NewFromUtf8(isolate, "set").ToLocalChecked()).FromMaybe(false);
}

USE(ctx->sandbox()->Set(context, property, value));
if (is_contextual_store || is_function) {

// We have to specify the return value for any contextual or get/set property
if (is_get_set_property)
args.GetReturnValue().Set(value);
}
}

// static
Expand Down
155 changes: 146 additions & 9 deletions test/parallel/test-vm-global-setter.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,27 +3,164 @@ const common = require('../common');
const assert = require('assert');
const vm = require('vm');

const getSetSymbolReceivingFunction = Symbol('sym-1');
const getSetSymbolReceivingNumber = Symbol('sym-2');
const symbolReceivingNumber = Symbol('sym-3');
const unknownSymbolReceivingNumber = Symbol('sym-4');

const window = createWindow();

const descriptor =
Object.getOwnPropertyDescriptor(window.globalProxy, 'onhashchange');
const descriptor1 = Object.getOwnPropertyDescriptor(
window.globalProxy,
'getSetPropReceivingFunction'
);
assert.strictEqual(typeof descriptor1.get, 'function');
assert.strictEqual(typeof descriptor1.set, 'function');
assert.strictEqual(descriptor1.configurable, true);

const descriptor2 = Object.getOwnPropertyDescriptor(
window.globalProxy,
'getSetPropReceivingNumber'
);
assert.strictEqual(typeof descriptor2.get, 'function');
assert.strictEqual(typeof descriptor2.set, 'function');
assert.strictEqual(descriptor2.configurable, true);

const descriptor3 = Object.getOwnPropertyDescriptor(
window.globalProxy,
'propReceivingNumber'
);
assert.strictEqual(descriptor3.value, 44);

const descriptor4 = Object.getOwnPropertyDescriptor(
window.globalProxy,
'unknownPropReceivingNumber'
);
assert.strictEqual(descriptor4, undefined);

const descriptor5 = Object.getOwnPropertyDescriptor(
window.globalProxy,
getSetSymbolReceivingFunction
);
assert.strictEqual(typeof descriptor5.get, 'function');
assert.strictEqual(typeof descriptor5.set, 'function');
assert.strictEqual(descriptor5.configurable, true);

const descriptor6 = Object.getOwnPropertyDescriptor(
window.globalProxy,
getSetSymbolReceivingNumber
);
assert.strictEqual(typeof descriptor6.get, 'function');
assert.strictEqual(typeof descriptor6.set, 'function');
assert.strictEqual(descriptor6.configurable, true);

const descriptor7 = Object.getOwnPropertyDescriptor(
window.globalProxy,
symbolReceivingNumber
);
assert.strictEqual(descriptor7.value, 48);

assert.strictEqual(typeof descriptor.get, 'function');
assert.strictEqual(typeof descriptor.set, 'function');
assert.strictEqual(descriptor.configurable, true);
const descriptor8 = Object.getOwnPropertyDescriptor(
window.globalProxy,
unknownSymbolReceivingNumber
);
assert.strictEqual(descriptor8, undefined);

const descriptor9 = Object.getOwnPropertyDescriptor(
window.globalProxy,
'getSetPropThrowing'
);
assert.strictEqual(typeof descriptor9.get, 'function');
assert.strictEqual(typeof descriptor9.set, 'function');
assert.strictEqual(descriptor9.configurable, true);

const descriptor10 = Object.getOwnPropertyDescriptor(
window.globalProxy,
'nonWritableProp'
);
assert.strictEqual(descriptor10.value, 51);
assert.strictEqual(descriptor10.writable, false);

// Regression test for GH-42962. This assignment should not throw.
window.globalProxy.onhashchange = () => {};
window.globalProxy.getSetPropReceivingFunction = () => {};
assert.strictEqual(window.globalProxy.getSetPropReceivingFunction, 42);

window.globalProxy.getSetPropReceivingNumber = 143;
assert.strictEqual(window.globalProxy.getSetPropReceivingNumber, 43);

window.globalProxy.propReceivingNumber = 144;
assert.strictEqual(window.globalProxy.propReceivingNumber, 144);

window.globalProxy.unknownPropReceivingNumber = 145;
assert.strictEqual(window.globalProxy.unknownPropReceivingNumber, 145);

window.globalProxy[getSetSymbolReceivingFunction] = () => {};
assert.strictEqual(window.globalProxy[getSetSymbolReceivingFunction], 46);

window.globalProxy[getSetSymbolReceivingNumber] = 147;
assert.strictEqual(window.globalProxy[getSetSymbolReceivingNumber], 47);

assert.strictEqual(window.globalProxy.onhashchange, 42);
window.globalProxy[symbolReceivingNumber] = 148;
assert.strictEqual(window.globalProxy[symbolReceivingNumber], 148);

window.globalProxy[unknownSymbolReceivingNumber] = 149;
assert.strictEqual(window.globalProxy[unknownSymbolReceivingNumber], 149);

let throwOnSet = false;
try {
window.globalProxy.getSetPropThrowing = 150;
} catch (err) {
throwOnSet = true;
}
assert.strictEqual(window.globalProxy.getSetPropThrowing, 150);
assert.ok(throwOnSet);

throwOnSet = false;
try {
window.globalProxy.nonWritableProp = 151;
} catch (err) {
throwOnSet = true;
}
assert.strictEqual(window.globalProxy.nonWritableProp, 151);
assert.ok(throwOnSet);

function createWindow() {
const obj = {};
vm.createContext(obj);
Object.defineProperty(obj, 'onhashchange', {
Object.defineProperty(obj, 'getSetPropReceivingFunction', {
get: common.mustCall(() => 42),
set: common.mustCall(),
configurable: true
configurable: true,
});
Object.defineProperty(obj, 'getSetPropReceivingNumber', {
get: common.mustCall(() => 43),
set: common.mustCall(),
configurable: true,
});
obj.propReceivingNumber = 44;
// no property for unknownPropReceivingNumber
Object.defineProperty(obj, getSetSymbolReceivingFunction, {
get: common.mustCall(() => 46),
set: common.mustCall(),
configurable: true,
});
Object.defineProperty(obj, getSetSymbolReceivingNumber, {
get: common.mustCall(() => 47),
set: common.mustCall(),
configurable: true,
});
obj[symbolReceivingNumber] = 48;
// no property for unknownSymbolReceivingNumber
Object.defineProperty(obj, 'getSetPropThrowing', {
get: common.mustCall(() => 50),
set: common.mustCall(() => {
throw new Error('setter called');
}),
configurable: true,
});
Object.defineProperty(obj, 'nonWritableProp', {
value: 51,
writable: false,
});

obj.globalProxy = vm.runInContext('this', obj);
Expand Down
11 changes: 11 additions & 0 deletions test/parallel/test-vm-global-symbol.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ const assert = require('assert');
const vm = require('vm');

const global = vm.runInContext('this', vm.createContext());

const totoSymbol = Symbol.for('toto');
Object.defineProperty(global, totoSymbol, {
enumerable: true,
Expand All @@ -13,3 +14,13 @@ Object.defineProperty(global, totoSymbol, {
});
assert.strictEqual(global[totoSymbol], 4);
assert.ok(Object.getOwnPropertySymbols(global).includes(totoSymbol));

const totoKey = 'toto';
Object.defineProperty(global, totoKey, {
enumerable: true,
writable: true,
value: 5,
configurable: true,
});
assert.strictEqual(global[totoKey], 5);
assert.ok(Object.getOwnPropertyNames(global).includes(totoKey));

0 comments on commit f5de052

Please sign in to comment.