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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

fhinkel
Copy link
Member

@fhinkel fhinkel commented Feb 1, 2017

Another try for #10223.

This is @bnoordhuis reverted commit, without removing is_contextual_store.

Now defineProperty(this, ..., {value: }) is intercepted and copied by the SetterCallback and global assignment succeeds the first time. The callback does not overwrite read-only values.

The first assert in the test is different from the reverted commit, because defineProperty(..., {value: }) is intercepted and copied by the SetterCallback.

https://github.com/nodejs/node/blob/master/test/known_issues/test-vm-data-property-writable.js is still a known issue, because the property descriptor is taken from the sandbox not the vm context. The test is not testing that read-only properties are not overwritten, but that defineProperty works.

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. vm Issues and PRs related to the vm subsystem. labels Feb 1, 2017
@fhinkel fhinkel force-pushed the vm_read_only branch 2 times, most recently from 7546f71 to aa952c4 Compare February 2, 2017 08:44
thefourtheye
thefourtheye reviewed Feb 2, 2017
View reviewed changes
test/parallel/test-vm-context.js

// https://github.com/nodejs/node/issues/10223
ctx = vm.createContext();
vm.runInContext('Object.defineProperty(this, "x", { value: 42 })', ctx);
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: These tests can be logically grouped. Can you group them in blocks?

Copy link
Member Author

Choose a reason for hiding this comment

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

Which groups did you have in mind?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe something like this?

{
  // https://github.com/nodejs/node/issues/10223
  const ctx = vm.createContext();

  {
    vm.runInContext('Object.defineProperty(this, "x", { value: 42 })', ctx);
    assert.strictEqual(ctx.x, 42);
    assert.strictEqual(vm.runInContext('x', ctx), 42);
  }

  {
    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'/);
    assert.strictEqual(vm.runInContext('x', ctx), 42);
  }

}

Copy link
Member Author

Choose a reason for hiding this comment

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

Blocks in JS Code look weird to me. I added some extra blank lines. OK like this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool :-) This looks clean to my eyes now.

@fhinkel
src: don't overwrite non-writable vm globals
c081b5b 
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
thefourtheye
thefourtheye approved these changes Feb 3, 2017
View reviewed changes
test/parallel/test-vm-context.js

// https://github.com/nodejs/node/issues/10223
ctx = vm.createContext();
vm.runInContext('Object.defineProperty(this, "x", { value: 42 })', ctx);
Copy link
Contributor

Choose a reason for hiding this comment

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

Cool :-) This looks clean to my eyes now.

@fhinkel
Copy link
Member Author

fhinkel commented Feb 4, 2017

Thanks for the reviews. Landed in e116cbe.

@fhinkel fhinkel closed this Feb 4, 2017
fhinkel added a commit that referenced this pull request Feb 4, 2017
@fhinkel
src: don't overwrite non-writable vm globals
e116cbe 
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 <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 pull request Feb 5, 2017
@fhinkel @italoacasas
src: don't overwrite non-writable vm globals
315d59f 
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 italoacasas mentioned this pull request Feb 5, 2017
Merged
italoacasas pushed a commit to italoacasas/node that referenced this pull request Feb 14, 2017
@fhinkel @italoacasas
src: don't overwrite non-writable vm globals
d8a5e1c 
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 pull request Feb 25, 2017
@fhinkel @krydos
src: don't overwrite non-writable vm globals
86da421 
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
Copy link
Member

jasnell commented Mar 7, 2017 •
edited
Loading

This would need a backport PR in order to land on v4 or v6 (if it should land on those at all)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thefourtheye thefourtheye thefourtheye approved these changes

@jasnell jasnell jasnell approved these changes

@bnoordhuis bnoordhuis bnoordhuis approved these changes

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@fhinkel @jasnell @bnoordhuis @thefourtheye @nodejs-github-bot