Skip to content

Commit

Permalink
vm: properly handle defining symbol props
Browse files Browse the repository at this point in the history
This PR is a follow-up of nodejs#46615.

When bumping V8 for node 18, various bugs appeared around vm.
Some have been fixed along the flow, but there is at least one
remaining and described at:
jestjs/jest#13338.

The current fix does nothing on node 20: the new bump of v8 done for
node 20 seems to fix it. But it would fix the problem in both node 18
and node 19. I confirmed it would fix node 19 by cherry-picking the
commit on v19.x-staging and launching `make -j4 jstest` on it.
  • Loading branch information
dubzzz committed Apr 15, 2023
1 parent 17570c0 commit d164cd4
Show file tree
Hide file tree
Showing 3 changed files with 81 additions and 26 deletions.
1 change: 1 addition & 0 deletions src/node_contextify.cc
Expand Up @@ -527,6 +527,7 @@ void ContextifyContext::PropertySetterCallback(
!is_function)
return;

if (!is_declared && property->IsSymbol()) return;
if (ctx->sandbox()->Set(context, property, value).IsNothing()) return;

Local<Value> desc;
Expand Down
80 changes: 80 additions & 0 deletions test/parallel/test-vm-global-get-own.js
@@ -0,0 +1,80 @@
'use strict';
require('../common');
const assert = require('assert');
const vm = require('vm');

// These assertions check that we can set new keys to the global context,
// get them back and also list them via getOwnProperty*.
//
// Related to:
// - https://github.com/nodejs/node/issues/45983

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

const totoSymbol = Symbol.for('toto');
Object.defineProperty(global, totoSymbol, {
enumerable: true,
writable: true,
value: 4,
configurable: true,
});
assert.strictEqual(global[totoSymbol], 4);
assert.ok(Object.getOwnPropertySymbols(global).includes(totoSymbol));
Object.defineProperty(global, totoSymbol, {
enumerable: true,
writable: true,
value: 44,
configurable: true,
});
assert.strictEqual(global[totoSymbol], 44);
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));
Object.defineProperty(global, totoKey, {
enumerable: true,
writable: true,
value: 55,
configurable: true,
});
assert.strictEqual(global[totoKey], 55);
assert.ok(Object.getOwnPropertyNames(global).includes(totoKey));

const titiSymbol = Symbol.for('titi');
global[titiSymbol] = 6;
assert.strictEqual(global[titiSymbol], 6);
assert.ok(Object.getOwnPropertySymbols(global).includes(titiSymbol));
global[titiSymbol] = 66;
assert.strictEqual(global[titiSymbol], 66);
assert.ok(Object.getOwnPropertySymbols(global).includes(titiSymbol));

const titiKey = 'titi';
global[titiKey] = 7;
assert.strictEqual(global[titiKey], 7);
assert.ok(Object.getOwnPropertyNames(global).includes(titiKey));
global[titiKey] = 77;
assert.strictEqual(global[titiKey], 77);
assert.ok(Object.getOwnPropertyNames(global).includes(titiKey));

const tutuSymbol = Symbol.for('tutu');
global[tutuSymbol] = () => {};
assert.strictEqual(typeof global[tutuSymbol], 'function');
assert.ok(Object.getOwnPropertySymbols(global).includes(tutuSymbol));
global[tutuSymbol] = () => {};
assert.strictEqual(typeof global[tutuSymbol], 'function');
assert.ok(Object.getOwnPropertySymbols(global).includes(tutuSymbol));

const tutuKey = 'tutu';
global[tutuKey] = () => {};
assert.strictEqual(typeof global[tutuKey], 'function');
assert.ok(Object.getOwnPropertyNames(global).includes(tutuKey));
global[tutuKey] = () => {};
assert.strictEqual(typeof global[tutuKey], 'function');
assert.ok(Object.getOwnPropertyNames(global).includes(tutuKey));
26 changes: 0 additions & 26 deletions test/parallel/test-vm-global-symbol.js

This file was deleted.

0 comments on commit d164cd4

Please sign in to comment.