Skip to content
This repository has been archived by the owner on Aug 31, 2018. It is now read-only.

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: nodejs/node#16487
Fixes: nodejs/node#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 Qard committed Nov 2, 2017
1 parent b364a58 commit 456caf5
Show file tree
Hide file tree
Showing 2 changed files with 21 additions and 7 deletions.
21 changes: 19 additions & 2 deletions src/node_contextify.cc
Original file line number Diff line number Diff line change
Expand Up @@ -346,14 +346,21 @@ class ContextifyContext {
return;

auto attributes = PropertyAttribute::None;
bool is_declared = ctx->global_proxy()
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 @@ -371,10 +378,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
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,8 @@ const vm = require('vm');

const ctx = vm.createContext({ x: 42 });

// The following line wrongly throws an
// error because GlobalPropertySetterCallback()
// does not check if the property exists
// on the sandbox. It should just set x to 1
// instead of throwing an error.
// 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);

assert.strictEqual(ctx.x, 1);

0 comments on commit 456caf5

Please sign in to comment.