Skip to content

Commit

Permalink
vm: allow proxy callbacks to throw
Browse files Browse the repository at this point in the history
Fixes: #33806

PR-URL: #33808
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
  • Loading branch information
devsnek authored and codebytere committed Jun 18, 2020
1 parent fdaf0ca commit 879c932
Show file tree
Hide file tree
Showing 2 changed files with 26 additions and 6 deletions.
12 changes: 6 additions & 6 deletions src/node_contextify.cc
Original file line number Diff line number Diff line change
Expand Up @@ -422,7 +422,7 @@ void ContextifyContext::PropertySetterCallback(
args.GetReturnValue().Set(false);
}

ctx->sandbox()->Set(ctx->context(), property, value).Check();
USE(ctx->sandbox()->Set(ctx->context(), property, value));
}

// static
Expand All @@ -440,9 +440,10 @@ void ContextifyContext::PropertyDescriptorCallback(
Local<Object> sandbox = ctx->sandbox();

if (sandbox->HasOwnProperty(context, property).FromMaybe(false)) {
args.GetReturnValue().Set(
sandbox->GetOwnPropertyDescriptor(context, property)
.ToLocalChecked());
Local<Value> desc;
if (sandbox->GetOwnPropertyDescriptor(context, property).ToLocal(&desc)) {
args.GetReturnValue().Set(desc);
}
}
}

Expand Down Expand Up @@ -485,8 +486,7 @@ void ContextifyContext::PropertyDefinerCallback(
desc_for_sandbox->set_configurable(desc.configurable());
}
// Set the property on the sandbox.
sandbox->DefineProperty(context, property, *desc_for_sandbox)
.Check();
USE(sandbox->DefineProperty(context, property, *desc_for_sandbox));
};

if (desc.has_get() || desc.has_set()) {
Expand Down
20 changes: 20 additions & 0 deletions test/parallel/test-vm-context-property-forwarding.js
Original file line number Diff line number Diff line change
Expand Up @@ -43,3 +43,23 @@ assert.deepStrictEqual(pd_actual, pd_expected);
assert.strictEqual(ctx2[1], 5);
delete ctx2[1];
assert.strictEqual(ctx2[1], undefined);

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

Object.defineProperty(ctx, 'prop', {
get() {
return undefined;
},
set(val) {
throw new Error('test error');
},
});

assert.throws(() => {
vm.runInContext('prop = 42', ctx);
}, {
message: 'test error',
});
}

0 comments on commit 879c932

Please sign in to comment.