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

Question about timeout option #475

Closed
JeanSamGirard opened this issue May 31, 2024 · 5 comments
Closed

Question about timeout option #475

JeanSamGirard opened this issue May 31, 2024 · 5 comments

Comments

@JeanSamGirard
Copy link

JeanSamGirard commented May 31, 2024

I'd like to understand better how the timeout option works in isolated-vm, especially in asynchronous scenarios.

I have something a little like this:

import ivm from 'isolated-vm';

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

// Set a reference to a function outside of the isolate
context.global.set(
    'externalFunctionRef',
    new ivm.Reference(async (params) => {
        // Asynchronous code here (database query, http request, etc...)
    })
);

// Make a function in the isolate to abstract the call to the external function
context.evalIgnored(`const externalFunction = async (params) => {
    return await externalFunctionRef.apply(null, [params], {
        arguments: { copy: true },
        result: { copy: true, promise: true },
    });
};`);

// Later on...

// Run the actual code inside the isolate

const code =
    'const myfunc = async ()=>{while(true){await externalFunction("yo")}}; myfunc();';

try {
    console.log(
        'RESULT: ' +
            (await context.eval(code, {
                timeout: 5000,
                promise: true,
                copy: true,
            }))
    );
} catch (e) {
    console.error(e);
}

This seems to never timeout?

Is it because the time spent waiting for the external promise to resolve isn't counted towards the timeout? (aka timeout is CPU time only)

I already have an idea for a workaround for my use case, but if there's a better way I'd love to hear it.


My workaround:
When calling externalFunction before going out of the isolate compare Date.now to the Date.now at the start of execution, if too much time has passed, throw. (this is not a real timeout, but would be sufficient for my use case)

@laverdet
Copy link
Owner

laverdet commented Jun 4, 2024

The timeout only applies to the initial synchronous execution. In this case your anonymous function is invoked, it invokes externalFunction which immediately returns a promise. Then myfunc() returns a promise and evaluation has finished.

The promise-related convenience utilities present a challenge when invoked against untrusted code which needs to timeout.

@JeanSamGirard
Copy link
Author

This makes so much sense, can't believe I didn't notice that...

Also makes me realize that my workaround doesn't really work :

import ivm from 'isolated-vm';

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

// Set a reference to a function outside of the isolate
context.global.set(
    'externalFunctionRef',
    new ivm.Reference(async (params) => {
        // Asynchronous code here (database query, http request, etc...)
    })
);

// Make a function in the isolate to abstract the call to the external function
context.evalIgnored(`
let started;
const externalFunction = async (params) => {
    if(Date.now() > started + 5000) throw "Timeout reject the promise";
    return await externalFunctionRef.apply(null, [params], {
        arguments: { copy: true },
        result: { copy: true, promise: true },
    });
};`);

// Later on...

// Run the actual code inside the isolate

const code = `started = Date.now(); // I inject this before the "untrusted code" (in my actual use case, the code is transpiled so I have control on the variable names used in the user code, they wouldn't be able to alter this value)

    const myfunc = async ()=>{
        let firstTime = true;
        while(true){
         if(firstTime) await externalFunction(); // myfunc will return a promise here
         firstTime = false; // The promise never resolves and can't be rejected by externalFunction because externalFunction doesn't get called anymore...
    }}; myfunc();`;

try {
    console.log(
        'RESULT: ' +
            (await context.eval(code, {
                timeout: 5000,
                promise: true,
                copy: true,
            }))
    );
} catch (e) {
    console.error(e);
}

I guess I could add the same timeout check that I do in externalFunction at the beginning of while and for loops during transpilation.

Would that cover everything to prevent infinite code execution ?

They could still write recursive code but they'll hit the max call stack size and stop eventually.

@JeanSamGirard
Copy link
Author

Would calling isolate.dispose() force the pending promise to reject ?

@JeanSamGirard JeanSamGirard reopened this Jun 4, 2024
@laverdet
Copy link
Owner

laverdet commented Jun 4, 2024

If you want to have a timeout which is cumulative between multiple entrances of the isolate, and also includes asynchronous wall time, then you'll need to handle that bookkeeping outside of isolated-vm.

The problem is not simple. For example you need to make sure that await Promise.all([ externalFunction(), externalFunction() ]) doesn't "charge" the user for 2x wall time.

My advice here is to pretend that { promise: true } does not exist and instead pass around your own resolver functions.

@JeanSamGirard
Copy link
Author

Thanks for the help.

I think for my use case I'll go with this for now:

import ivm from 'isolated-vm';

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

// Set a reference to a function outside of the isolate
context.global.set(
    'externalFunctionRef',
    new ivm.Reference(async (params) => {
        // Asynchronous code here (database query, http request, etc...)
    })
);

// Make a function in the isolate to abstract the call to the external function
context.evalIgnored(`
let started;
const externalFunction = async (params) => {
    if(Date.now() > started + 5000) throw "Timeout reject the promise";
    return await externalFunctionRef.apply(null, [params], {
        arguments: { copy: true },
        result: { copy: true, promise: true },
    });
};`);

// Later on...

// Run the actual code inside the isolate

const code = `started = Date.now(); // I inject this before the "untrusted code" (in my actual use case, the code is transpiled so I have control on the variable names used in the user code, they wouldn't be able to alter this value)

    const myfunc = async ()=>{
        let firstTime = true;
        while(true){
         if(firstTime) await externalFunction(); // myfunc will return a promise here
         firstTime = false; // The promise never resolves and can't be rejected by externalFunction because externalFunction doesn't get called anymore...
    }}; myfunc();`;

let promiseRef;

try {
    promiseRef = context.eval(code, {
        timeout: 5000,
        promise: true,
        copy: true,
    });

    console.log(
        'RESULT: ' +
            (await Promise.race([
                promiseRef,
                new Promise((resolve, reject) => {
                    setTimeout(() => reject('TIMED OUT'), 5000);
                }),
            ]))
    );
} catch (e) {
    console.error(e);
} finally {
    if (!isolate.isDisposed) isolate.dispose();
    setTimeout(() => console.log(promiseRef), 1000);
}

I don't mind the infinite loop running for a little while longer in the isolate as long as it doesn't stop my main process awaiting it and the Promise gets rejected when the isolate is disposed of.

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