vm: allow proxy callbacks to throw

Fixes: #33806 PR-URL: #33808 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gerhard Stöbich <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
nodejs · Jun 11, 2020 · 4faec56 · 4faec56
1 parent db3d6b3
commit 4faec56
Show file tree

Hide file tree

Showing 2 changed files with 26 additions and 6 deletions.
diff --git a/src/node_contextify.cc b/src/node_contextify.cc
@@ -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
@@ -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);
+    }
   }
 }
 
@@ -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()) {

diff --git a/test/parallel/test-vm-context-property-forwarding.js b/test/parallel/test-vm-context-property-forwarding.js
@@ -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',
+  });
+}