FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

vm: properly handle defining symbol props · nodejs/node@d9a68b8 · GitHub

/ node Public

Commit d9a68b8

Browse files
dubzzz
authored and
MoLow
committed
vm: properly handle defining symbol props
This PR is a follow-up of #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. PR-URL: #47572 Reviewed-By: James M Snell <jasnell@gmail.com>
1 parent 1662e89 commit d9a68b8

3 files changed

Lines changed: 106 additions & 26 deletions

File tree

‎src/node_contextify.cc‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -529,6 +529,7 @@ void ContextifyContext::PropertySetterCallback(
529529
!is_function)
530530
return;
531531

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

534535
Local<Value> desc;

‎test/parallel/test-vm-global-get-own.js‎

Lines changed: 105 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,105 @@
1+
'use strict';
2+
require('../common');
3+
const assert = require('assert');
4+
const vm = require('vm');
5+
6+
// These assertions check that we can set new keys to the global context,
7+
// get them back and also list them via getOwnProperty* or in.
8+
//
9+
// Related to:
10+
// - https://github.com/nodejs/node/issues/45983
11+
12+
const global = vm.runInContext('this', vm.createContext());
13+
14+
function runAssertions(data, property, viaDefine, value1, value2, value3) {
15+
// Define the property for the first time
16+
setPropertyAndAssert(data, property, viaDefine, value1);
17+
// Update the property
18+
setPropertyAndAssert(data, property, viaDefine, value2);
19+
// Delete the property
20+
deletePropertyAndAssert(data, property);
21+
// Re-define the property
22+
setPropertyAndAssert(data, property, viaDefine, value3);
23+
// Delete the property again
24+
deletePropertyAndAssert(data, property);
25+
}
26+
27+
const fun1 = () => 1;
28+
const fun2 = () => 2;
29+
const fun3 = () => 3;
30+
31+
function runAssertionsOnSandbox(builder) {
32+
const sandboxContext = vm.createContext({ runAssertions, fun1, fun2, fun3 });
33+
vm.runInContext(builder('this'), sandboxContext);
34+
vm.runInContext(builder('{}'), sandboxContext);
35+
}
36+
37+
// Assertions on: define property
38+
runAssertions(global, 'toto', true, 1, 2, 3);
39+
runAssertions(global, Symbol.for('toto'), true, 1, 2, 3);
40+
runAssertions(global, 'tutu', true, fun1, fun2, fun3);
41+
runAssertions(global, Symbol.for('tutu'), true, fun1, fun2, fun3);
42+
runAssertions(global, 'tyty', true, fun1, 2, 3);
43+
runAssertions(global, Symbol.for('tyty'), true, fun1, 2, 3);
44+
45+
// Assertions on: direct assignment
46+
runAssertions(global, 'titi', false, 1, 2, 3);
47+
runAssertions(global, Symbol.for('titi'), false, 1, 2, 3);
48+
runAssertions(global, 'tata', false, fun1, fun2, fun3);
49+
runAssertions(global, Symbol.for('tata'), false, fun1, fun2, fun3);
50+
runAssertions(global, 'tztz', false, fun1, 2, 3);
51+
runAssertions(global, Symbol.for('tztz'), false, fun1, 2, 3);
52+
53+
// Assertions on: define property from sandbox
54+
runAssertionsOnSandbox(
55+
(variable) => `
56+
runAssertions(${variable}, 'toto', true, 1, 2, 3);
57+
runAssertions(${variable}, Symbol.for('toto'), true, 1, 2, 3);
58+
runAssertions(${variable}, 'tutu', true, fun1, fun2, fun3);
59+
runAssertions(${variable}, Symbol.for('tutu'), true, fun1, fun2, fun3);
60+
runAssertions(${variable}, 'tyty', true, fun1, 2, 3);
61+
runAssertions(${variable}, Symbol.for('tyty'), true, fun1, 2, 3);`
62+
);
63+
64+
// Assertions on: direct assignment from sandbox
65+
runAssertionsOnSandbox(
66+
(variable) => `
67+
runAssertions(${variable}, 'titi', false, 1, 2, 3);
68+
runAssertions(${variable}, Symbol.for('titi'), false, 1, 2, 3);
69+
runAssertions(${variable}, 'tata', false, fun1, fun2, fun3);
70+
runAssertions(${variable}, Symbol.for('tata'), false, fun1, fun2, fun3);
71+
runAssertions(${variable}, 'tztz', false, fun1, 2, 3);
72+
runAssertions(${variable}, Symbol.for('tztz'), false, fun1, 2, 3);`
73+
);
74+
75+
// Helpers
76+
77+
// Set the property on data and assert it worked
78+
function setPropertyAndAssert(data, property, viaDefine, value) {
79+
if (viaDefine) {
80+
Object.defineProperty(data, property, {
81+
enumerable: true,
82+
writable: true,
83+
value: value,
84+
configurable: true,
85+
});
86+
} else {
87+
data[property] = value;
88+
}
89+
assert.strictEqual(data[property], value);
90+
assert.ok(property in data);
91+
if (typeof property === 'string') {
92+
assert.ok(Object.getOwnPropertyNames(data).includes(property));
93+
} else {
94+
assert.ok(Object.getOwnPropertySymbols(data).includes(property));
95+
}
96+
}
97+
98+
// Delete the property from data and assert it worked
99+
function deletePropertyAndAssert(data, property) {
100+
delete data[property];
101+
assert.strictEqual(data[property], undefined);
102+
assert.ok(!(property in data));
103+
assert.ok(!Object.getOwnPropertyNames(data).includes(property));
104+
assert.ok(!Object.getOwnPropertySymbols(data).includes(property));
105+
}

‎test/parallel/test-vm-global-symbol.js‎

Lines changed: 0 additions & 26 deletions
This file was deleted.

0 commit comments

Comments
 (0)
Back | FazBrowse Home | New Git URL