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

Revert "src: don't overwrite non-writable vm globals" #10920

Closed
wants to merge 2 commits into from
Closed

Revert "src: don't overwrite non-writable vm globals" #10920

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
179ea5e
Revert "src: don't overwrite non-writable vm globals"
addaleax Jan 20, 2017
074d588
test: add regression tests for vm bugs
addaleax Jan 20, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 10 additions & 13 deletions src/node_contextify.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -383,22 +383,19 @@ class ContextifyContext {
if (ctx->context_.IsEmpty())
return;

auto attributes = PropertyAttribute::None;
bool is_declared =
ctx->global_proxy()->GetRealNamedPropertyAttributes(ctx->context(),
property)
.To(&attributes);
bool read_only =
static_cast<int>(attributes) &
static_cast<int>(PropertyAttribute::ReadOnly);

if (is_declared && read_only)
return;
ctx->global_proxy()->HasRealNamedProperty(ctx->context(),
property).FromJust();
bool is_contextual_store = ctx->global_proxy() != args.This();
Copy link
Member

Choose a reason for hiding this comment

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

We came to the conclusion that is_contextual_store is always true and shouldn't be here in the first place. Do you want to remove it from the revert or should I make an extra PR. Extra PR probably keeps history and bisects cleaner.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, an extra PR sounds better – mostly because
a) this PR can be landed now (if anybody reading this has a few minutes left over, please feel free to take that task) and
b) because my understanding of this code is actually kind of limited ;)


if (!is_declared && args.ShouldThrowOnError())
return;
bool set_property_will_throw =
args.ShouldThrowOnError() &&
!is_declared &&
is_contextual_store;

ctx->sandbox()->Set(property, value);
if (!set_property_will_throw) {
ctx->sandbox()->Set(property, value);
}
}


Expand Down
16 changes: 16 additions & 0 deletions test/known_issues/test-vm-global-non-writable-properties.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
'use strict';
// https://github.com/nodejs/node/issues/10223

require('../common');
const assert = require('assert');
const vm = require('vm');

const ctx = vm.createContext();
vm.runInContext('Object.defineProperty(this, "x", { value: 42 })', ctx);
assert.strictEqual(ctx.x, undefined); // Not copied out by cloneProperty().
Copy link
Member

Choose a reason for hiding this comment

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

For future reference: it's not IMO the expected or desired behavior, just the actual behavior.

Copy link
Member Author

Choose a reason for hiding this comment

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

@bnoordhuis Right, I agree. Would you prefer it if I updated the test with strictEqual(ctx.x, 42) here?

Copy link
Member

Choose a reason for hiding this comment

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

No need, it was more of an off-the-cuff remark.

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);
11 changes: 0 additions & 11 deletions test/parallel/test-vm-context.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,14 +75,3 @@ assert.throws(function() {
// https://github.com/nodejs/node/issues/6158
ctx = new Proxy({}, {});
assert.strictEqual(typeof vm.runInNewContext('String', ctx), 'function');

// https://github.com/nodejs/node/issues/10223
ctx = vm.createContext();
vm.runInContext('Object.defineProperty(this, "x", { value: 42 })', ctx);
assert.strictEqual(ctx.x, undefined); // Not copied out by cloneProperty().
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);
15 changes: 15 additions & 0 deletions test/parallel/test-vm-global-assignment.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
'use strict';
// Regression test for https://github.com/nodejs/node/issues/10806

require('../common');
const assert = require('assert');
const vm = require('vm');
const ctx = vm.createContext({ open() { } });
const window = vm.runInContext('this', ctx);
const other = 123;

assert.notStrictEqual(window.open, other);
window.open = other;
assert.strictEqual(window.open, other);
window.open = other;
assert.strictEqual(window.open, other);