Repository navigation
vm module regression in v18: setting properties on global proxy breaks jsdom #42962
Description
Version
v18.1.0
Platform
Microsoft Windows NT 10.0.22000.0 x64
Subsystem
vm
What steps will reproduce the bug?
Put this code in vm-test.js:
"use strict";
const vm = require("vm");
const window = createWindow();
console.log(Object.getOwnPropertyDescriptor(window.globalProxy, "onhashchange"));
window.globalProxy.onhashchange = () => {};
console.log("success");
function createWindow() {
const obj = {};
vm.createContext(obj);
Object.defineProperty(obj, "onhashchange", {
get() { },
set() {},
configurable: true
});
obj.globalProxy = vm.runInContext("this", obj);
return obj;
}How often does it reproduce? Is there a required condition?
Always reproduces
What is the expected behavior?
The v17 behavior was relied upon by jsdom extensively. Using Node v17.9.0 you get:
$ node vm-test.js
{
get: [Function: get],
set: [Function: set],
enumerable: false,
configurable: true
}
success
What do you see instead?
Using Node v18.0.0 or v18.1.0 you get:
$ node vm-test.js
{
get: [Function: get],
set: [Function: set],
enumerable: false,
configurable: true
}
C:\Users\Domenic\Dropbox\GitHub\jsdom\jsdom\vm-test.js:7
window.globalProxy.onhashchange = () => {};
^
TypeError: Cannot redefine property: onhashchange
at Object.<anonymous> (C:\Users\Domenic\Dropbox\GitHub\jsdom\jsdom\vm-test.js:7:33)
at Module._compile (node:internal/modules/cjs/loader:1105:14)
at Module._extensions..js (node:internal/modules/cjs/loader:1159:10)
at Module.load (node:internal/modules/cjs/loader:981:32)
at Module._load (node:internal/modules/cjs/loader:827:12)
at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:77:12)
at node:internal/main/run_main_module:17:47
Node.js v18.1.0
Additional information
No response
Activity
[-]vm module regression in in v18: setting properties on global proxy breaks jsdom[/-][+]vm module regression in v18: setting properties on global proxy breaks jsdom[/+]legendecas commentedon May 4, 2022
Bisected into history I found that #42657 introduced the breakage. Will try to dig out the related V8 changes.
syg commentedon May 4, 2022
The problem seems to be with named interceptors in node_contextify.cc. What's happening is that the setter interceptor, even when setting the property on the backing sandbox object, does not signal that the assignment is in fact now intercepted. So V8 tries to redo the assignment, which uses define semantics when interceptors are present, and doesn't allow redefines to accessors. I don't really grok the interceptor stuff, and since it's special V8 embedder stuff, I'm not entirely surprised it's behaving very weirdly from a JS MOP perspective.
As a fix/workaround, reading https://github.com/nodejs/node/blob/master/src/node_contextify.cc#L461, ISTM the intention is that this interceptor intends to handle the assignment at that point. I.e., V8 should consider the assignment complete and not continue if execution reaches L461 of that setter interceptor. If that's the case, then
args.GetReturnValue()should be set with a non-null value to short-circuit, as per the API.cc @joyeecheung to see if the suggested fix above makes sense.
The behavior change is from https://bugs.chromium.org/p/chromium/issues/detail?id=1309225 (you might not be able to see this due to security sensitivity), where for a security fix it looks like setter interceptors behavior was perhaps inadvertently changed.
I'll try to follow up internally with V8 to understand the behavior of interceptors.
targos commentedon May 4, 2022
#42963
@syg looks like it works!
@domenic I used your example as a test case.
src: always signal V8 for intercepted properties
src: always signal V8 for intercepted properties
src: always signal V8 for intercepted properties
src: always signal V8 for intercepted properties
src: always signal V8 for intercepted properties