Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

VM module doesn't handle undefined/NaN/null correctly #14660

Closed
TimothyGu opened this issue Aug 7, 2017 · 6 comments
Closed

VM module doesn't handle undefined/NaN/null correctly #14660

TimothyGu opened this issue Aug 7, 2017 · 6 comments
Labels
v8 engine Issues and PRs related to the V8 dependency. vm Issues and PRs related to the vm subsystem.

Comments

@TimothyGu
Copy link
Member

  • Version: v8.2.1
  • Platform: Linux 4.9.0-3-amd64 SMP Debian 4.9.30-2+deb9u2 (2017-06-26) x86_64 GNU/Linux
  • Subsystem: vm
> vm.runInNewContext('undefined', { undefined: 123 })
123
> vm.runInNewContext('this.undefined', { undefined: 123 })
123
> // Compare:
> this.undefined = 123
> undefined
undefined
> this.undefined
undefined

undefined (and NaN and Infinity) is defined as unwritable, unconfigurable, unenumerable properties on the global object, and thus IMO should not be overridable through the VM module.

It is unclear if #13265 fixes this, probably not though as this issue concerns getting the value.

@TimothyGu TimothyGu added the vm Issues and PRs related to the vm subsystem. label Aug 7, 2017
@TimothyGu
Copy link
Member Author

(I discovered it when debugging jsdom/jsdom#1932, which turns out to be primarily a bug on jsdom's side. I'm okay with closing this, but still keeping this open to see what anyone else thinks.)

@bnoordhuis
Copy link
Member

I suspect this is a V8 bug that has since been fixed because I can't reproduce with today's master; I can with the v8.x branch.

@mscdex mscdex added the v8 engine Issues and PRs related to the V8 dependency. label Aug 7, 2017
@TimothyGu
Copy link
Member Author

@bnoordhuis In master the first case returns undefined but the second still evaluates to 123

@bnoordhuis
Copy link
Member

Sorry yes, I was referring to the first case. The second one is a bug in the vm module, not V8.

(Or bug... it makes perfect sense in the proxy-to-another-object model, but it's different from a native context.)

@maclover7
Copy link
Contributor

As of f1d6b04, it looks like this behavior has changed a little bit:

> vm.runInNewContext('undefined', { undefined: 123 })
undefined
> vm.runInNewContext('this.undefined', { undefined: 123 })
123
> this.undefined = 123
123
> this.undefined
undefined

@TimothyGu
Copy link
Member Author

I'm going to call this one of the things that one has to take care of when trying to use the vm module "correctly". It's not within our power to fix as it originates in V8, and it's pretty difficult to fix even on the V8 end.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v8 engine Issues and PRs related to the V8 dependency. vm Issues and PRs related to the vm subsystem.
Projects
None yet
Development

No branches or pull requests

4 participants