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

src: don't overwrite non-writable vm globals #10227

Merged
merged 1 commit into from
Dec 13, 2016

Conversation

bnoordhuis
Copy link
Member

@bnoordhuis bnoordhuis commented Dec 11, 2016

@fhinkel Can you take a look? I couldn't figure out what is_contextual_store is needed for. Tests pass without it.

CI: https://ci.nodejs.org/job/node-test-pull-request/5357/

@nodejs-github-bot nodejs-github-bot added dont-land-on-v7.x c++ Issues and PRs that require attention from people who are familiar with C++. labels Dec 11, 2016
@mscdex mscdex added the vm Issues and PRs related to the vm subsystem. label Dec 11, 2016
@fhinkel
Copy link
Member

fhinkel commented Dec 11, 2016

LGTM.

I was trying to mirror when V8 does not set the property. https://github.com/v8/v8/blob/master/src/objects.cc#L4812

I think we don't need is_contextual_store, because in the VM context we always work on the global object.

vm.runInContext('x = 0', ctx); // Does not throw but x...
assert.strictEqual(vm.runInContext('x', ctx), 42); // ...should be unaltered.
assert.throws(() => vm.runInContext('"use strict"; x = 0', ctx),
/Cannot assign to read only property 'x'/);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure checking the error string is a great idea here as it might change.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, but we can update the test when it does. I opted to be specific because it needs to throw this particular exception. Any other exception probably means something is broken.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough

Check that the property doesn't have the read-only flag set before
overwriting it.

Fixes: nodejs#10223
PR-URL: nodejs#10227
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
@bnoordhuis
Copy link
Member Author

Landed in 524f693, thanks for reviewing.

@bnoordhuis bnoordhuis merged commit 524f693 into nodejs:master Dec 13, 2016
Fishrock123 pushed a commit that referenced this pull request 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 <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
@italoacasas italoacasas mentioned this pull request Dec 15, 2016
@MylesBorins
Copy link
Contributor

This appears to have broken console logging with jsdom

#10492

@cpojer
Copy link

cpojer commented Dec 29, 2016

See jestjs/jest#2457 (comment) for @thealphanerd's repro. This also applies to globals like setTimeout and other timer related features. The only workaround I can find so far is to set the fields in a non-strict module, which seems quite hacky. Can we at least make Object.defineProperty work for these.

addaleax added a commit to addaleax/node that referenced this pull request Jan 20, 2017
@MylesBorins
Copy link
Contributor

due to the revert I am opting to label this as do not land.

addaleax added a commit that referenced this pull request Jan 25, 2017
This reverts commit 524f693.

Fixes: #10806
Fixes: #10492
Ref: #10227
PR-URL: #10920
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
targos pushed a commit that referenced this pull request Jan 28, 2017
This reverts commit 524f693.

Fixes: #10806
Fixes: #10492
Ref: #10227
PR-URL: #10920
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 30, 2017
This reverts commit 524f693.

Fixes: nodejs#10806
Fixes: nodejs#10492
Ref: nodejs#10227
PR-URL: nodejs#10920
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 30, 2017
This reverts commit 524f693.

Fixes: nodejs#10806
Fixes: nodejs#10492
Ref: nodejs#10227
PR-URL: nodejs#10920
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
@fhinkel
Copy link
Member

fhinkel commented Feb 1, 2017

@bnoordhuis, I know this is long closed and reverted, but I figured out what is_contextual_store is. Thought I leave a comment. It describes whether this.foo = 42 or foo = 42 was called. The second is contextual and should fail in strict mode if foo is used without declaration.

This test would rely on it, if it weren't for the CopyProperties() hack. The setter callback intercepts this.z = 1, doesn't copy z over if there is no check for is_contextual_store, and instead CopyProperties() copies z to the sandbox. So the test passes with or without is_contextual_store.

fhinkel added a commit to fhinkel/node that referenced this pull request 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 added a commit that referenced this pull request Feb 4, 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: #10223
Refs: #10227
PR-URL: #11109
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
italoacasas pushed a commit to italoacasas/node that referenced this pull request 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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
italoacasas pushed a commit to italoacasas/node that referenced this pull request 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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
krydos pushed a commit to krydos/node that referenced this pull request 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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. vm Issues and PRs related to the vm subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants