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.

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

Errors thrown from the Node isolate in an async function before any async call can't be caught in another isolate #417

Open
nickrum opened this issue Oct 26, 2023 · 2 comments

Comments

@nickrum
Copy link

nickrum commented Oct 26, 2023

When creating a Reference to an asynchronous function in the Node isolate, that throws an error before doing any asynchronous operations, and passing it to an ivm-created isolate via a closure, the error can't be caught in that isolate when the referenced function is called.

Here is a reproduction of the issue:

import ivm from 'isolated-vm';
import { setTimeout } from 'node:timers/promises';

const isolate = new ivm.Isolate();
const context = await isolate.createContext();

await context.evalClosure(
  `
    const callHost = () => $0.apply(undefined, [], { result: { promise: true } });
    const log = (message) => $1.applySync(undefined, [message], { arguments: { copy: true } });

    const fn = async () => {
      try {
        await callHost();
      } catch {
        log('error');
      }
    }

    return fn();
  `,
  [
    new ivm.Reference(async () => {
      // await setTimeout();

      throw new Error();
    }),
    new ivm.Reference((message) => {
      console.log(message);
    })
  ],
  { result: { promise: true } }
);

Instead of printing "error", this will trigger an uncaught exception, hanging the Node process indefinitely. When the async keyword is removed from the referenced function or await setTimeout(); is commented in, "error" is printed as expected.

What's interesting is that it is working perfectly fine when you reverse the situation by throwing from the ivm-created isolate and catching in the Node isolate:

import ivm from 'isolated-vm';

const isolate = new ivm.Isolate();
const context = await isolate.createContext();

await context.evalClosure(
  `
    const callHost = (cb) => $0.apply(undefined, [cb], { arguments: { reference: true }, result: { promise: true } });
    const log = (message) => $1.applySync(undefined, [message], { arguments: { copy: true } });

    const fn = async () => {
      await callHost(async () => {
        throw new Error();
      });
    }

    return fn();
  `,
  [
    new ivm.Reference(async (cb) => {
      try {
        await cb.apply(undefined, [], {
          result: { promise: true }
        });
      } catch {
        console.log('error');
      }
    }),
    new ivm.Reference((message) => {
      console.log(message);
    })
  ],
  { result: { promise: true } }
);

In this case, "error" is printed to the console as expected.


Isolated-vm version: 4.6.0
Node version: 18.18.0
OS: Ubuntu 20.04
Compiler: gcc 9.4.0

@BartTactick
Copy link

BartTactick commented Nov 2, 2023

@nickrum We are experiencing the same.
For some reason if you use async and you return a new Promise, which you reject on error and resolve on success, it does play "nicely" (which triggers a catch inside of the isolate):

return new Promise(async (res, rej) => {
            rej("Some error?");
});

However this is very ugle if you ask me.

@nickrum
Copy link
Author

nickrum commented Nov 2, 2023

@BartTactick Very interesting!

If you remove the async keyword, an uncaught exception is triggered (using Promise.reject() should be equivalent to your approach):

new ivm.Reference(() => {
  return Promise.reject();
}),

Wrapping the rejected promise in another resolved promise prints "error" as expected:

new ivm.Reference(() => {
  return new Promise((res) => {
    res(Promise.reject());
  });
}),

This leads me to believe that it's the first promise in the chain returned by the referenced function that, if rejected, triggers this issue. That also makes sense in the context of asynchronous functions as they are more or less wrapped in a resolved promise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants