Find a way to apply a execution timeout even for code in protected regions

[kpreisser]

Hi,

some time ago I opened #42 to write a wiki page that shows how to apply a timeout to script execution.
Just now I was looking at the emit for the TryCatchFinallyStatement and realized that it emits JS catch and finally blocks as .NET catch and finally blocks. This means the timeout (which is done by Thread.Abort()) can easily be bypassed:

try {} finally {
    while (true);
}

Drat!

Also, when the script calls an API in the catch or finally block which uses ScriptTimeoutHelper.EnterCriticalSection, it will lead to a deadlock as that method waits for a ThreadAbortException, which will not happen since the code is currently in a protected region.

Further, this means that JS code in a finally block is executed even when non-JavaScriptExceptions, e.g. a ThreadAbortException or a StackOverflowException from the recursion depth check, are thrown (whereas in my case I would like to ensure no more script code is executed when throwing a non-JavaScriptException).

Unfortunately, I'm not sure if it is somehow possible to mark generated catch and finally blocks as non-protected region, so that the CLR does not delay thread aborts for these regions. If this is not possible, the only way I can think of which would still be practically, is to generate functions for the contents of a try and catch statement, and catching an exception before running the code for a catch and finally clause.

For example, if the JS is this:

function myFunc() {
    for (var i = 0; i < 20; i++) {
        try {
            console.log("Try!");

            if (i == 10)
                break;
            else if (i == 11)
                continue;
            else if (i == 12)
                return "i: " + i;

        }
        catch (e) {
            console.log("Catch!");
        }
        finally {
            console.log("Finally!");
        }
    }
}

the generated code would look in C# like this:

        private static object JS_myFunc()
        {
            for (var i = 0; i < 20; i++)
            {
                // Try
                ExceptionDispatchInfo caughtException1 = null;
                int state1 = 0;
                object returnValue1 = null;
                try
                {
                    state1 = TryFunction1(ref returnValue1, ref i);
                }
                catch (Exception ex) when (ex is JavaScriptException)
                {
                    caughtException1 = ExceptionDispatchInfo.Capture(ex);
                }
                // Finally
                Console.WriteLine("Finally!");
                if (caughtException1 != null)
                    caughtException1.Throw();
                // Handle return state
                if (state1 == 0)
                    break;
                else if (state1 == 1)
                    continue;
                else if (state1 == 2)
                    return returnValue1;
            }

            return null;
        }
        private static int TryFunction1(ref object returnValue, ref int i)
        {
            Exception caughtException1 = null;
            try
            {
                Console.WriteLine("Try!");

                if (i == 10)
                    return 0;
                else if (i == 11)
                    return 1;
                else if (i == 12)
                {
                    returnValue = "i: " + i;
                    return 2;
                }
            }
            catch (Exception ex) when (ex is JavaScriptException)
            {
                caughtException1 = ex;
            }
            if (caughtException1 != null)
            {
                Console.WriteLine("Catch!");
            }
            return 0;
        }

(although that seems already quite a bit complicated).
Unfortunately, I also don't know much of the internals of CIL, so I'm not sure if I can do this.

Do you have any other idea how this could be solved?
Thanks!

[kpreisser]

A less invasive (and probably more clean) alternative would be to implement a cancellation check directly in the generated code. This would avoid the need to use Thread.Abort(), and therefore also avoid the need to call ScriptTimeoutHelper.EnterCriticalSection()/ExitCriticalSection() in API methods (and this should allow to use a timeout also on .NET Core where Thread.Abort() isn't supported).

For example:

• Create two new fields on the ScriptEngine: volatile bool cancellationRequested and bool cancellationExceptionThrown.
• Create a method on the ScriptEngine:

private void HandleCancellationRequest() {
    CancellationExceptionThrown = true;
    throw new ScriptCancellationException();
}

• In Code generation, at the beginning of a function and at the beginning of loop statements (while, for, ...), insert code like:

if (Volatile.Read(ref engine.cancellationRequested))
    HandleCancellationRequest();

• In finally clauses, insert code like:

if (!engine.cancellationExceptionThrown) {
  // Code
}

Then, another thread can set the cancellationRequested flag to true after the timeout has passed.

This would of course be slower due to the need for checking the flags, so these checks probably shouldn't be compiled into the code unless the user explicitely enables a compiler option.

I think it is sufficient to check only at the beginning of functions and loops, but not after every single statetment (like i++;) because for generating real long-executing code, the user would have to use some sort of loop or recursion. The check for the cancellationExceptionThrown flag in finally clauses would be needed to ensure its code is not executed if a cancellation is requested.

What do you think?
Thanks!

[kpreisser]

Hi @paulbartrum,

I have committed the above feature to my branch implementScriptTimeout3 (eventually I'd like to submit a PR if you agree with the changes).

The user can now set the property ScriptEngine.GenerateCancellationChecks generate checks for the ScriptEngine.CancellationRequested property into loops and at the beginning of user-defined functions. When setting that property to true from another thread while the script is still executing, the checks will throw a ScriptCancelledException to abort the script.

When using a simple loop like

var dbl = 2;
for (var i = 0; i < 30000000; i++) {
    dbl *= 1.0000001;
}

execution time increases by about 14% on my machine when enabling GenerateCancellationChecks, but this is OK for my case (and most loops will have more statements in the body so the relative additional time should be less).

To ensure finally clauses are not executed when a non-JavaScriptException is thrown, I'm using a filter block to set a bool variable to true, which is then checked in finally clauses. This ensures that when a exception is thrown, finally clauses is only executed if a catch block would have been able to catch the exception if it were present (which is only the case with JavaScriptExceptions). I'm also storing the ScriptEngine on the JSException to fix #45.

However, I ran into a problem with the DynamicILGenerator. In order to check if a JavaScriptException can be caught in a catch clause and if code in finally clauses should run, I need to use a exception filter, to emit code like:

catch (JavaScriptException ex) when (ex.ScriptEngine == engine)

For this, I'm using a filter block before the catch block, which requires that BeginCatchBlock has null argument:

generator.BeginFilterBlock();
// Check the exception and load a Int32 on the stack...
generator.BeginCatchBlock(null);
// Handle the exception...

This is working fine with the ReflectionEmitILGenerator. The DynamicILGenerator however throws an ArgumentNullException in this case:

        public override void BeginCatchBlock(Type exceptionType)
        {
            if (exceptionType == null)
                throw new ArgumentNullException("exceptionType");

I cannot simply remove the check because later exceptionType.TypeHandle is used.

Do you have any idea how to solve this, so that BeginCatchBlock can be called with a null type?

Thank you!

[kpreisser]

Hmm, I found out that on a ILGenerator which belongs to a dynamic method, BeginExceptFilterBlock() is not supported (and the ILGenerator has to be used on Mono and on .NET Core), which means I will have to find an other way for this.
Currently I'm using a wrapper try-filter-catch block in the Method to detect when a finally block is executed due to an "uncatchable" exception being thrown. I think I can put the filter block into the UserDefinedFunction and setting a flag on the ScriptEngine.

[paulbartrum]

I've checked in a fix for this issue. I copied most of your code, so thanks for that. I didn't copy GenerateInternalCode or WrapGeneratedDelegate though, 'cause I wasn't sure what that code was doing. Let me know if I missed something.

[kpreisser]

Hi @paulbartrum,
thank you for incorporating the changes!

I see that you have applied the fix to not run JS catch and finally blocks if the exception is uncatchable in JS, by catching Exception and rethrowing it (and setting a flag) if it is not catchable. This is corresponds to the catch (Exception ex) when (FilterException(ex, engine, marker)) exception filter that I was using in WrapGeneratedDelegate.

I think using an exception filter might a bit faster if there are multiple catch blocks through which the exception bubbles, because then the exception will only be thrown one time instead of multiple times using rethrow statments. But unfortunately the ILGenerator doesn't always support exception filter blocks, which is why I wrapped the delegate in another method that run the exception filter, and added GenerateInternalCode that gets a reference to a object in which the exception filter will store the flag.
This which worked because exception filters are run until a catch block is found that will handle the exception, but before finally blocks are run from the current block through the found catch block.
But of course that is not as clean as simply using catch clauses and rethrow the exception there, so I also prefer your approach.

However, I think your change is not yet complete: The skipFinallyClause variable is only generated and set when the JS has a try-catch-finally block. If it only has try-finally, no catch clause is generated and therefore the skipFinallyBlock is not set. I think with your approach, you will need to generate a catch clause even if the JS didn't specify one, and rethrow the exception in this case.

Note: This does not yet solve the original problem of this issue, which is that aborting a thread is delayed until a protected region (catch, finally) is left (this is the issue in the first comment) and therefore the solution of setting a Script Timeout described in the wiki page will not work. This is why I added the ScriptEngine.GenerateCancellationChecks flag which, if set to true, will alter the code generation for loops and at the start of methods to include a check of the cancellationRequested flag and throw a ScriptCancelledException in this case. This allows to cancel script code even if the endless loop is running within a finally block.

Beacuse the flag needs to be checked frequently, it has an impact on performance (which is why the cancellation checks aren't enabled by default), but unfortunately I can't see another way of solving this problem, except than splitting try-finally-blocks in different methods as I described in the first comment, which would be quite an effort.

What do you think? Should I submit a PR to implement the cancellation checks? In this case I could update the wiki page about the script timeout (and then setting a timeout should also work on .NET Core where Thread.Abort() currently isn't supported).

Thanks!

[paulbartrum]

Ah, you're right, I missed the try-finally case. Thanks, I'll look into that.

If I seem a bit hesitant about some of these changes, it's because I'm doubtful it can be made robust. The only truly robust way of safely running user code is in a separate process IMO (this is what the Test Suite Runner project does). By robust I mean that the user code will be stopped immediately 100% of time, with zero impact on the hosting code, even if the user code is "hostile".

I am of course aware that running JS in a separate process has it's own problems. UWP doesn't support background processes, for one. It also means that all comms have to go over pipes or some other RPC protocol, which is slow and somewhat finicky.

Still, I can't help but think we're encouraging bad practices by publishing the ScriptTimeoutHelper class.

Thoughts?

[kpreisser]

Hi @paulbartrum,
thanks for your reply.

I still think there should be a way to cancel script execution. In our case (which I described in #83), it would be extremely difficult and costly to run scripts in another process. For example, in the application there can be multiple scripts which are active (which means some callback of these scripts may be called soon). If we would run the scripts in a separate process, we would need an individual process for each script to maintain each script's state even if one script is cancelled by terminating the process. So if the user created 50 scripts, we would need to start 50 processes, which is very expensive in resource usage.
Further, this would require some serialized communication, so in order to maintain object identity, we would have to use some sort of object IDs. However, in our system we map .NET objects to JS ObjectInstances using a ConditionalWeakTable so that the GC can collect the ObjectInstances if their source object is also collected. When this is done in a separate process, that process wouldn't know when the original object in the main process has been GCed.
Also, this would require that when JS code calls some getters of such wrapper objects (which in turn call the .NET getters), each time the getter is called the JS process needs to send a command to the main process to call the getter, which includes serialization of the actual data to be transmitted.
This is especially a problem since in our application the user can register Script callbacks for .NET events which can be raised often in short intervals, so the code for executing those JS callbacks should be as lightweight as possible (with little overhead).

Therefore, at least in our case I can't see a way to avoid cancelling script execution.

Because Thread.Abort() doesn't work when using code in catch or finallly clauses (and requires to call ScriptTimeoutHelper.EnterCriticalSection() etc.) I agree that this can be a bad practice. However, I think the way with checking a flag (ScriptEngine.CancellationRequested) every time user code is executed should be more clean, though it might not terminate the script immediately but only after the next time the flag is checked, and it slows down performance a bit due to the check.
AFAIK, Jint also applies a timeout by checking some flag (or the elapsed time) often.

Currently, I added the check only at the beginning of loop statements and at the top of generated methods. This might be sufficient if the script code length is limited, but maybe the check should be executed after every statement or expression.

What do you think?
Thanks!

[paulbartrum]

Thanks for the detailed reply. You've convinced me that there are definitely use cases that cannot be solved using multi-process, but I need to mull on the best way to implement this.

[kpreisser]

Hi @paulbartrum,
I created PR #90 to handle the try-finally case.
Thanks!