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

Object.defineProperty on the global object does not handle 'writable' properly in vm context #10223

Closed
i8-pi opened this issue Dec 11, 2016 · 4 comments
Labels
v8 engine Issues and PRs related to the V8 dependency. vm Issues and PRs related to the vm subsystem.

Comments

@i8-pi
Copy link
Contributor

i8-pi commented Dec 11, 2016

  • Version: v6.9.2
  • Platform: Linux 4.8.12-3-ARCH SMP PREEMPT x86_64 GNU/Linux
  • Subsystem: vm

When running in a vm context, calling Object.defineProperty on the global object does not handle writable properly

'use strict';
const vm = require('vm');
const g = {console: console};
vm.createContext(g);
vm.runInContext(`
'use strict';
Object.defineProperty(this, 'dummy', {value: 'zxcv', 'writable': false});
console.log(Object.getOwnPropertyDescriptor(this, 'dummy'));
try {
    dummy = 'osef';
} catch(e) {
    console.log(e);
}
console.log(dummy);
`, g);

Output

{ value: 'zxcv',
  writable: true,
  enumerable: true,
  configurable: true }
TypeError: Cannot assign to read only property 'dummy' of object '#<Object>'
    at evalmachine.<anonymous>:7:11
    at ContextifyScript.Script.runInContext (vm.js:35:29)
    at Object.exports.runInContext (vm.js:67:17)
    at Object.<anonymous> (/home/i8-pi/src/zxcv.js:5:4)
    at Module._compile (module.js:570:32)
    at Object.Module._extensions..js (module.js:579:10)
    at Module.load (module.js:487:32)
    at tryModuleLoad (module.js:446:12)
    at Function.Module._load (module.js:438:3)
    at Module.runMain (module.js:604:10)
osef

Expected behaviour is that the property descriptor for dummy to have writable, enumerable and configurable == false. Omitting writable from the defineProperty call behaves in the same way as specifying it to be false. Also, assigning to dummy then actually changes the property value, even though the assignment throws an exception

@addaleax addaleax added v8 engine Issues and PRs related to the V8 dependency. vm Issues and PRs related to the vm subsystem. labels Dec 11, 2016
@addaleax
Copy link
Member

/cc @nodejs/v8

@bnoordhuis
Copy link
Member

#10227

@fhinkel
Copy link
Member

fhinkel commented Dec 11, 2016

FYI, calling defineProperty on a sandbox doesn't work correctly with accessor descriptors. But we should be able to handle value descriptors. Once V8 5.5 lands, we can use the new API which allows us to get rid of CopyProperties() which is currently causing several issues. (Fixing the vm module is an Outreachy project, maybe you can hold of fixing since the mentee is working on this.)

If we're fixing readonly properties, should we also add checks for e.g., deleting non configurable properties?

Fishrock123 pushed a commit that referenced this issue Dec 13, 2016
Check that the property doesn't have the read-only flag set before
overwriting it.

Fixes: #10223
PR-URL: #10227
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
addaleax added a commit to addaleax/node that referenced this issue Jan 20, 2017
Add the regression test script presented in
nodejs#10806 to `test/parallel` and
re-add the original regression test for
nodejs#10223 in `test/known_issues`.
addaleax added a commit that referenced this issue Jan 25, 2017
Add the regression test script presented in
#10806 to `test/parallel` and
re-add the original regression test for
#10223 in `test/known_issues`.

PR-URL: #10920
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@bnoordhuis
Copy link
Member

Reopening now that 524f693 has been reverted in #10920.

@bnoordhuis bnoordhuis reopened this Jan 25, 2017
AnnaMag added a commit to AnnaMag/node that referenced this issue Jan 26, 2017
targos pushed a commit that referenced this issue Jan 28, 2017
Add the regression test script presented in
#10806 to `test/parallel` and
re-add the original regression test for
#10223 in `test/known_issues`.

PR-URL: #10920
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
fhinkel pushed a commit that referenced this issue Jan 30, 2017
PR-URL: #11024
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
italoacasas pushed a commit to italoacasas/node that referenced this issue Jan 30, 2017
Add the regression test script presented in
nodejs#10806 to `test/parallel` and
re-add the original regression test for
nodejs#10223 in `test/known_issues`.

PR-URL: nodejs#10920
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
italoacasas pushed a commit to italoacasas/node that referenced this issue Jan 30, 2017
Add the regression test script presented in
nodejs#10806 to `test/parallel` and
re-add the original regression test for
nodejs#10223 in `test/known_issues`.

PR-URL: nodejs#10920
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
evanlucas pushed a commit that referenced this issue Jan 31, 2017
PR-URL: #11024
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
fhinkel added a commit to fhinkel/node that referenced this issue Feb 3, 2017
Check that the property doesn't have the read-only flag set before
overwriting it.

This is Ben Noordhuis previous commit, but keeping
is_contextual_store. is_contextual_store describes whether this.foo = 42 or
foo = 42 was called. The second is contextual and will fail
in strict mode if foo is used without declaration. Therefore only do an
early return if it is a contextual store. In particular,
don't do an early return for Object.defineProperty(this, ...).

Fixes: nodejs#10223
Refs: nodejs#10227
@fhinkel fhinkel closed this as completed in e116cbe Feb 4, 2017
italoacasas pushed a commit to italoacasas/node that referenced this issue Feb 5, 2017
Check that the property doesn't have the read-only flag set before
overwriting it.

This is Ben Noordhuis previous commit, but keeping
is_contextual_store. is_contextual_store describes whether
this.foo = 42 or foo = 42 was called. The second is contextual
and will fail in strict mode if foo is used without
declaration. Therefore only do an early return if it is
a contextual store. In particular, don't do an early
return for Object.defineProperty(this, ...).

Fixes: nodejs#10223
Refs: nodejs#10227
PR-URL: nodejs#11109
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this issue Feb 14, 2017
Check that the property doesn't have the read-only flag set before
overwriting it.

This is Ben Noordhuis previous commit, but keeping
is_contextual_store. is_contextual_store describes whether
this.foo = 42 or foo = 42 was called. The second is contextual
and will fail in strict mode if foo is used without
declaration. Therefore only do an early return if it is
a contextual store. In particular, don't do an early
return for Object.defineProperty(this, ...).

Fixes: nodejs#10223
Refs: nodejs#10227
PR-URL: nodejs#11109
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
krydos pushed a commit to krydos/node that referenced this issue Feb 25, 2017
Check that the property doesn't have the read-only flag set before
overwriting it.

This is Ben Noordhuis previous commit, but keeping
is_contextual_store. is_contextual_store describes whether
this.foo = 42 or foo = 42 was called. The second is contextual
and will fail in strict mode if foo is used without
declaration. Therefore only do an early return if it is
a contextual store. In particular, don't do an early
return for Object.defineProperty(this, ...).

Fixes: nodejs#10223
Refs: nodejs#10227
PR-URL: nodejs#11109
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
jasnell pushed a commit that referenced this issue Mar 8, 2017
PR-URL: #11024
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
jasnell pushed a commit that referenced this issue Mar 8, 2017
PR-URL: #11024
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit that referenced this issue Mar 9, 2017
PR-URL: #11024
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit that referenced this issue Mar 9, 2017
PR-URL: #11024
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
fhinkel added a commit to fhinkel/node that referenced this issue Oct 23, 2017
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
nodejs#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
nodejs#13265

This PR requires a backport of
https://chromium.googlesource.com/v8/v8/+/37a3a15c3e52e2146e45f41c427f24414e4d7f6f

Refs: nodejs#6283
Refs: nodejs#15114
Refs: nodejs#13265

Fixes: nodejs#2734
Fixes: nodejs#10223
Fixes: nodejs#11803
Fixes: nodejs#11902
fhinkel added a commit to fhinkel/node that referenced this issue Oct 23, 2017
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
nodejs#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
nodejs#13265

This PR requires a backport of
https://chromium.googlesource.com/v8/v8/+/37a3a15c3e52e2146e45f41c427f24414e4d7f6f

PR-URL: nodejs#16293
Fixes: nodejs#2734
Fixes: nodejs#10223
Fixes: nodejs#11803
Fixes: nodejs#11902
Ref: nodejs#6283
Ref: nodejs#15114
Ref: nodejs#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>
addaleax pushed a commit to ayojs/ayo that referenced this issue Oct 26, 2017
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
nodejs/node#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
nodejs/node#13265

This PR requires a backport of
https://chromium.googlesource.com/v8/v8/+/37a3a15c3e52e2146e45f41c427f24414e4d7f6f

PR-URL: nodejs/node#16293
Fixes: nodejs/node#2734
Fixes: nodejs/node#10223
Fixes: nodejs/node#11803
Fixes: nodejs/node#11902
Ref: nodejs/node#6283
Ref: nodejs/node#15114
Ref: nodejs/node#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>
addaleax pushed a commit to ayojs/ayo that referenced this issue Dec 7, 2017
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
nodejs/node#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
nodejs/node#13265

This PR requires a backport of
https://chromium.googlesource.com/v8/v8/+/37a3a15c3e52e2146e45f41c427f24414e4d7f6f

PR-URL: nodejs/node#16293
Fixes: nodejs/node#2734
Fixes: nodejs/node#10223
Fixes: nodejs/node#11803
Fixes: nodejs/node#11902
Ref: nodejs/node#6283
Ref: nodejs/node#15114
Ref: nodejs/node#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>
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

Successfully merging a pull request may close this issue.

4 participants