Skip to content

Commit d9a68b8

Browse files
dubzzzMoLow
authored andcommitted
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;
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+
constassert=require('assert');
4+
constvm=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+
constglobal=vm.runInContext('this',vm.createContext());
13+
14+
functionrunAssertions(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+
constfun1=()=>1;
28+
constfun2=()=>2;
29+
constfun3=()=>3;
30+
31+
functionrunAssertionsOnSandbox(builder){
32+
constsandboxContext=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+
functionsetPropertyAndAssert(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(propertyindata);
91+
if(typeofproperty==='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+
functiondeletePropertyAndAssert(data,property){
100+
deletedata[property];
101+
assert.strictEqual(data[property],undefined);
102+
assert.ok(!(propertyindata));
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)