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

Experimental Stackguard #1566

Merged
merged 15 commits into from
Jul 2, 2023
40 changes: 38 additions & 2 deletions Jint.Tests/Runtime/EngineLimitTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ public void ShouldAllowReasonableCallStackDepth()

// generate call tree
var sb = new StringBuilder();
sb.AppendLine("var x = 10;");
sb.AppendLine("var x = 1;");
sb.AppendLine();
for (var i = 1; i <= FunctionNestingCount; ++i)
{
Expand All @@ -32,7 +32,7 @@ public void ShouldAllowReasonableCallStackDepth()
if (i != FunctionNestingCount)
{
// just to create a bit more nesting add some constructs
sb.Append("return x++ > 1 ? func").Append(i + 1).Append("(func").Append(i).AppendLine("Param): undefined;");
sb.Append("return x++ >= 1 ? func").Append(i + 1).Append("(func").Append(i).AppendLine("Param): undefined;");
}
else
{
Expand All @@ -47,6 +47,42 @@ public void ShouldAllowReasonableCallStackDepth()
var engine = new Engine();
engine.Execute(sb.ToString());
Assert.Equal(123, engine.Evaluate("func1(123);").AsNumber());
Assert.Equal(FunctionNestingCount, engine.Evaluate("x").AsNumber());
}

[Fact]
public void ShouldNotStackoverflowWhenStackGuardEnable()
{
// Can be more than 10000, It does not hit stackoverflow anymore.
const int FunctionNestingCount = 10000;

// generate call tree
var sb = new StringBuilder();
sb.AppendLine("var x = 1;");
sb.AppendLine();
for (var i = 1; i <= FunctionNestingCount; ++i)
{
sb.Append("function func").Append(i).Append("(func").Append(i).AppendLine("Param) {");
sb.Append(" ");
if (i != FunctionNestingCount)
{
// just to create a bit more nesting add some constructs
sb.Append("return x++ >= 1 ? func").Append(i + 1).Append("(func").Append(i).AppendLine("Param): undefined;");
}
else
{
// use known CLR function to add breakpoint
sb.Append("return Math.max(0, func").Append(i).AppendLine("Param);");
}

sb.AppendLine("}");
sb.AppendLine();
}

var engine = new Engine(option => option.Constraints.MaxExecutionStackCount = FunctionNestingCount);
engine.Execute(sb.ToString());
Assert.Equal(123, engine.Evaluate("func1(123);").AsNumber());
Assert.Equal(FunctionNestingCount, engine.Evaluate("x").AsNumber());
}
}

Expand Down
25 changes: 25 additions & 0 deletions Jint.Tests/Runtime/EngineTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1883,6 +1883,31 @@ public void DateShouldParseToString()
");
}


[Fact]
public void ShouldThrowErrorWhenMaxExecutionStackCountLimitExceeded()
{
new Engine(options => options.Constraints.MaxExecutionStackCount = 1000)
.SetValue("assert", new Action<bool>(Assert.True))
.Evaluate(@"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe no console write lines, they just make test output bad - maybe just return needed data from Evaluate

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

log removed.
I used it for testing locally and did not expect to effect test output :)

var count = 0;
function recurse() {
count++;
recurse();
return null; // ensure no tail recursion
}
try {
count = 0;
recurse();
assert(false);
} catch(err) {
assert(count >= 1000);
}
");

}


[Fact]
public void LocaleNumberShouldUseLocalCulture()
{
Expand Down
2 changes: 2 additions & 0 deletions Jint/Engine.cs
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ public sealed partial class Engine : IDisposable
internal ConditionalWeakTable<object, ObjectInstance>? _objectWrapperCache;

internal readonly JintCallStack CallStack;
internal readonly StackGuard _stackGuard;

// needed in initial engine setup, for example CLR function construction
internal Intrinsics _originalIntrinsics = null!;
Expand Down Expand Up @@ -129,6 +130,7 @@ public Engine(Action<Engine, Options> options)
Options.Apply(this);

CallStack = new JintCallStack(Options.Constraints.MaxRecursionDepth >= 0);
_stackGuard = new StackGuard(this);
}

private void Reset()
Expand Down
6 changes: 6 additions & 0 deletions Jint/Options.cs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
using Jint.Runtime.Debugger;
using Jint.Runtime.Descriptors;
using Jint.Runtime.Modules;
using Jint.Runtime.CallStack;

namespace Jint
{
Expand Down Expand Up @@ -385,6 +386,11 @@ public class ConstraintOptions
/// </summary>
public int MaxRecursionDepth { get; set; } = -1;

/// <summary>
/// Maximum recursion stack count, defaults to -1 (as-is dotnet stacktrace).
/// </summary>
public int MaxExecutionStackCount { get; set; } = StackGuard.Disabled;
Copy link
Collaborator

@lahma lahma Jul 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the old comment had valuable info 14000 which comes from Chrome and V8 based engines (ClearScript) that can handle 13955. So might be good to indicate that option will allow more nested calls with slight performance / stack trace readability drawback. (after hitting the engine's own limit), The old info gave reasonable limit for expectations without people resorting to int.MaxValue.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated it as remarks. If you have another suggestion, please share it with me. It will be a pleasure for me to improve it.


/// <summary>
/// Maximum time a Regex is allowed to run, defaults to 10 seconds.
/// </summary>
Expand Down
87 changes: 87 additions & 0 deletions Jint/Runtime/CallStack/StackGuard.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

// https://github.com/dotnet/runtime/blob/a0964f9e3793cb36cc01d66c14a61e89ada5e7da/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/StackGuard.cs

using System.Runtime.CompilerServices;
using System.Threading;

namespace Jint.Runtime.CallStack;

internal sealed class StackGuard
{
public const int Disabled = -1;

private readonly Engine _engine;

public StackGuard(Engine engine)
{
_engine = engine;
}

public bool TryEnterOnCurrentStack()
{
if (_engine.Options.Constraints.MaxExecutionStackCount == Disabled)
{
return true;
}

#if NETFRAMEWORK || NETSTANDARD2_0
try
{
RuntimeHelpers.EnsureSufficientExecutionStack();
return true;
}
catch (InsufficientExecutionStackException)
{
}
#else
if (RuntimeHelpers.TryEnsureSufficientExecutionStack())
{
return true;
}
#endif

if (_engine.CallStack.Count > _engine.Options.Constraints.MaxExecutionStackCount)
{
ExceptionHelper.ThrowRangeError(_engine.Realm, "Maximum call stack size exceeded");
}

return false;
}

public TR RunOnEmptyStack<T1, TR>(Func<T1, TR> action, T1 arg1)
{
#if NETFRAMEWORK || NETSTANDARD2_0
return RunOnEmptyStackCore(static s =>
{
var t = (Tuple<Func<T1, TR>, T1>) s;
return t.Item1(t.Item2);
}, Tuple.Create(action, arg1));
#else
// Prefer ValueTuple when available to reduce dependencies on Tuple
return RunOnEmptyStackCore(static s =>
{
var t = ((Func<T1, TR>, T1)) s;
return t.Item1(t.Item2);
}, (action, arg1));
#endif

}

private R RunOnEmptyStackCore<R>(Func<object, R> action, object state)
{
// Using default scheduler rather than picking up the current scheduler.
Task<R> task = Task.Factory.StartNew((Func<object?, R>) action, state, CancellationToken.None, TaskCreationOptions.DenyChildAttach, TaskScheduler.Default);

// Avoid AsyncWaitHandle lazy allocation of ManualResetEvent in the rare case we finish quickly.
if (!task.IsCompleted)
{
// Task.Wait has the potential of inlining the task's execution on the current thread; avoid this.
((IAsyncResult) task).AsyncWaitHandle.WaitOne();
}

// Using awaiter here to propagate original exception
return task.GetAwaiter().GetResult();
}
}
5 changes: 5 additions & 0 deletions Jint/Runtime/Interpreter/Expressions/JintCallExpression.cs
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,11 @@ static bool CanSpread(Node? e)

protected override object EvaluateInternal(EvaluationContext context)
{
if (!context.Engine._stackGuard.TryEnterOnCurrentStack())
{
return context.Engine._stackGuard.RunOnEmptyStack(EvaluateInternal, context);
}

if (_calleeExpression._expression.Type == Nodes.Super)
{
return SuperCall(context);
Expand Down