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

src: use new V8 API in vm module · nodejs/node@f1d6b04 · GitHub

/ node Public

Commit f1d6b04

Browse files
fhinkel
committed
src: use new V8 API in vm module
Remove the CopyProperties() hack in the vm module, i.e., Contextify. Use different V8 API methods, that allow interception of DefineProperty() and do not flatten accessor descriptors to property descriptors. Move many known issues to test cases. Factor out the last test in test-vm-context.js for #10223 into its own file, test-vm-strict-assign.js. Part of this CL is taken from a stalled PR by https://github.com/AnnaMag #13265 This PR requires a backport of https://chromium.googlesource.com/v8/v8/+/37a3a15c3e52e2146e45f41c427f24414e4d7f6f PR-URL: #16293 Fixes: #2734 Fixes: #10223 Fixes: #11803 Fixes: #11902 Ref: #6283 Ref: #15114 Ref: #13265 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>
1 parent 556ebab commit f1d6b04

11 files changed

Lines changed: 237 additions & 195 deletions

‎src/node_contextify.cc‎

Lines changed: 183 additions & 120 deletions
Large diffs are not rendered by default.

‎test/known_issues/test-vm-attributes-property-not-on-sandbox.js‎

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

‎test/parallel/test-vm-attributes-property-not-on-sandbox.js‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
'use strict';
2+
require('../common');
3+
const assert = require('assert');
4+
const vm = require('vm');
5+
6+
// Assert that accessor descriptors are not flattened on the sandbox.
7+
// Issue: https://github.com/nodejs/node/issues/2734
8+
const sandbox = {};
9+
vm.createContext(sandbox);
10+
const code = `Object.defineProperty(
11+
this,
12+
'foo',
13+
{ get: function() {return 17} }
14+
);
15+
var desc = Object.getOwnPropertyDescriptor(this, 'foo');`;
16+
17+
vm.runInContext(code, sandbox);
18+
assert.strictEqual(typeof sandbox.desc.get, 'function');

‎test/parallel/test-vm-context.js‎

Lines changed: 0 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -106,16 +106,3 @@ assert.throws(() => {
106106
// https://github.com/nodejs/node/issues/6158
107107
ctx = new Proxy({}, {});
108108
assert.strictEqual(typeof vm.runInNewContext('String', ctx), 'function');
109-
110-
// https://github.com/nodejs/node/issues/10223
111-
ctx = vm.createContext();
112-
vm.runInContext('Object.defineProperty(this, "x", { value: 42 })', ctx);
113-
assert.strictEqual(ctx.x, 42);
114-
assert.strictEqual(vm.runInContext('x', ctx), 42);
115-
116-
vm.runInContext('x = 0', ctx); // Does not throw but x...
117-
assert.strictEqual(vm.runInContext('x', ctx), 42); // ...should be unaltered.
118-
119-
assert.throws(() => vm.runInContext('"use strict"; x = 0', ctx),
120-
/Cannot assign to read only property 'x'/);
121-
assert.strictEqual(vm.runInContext('x', ctx), 42);

test/known_issues/test-vm-data-property-writable.js renamed to test/parallel/test-vm-data-property-writable.js

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,11 +7,22 @@ const assert = require('assert');
77

88
const context = vm.createContext({});
99

10-
const code = `
10+
let code = `
1111
Object.defineProperty(this, 'foo', {value: 5});
1212
Object.getOwnPropertyDescriptor(this, 'foo');
1313
`;
1414

15-
const desc = vm.runInContext(code, context);
15+
let desc = vm.runInContext(code, context);
1616

1717
assert.strictEqual(desc.writable, false);
18+
19+
// Check that interceptors work for symbols.
20+
code = `
21+
const bar = Symbol('bar');
22+
Object.defineProperty(this, bar, {value: 6});
23+
Object.getOwnPropertyDescriptor(this, bar);
24+
`;
25+
26+
desc = vm.runInContext(code, context);
27+
28+
assert.strictEqual(desc.value, 6);

test/known_issues/test-vm-getters.js renamed to test/parallel/test-vm-getters.js

File renamed without changes.

‎test/parallel/test-vm-global-define-property.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -44,4 +44,4 @@ assert(res);
4444
assert.strictEqual(typeof res, 'object');
4545
assert.strictEqual(res, x);
4646
assert.strictEqual(o.f, res);
47-
assert.deepStrictEqual(Object.keys(o), ['console', 'x', 'g', 'f']);
47+
assert.deepStrictEqual(Object.keys(o), ['console', 'x', 'f', 'g']);

test/known_issues/test-vm-global-non-writable-properties.js renamed to test/parallel/test-vm-global-non-writable-properties.js

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@ const vm = require('vm');
77

88
const ctx = vm.createContext();
99
vm.runInContext('Object.defineProperty(this, "x", { value: 42 })', ctx);
10-
assert.strictEqual(ctx.x, undefined); // Not copied out by cloneProperty().
1110
assert.strictEqual(vm.runInContext('x', ctx), 42);
1211
vm.runInContext('x = 0', ctx); // Does not throw but x...
1312
assert.strictEqual(vm.runInContext('x', ctx), 42); // ...should be unaltered.

test/known_issues/test-vm-proxy-failure-CP.js renamed to test/parallel/test-vm-proxy-failure-CP.js

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,10 @@
11
'use strict';
2-
3-
// Sandbox throws in CopyProperties() despite no code being run
4-
// Issue: https://github.com/nodejs/node/issues/11902
5-
6-
72
require('../common');
83
const assert = require('assert');
94
const vm = require('vm');
105

6+
// Check that we do not accidentally query attributes.
7+
// Issue: https://github.com/nodejs/node/issues/11902
118
const handler = {
129
getOwnPropertyDescriptor: (target, prop) => {
1310
throw new Error('whoops');
@@ -16,5 +13,4 @@ const handler = {
1613
const sandbox = new Proxy({ foo: 'bar' }, handler);
1714
const context = vm.createContext(sandbox);
1815

19-
2016
assert.doesNotThrow(() => vm.runInContext('', context));

‎test/parallel/test-vm-strict-assign.js‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
'use strict';
2+
require('../common');
3+
const assert = require('assert');
4+
5+
const vm = require('vm');
6+
7+
// https://github.com/nodejs/node/issues/10223
8+
const ctx = vm.createContext();
9+
10+
// Define x with writable = false.
11+
vm.runInContext('Object.defineProperty(this, "x", { value: 42 })', ctx);
12+
assert.strictEqual(ctx.x, 42);
13+
assert.strictEqual(vm.runInContext('x', ctx), 42);
14+
15+
vm.runInContext('x = 0', ctx); // Does not throw but x...
16+
assert.strictEqual(vm.runInContext('x', ctx), 42); // ...should be unaltered.
17+
18+
assert.throws(() => vm.runInContext('"use strict"; x = 0', ctx),
19+
/Cannot assign to read only property 'x'/);
20+
assert.strictEqual(vm.runInContext('x', ctx), 42);

0 commit comments

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