Skip to content

Commit

Permalink
src: fix vm module for strict mode
Browse files Browse the repository at this point in the history
This patch fixes the problem with variables that
are declared only on the sandbox but not on the
global proxy.

PR-URL: #16487
Fixes: #12300
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
  • Loading branch information
fhinkel authored and gibfahn committed Oct 31, 2017
1 parent 4ba06d0 commit 0a5a2c4
Show file tree
Hide file tree
Showing 3 changed files with 27 additions and 41 deletions.
24 changes: 20 additions & 4 deletions src/node_contextify.cc
Original file line number Diff line number Diff line change
Expand Up @@ -414,15 +414,21 @@ class ContextifyContext {
return;

auto attributes = PropertyAttribute::None;
bool is_declared =
ctx->global_proxy()->GetRealNamedPropertyAttributes(ctx->context(),
property)
bool is_declared_on_global_proxy = 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)
bool is_declared_on_sandbox = ctx->sandbox()
->GetRealNamedPropertyAttributes(ctx->context(), property)
.To(&attributes);
read_only = read_only ||
(static_cast<int>(attributes) &
static_cast<int>(PropertyAttribute::ReadOnly));

if (read_only)
return;

// true for x = 5
Expand All @@ -440,10 +446,20 @@ class ContextifyContext {
// this.f = function() {}, is_contextual_store = false.
bool is_function = value->IsFunction();

bool is_declared = is_declared_on_global_proxy || is_declared_on_sandbox;
if (!is_declared && args.ShouldThrowOnError() && is_contextual_store &&
!is_function)
return;

if (!is_declared_on_global_proxy && is_declared_on_sandbox &&
args.ShouldThrowOnError() && is_contextual_store && !is_function) {
// The property exists on the sandbox but not on the global
// proxy. Setting it would throw because we are in strict mode.
// Don't attempt to set it by signaling that the call was
// intercepted. Only change the value on the sandbox.
args.GetReturnValue().Set(false);
}

ctx->sandbox()->Set(property, value);
}

Expand Down
17 changes: 0 additions & 17 deletions test/known_issues/test-vm-strict-mode.js

This file was deleted.

27 changes: 7 additions & 20 deletions test/parallel/test-vm-strict-mode.js
Original file line number Diff line number Diff line change
@@ -1,27 +1,14 @@
'use strict';
// https://github.com/nodejs/node/issues/12300

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

// Test strict mode inside a vm script, i.e., using an undefined variable
// throws a ReferenceError. Also check that variables
// that are not successfully set in the vm, must not be set
// on the sandboxed context.

vm.runInContext('w = 1;', ctx);
assert.strictEqual(1, ctx.w);

assert.throws(function() { vm.runInContext('"use strict"; x = 1;', ctx); },
/ReferenceError: x is not defined/);
assert.strictEqual(undefined, ctx.x);

vm.runInContext('"use strict"; var y = 1;', ctx);
assert.strictEqual(1, ctx.y);
const ctx = vm.createContext({ x: 42 });

vm.runInContext('"use strict"; this.z = 1;', ctx);
assert.strictEqual(1, ctx.z);
// This might look as if x has not been declared, but x is defined on the
// sandbox and the assignment should not throw.
vm.runInContext('"use strict"; x = 1', ctx);

// w has been defined
vm.runInContext('"use strict"; w = 2;', ctx);
assert.strictEqual(2, ctx.w);
assert.strictEqual(ctx.x, 1);

0 comments on commit 0a5a2c4

Please sign in to comment.