From 541df1c49b6556c67d07c92a33bb6564d302ea04 Mon Sep 17 00:00:00 2001 From: tomatosalat0 Date: Fri, 7 Nov 2025 16:57:00 +0100 Subject: [PATCH 1/3] Prevent accidential leakage of arguments between separate calls because of the JsValueArrayPool. --- .../Runtime/ArgumentsCacheBehaviorTests.cs | 116 ++++++++++++++++++ Jint/Native/Function/ScriptFunction.cs | 23 ++++ 2 files changed, 139 insertions(+) create mode 100644 Jint.Tests/Runtime/ArgumentsCacheBehaviorTests.cs diff --git a/Jint.Tests/Runtime/ArgumentsCacheBehaviorTests.cs b/Jint.Tests/Runtime/ArgumentsCacheBehaviorTests.cs new file mode 100644 index 0000000000..7ee3303217 --- /dev/null +++ b/Jint.Tests/Runtime/ArgumentsCacheBehaviorTests.cs @@ -0,0 +1,116 @@ +using Jint.Native; + +namespace Jint.Tests.Runtime; + +public class ArgumentsCacheBehaviorTests +{ + [Fact] + public void ArgsForGeneratorsAreNotReusedFromCache() + { + // Arrange + List logValues = new(); + var engine = new Engine(); + engine.SetValue("log", logValues.Add); + + // Act + engine.Evaluate( + """ + function *method() { + log(arguments[0]); + log(arguments[1]); + } + + function *other() { + log(arguments[0]); + log(arguments[1]); + } + + var generator1 = method(42, undefined); + var generator2 = other(10, undefined); + generator1.next(); + generator2.next(); + """); + + // Assert + Assert.Equal([ + JsNumber.Create(42), + JsValue.Undefined, + JsNumber.Create(10), + JsValue.Undefined, + ], logValues); + } + + [Fact] + public void ArgsForGeneratorsWithBindAreNotReusedFromCache() + { + // Arrange + List logValues = new(); + var engine = new Engine(); + engine.SetValue("log", logValues.Add); + + // Act + engine.Evaluate( + """ + function *method() { + log(arguments[0]); + log(arguments[1]); + } + + function *other() { + log(arguments[0]); + log(arguments[1]); + } + + var methodWithBind = method.bind({}); + var otherWithBind = other.bind({}); + + var generator1 = methodWithBind(42, undefined); + var generator2 = otherWithBind(10, undefined); + generator1.next(); + generator2.next(); + """); + + // Assert + Assert.Equal([ + JsNumber.Create(42), + JsValue.Undefined, + JsNumber.Create(10), + JsValue.Undefined, + ], logValues); + } + + [Fact] + public void ArgsForAsyncFunctionsAreNotReused() + { + // Arrange + List logValues = new(); + var engine = new Engine(new Options(){ ExperimentalFeatures = ExperimentalFeature.All }); + engine.SetValue("log", logValues.Add); + + // Act + engine.Execute( + """ + async function method() { + log(arguments[0]); + log(arguments[1]); + } + + async function other() { + log(arguments[0]); + log(arguments[1]); + } + + method(42, undefined); + other(10, undefined); + """); + engine.RunAvailableContinuations(); + + // Assert + Assert.Equal([ + JsNumber.Create(42), + JsValue.Undefined, + JsNumber.Create(10), + JsValue.Undefined, + ], logValues); + } +} diff --git a/Jint/Native/Function/ScriptFunction.cs b/Jint/Native/Function/ScriptFunction.cs index d9b92669e6..5cebe92787 100644 --- a/Jint/Native/Function/ScriptFunction.cs +++ b/Jint/Native/Function/ScriptFunction.cs @@ -75,6 +75,13 @@ protected internal override JsValue Call(JsValue thisObject, JsCallArguments arg // actual call var context = _engine._activeEvaluationContext ?? new EvaluationContext(_engine); + if (ArgumentsNeedToSurviveCall()) + { + // Take ownership of the array to allow the caller to + // modify/cache it if needed. + arguments = CloneArgumentsArray(arguments); + } + var result = _functionDefinition.EvaluateBody(context, this, arguments); result = calleeContext.LexicalEnvironment.DisposeResources(result); @@ -108,6 +115,11 @@ protected internal override JsValue Call(JsValue thisObject, JsCallArguments arg } } + private static JsCallArguments CloneArgumentsArray(JsCallArguments arguments) + { + return [..arguments]; + } + internal override bool IsConstructor { get @@ -212,4 +224,15 @@ internal void MakeClassConstructor() { _isClassConstructor = true; } + + /// + /// Checks if an argument array passed to execute this function (see ) + /// might still be in use when the call returns. + /// + /// + /// This method will only check if the declaration itself has an indicator for that. + /// It will not check if the code running inside the function saves the arguments outside somewhere. + /// + private bool ArgumentsNeedToSurviveCall() + => FunctionDeclaration is not null && (FunctionDeclaration.Generator || FunctionDeclaration.Async); } From 70cb40fccfe1f99ecb81dbabfe517e3052c6253e Mon Sep 17 00:00:00 2001 From: tomatosalat0 Date: Fri, 7 Nov 2025 17:24:45 +0100 Subject: [PATCH 2/3] Fix formatting --- Jint/Native/Function/ScriptFunction.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Jint/Native/Function/ScriptFunction.cs b/Jint/Native/Function/ScriptFunction.cs index 5cebe92787..33be2471d8 100644 --- a/Jint/Native/Function/ScriptFunction.cs +++ b/Jint/Native/Function/ScriptFunction.cs @@ -117,7 +117,7 @@ protected internal override JsValue Call(JsValue thisObject, JsCallArguments arg private static JsCallArguments CloneArgumentsArray(JsCallArguments arguments) { - return [..arguments]; + return [.. arguments]; } internal override bool IsConstructor From dfa2e3e9bedd378be12fd1ea993baa8e3fb27c90 Mon Sep 17 00:00:00 2001 From: tomatosalat0 Date: Sun, 9 Nov 2025 14:20:37 +0100 Subject: [PATCH 3/3] Moved arguments array ownership detection to function state, extended check to only need ownership when arguments is used. --- .../Runtime/ArgumentsCacheBehaviorTests.cs | 36 +++++++++++++++++++ .../Interpreter/JintFunctionDefinitionTest.cs | 31 ++++++++++++++++ Jint/Engine.cs | 4 +++ Jint/Native/Function/ScriptFunction.cs | 23 ------------ .../Interpreter/JintFunctionDefinition.cs | 12 +++++++ 5 files changed, 83 insertions(+), 23 deletions(-) diff --git a/Jint.Tests/Runtime/ArgumentsCacheBehaviorTests.cs b/Jint.Tests/Runtime/ArgumentsCacheBehaviorTests.cs index 7ee3303217..ded91fe4d7 100644 --- a/Jint.Tests/Runtime/ArgumentsCacheBehaviorTests.cs +++ b/Jint.Tests/Runtime/ArgumentsCacheBehaviorTests.cs @@ -40,6 +40,42 @@ public void ArgsForGeneratorsAreNotReusedFromCache() ], logValues); } + [Fact] + public void NamedArgsForGeneratorsAreNotReusedFromCache() + { + // Arrange + List logValues = new(); + var engine = new Engine(); + engine.SetValue("log", logValues.Add); + + // Act + engine.Evaluate( + """ + function *method(a, b) { + log(a); + log(b); + } + + function *other(a, b) { + log(a); + log(b); + } + + var generator1 = method(42, undefined); + var generator2 = other(10, undefined); + generator1.next(); + generator2.next(); + """); + + // Assert + Assert.Equal([ + JsNumber.Create(42), + JsValue.Undefined, + JsNumber.Create(10), + JsValue.Undefined, + ], logValues); + } + [Fact] public void ArgsForGeneratorsWithBindAreNotReusedFromCache() { diff --git a/Jint.Tests/Runtime/Interpreter/JintFunctionDefinitionTest.cs b/Jint.Tests/Runtime/Interpreter/JintFunctionDefinitionTest.cs index 2a131a0f85..806b2ffda9 100644 --- a/Jint.Tests/Runtime/Interpreter/JintFunctionDefinitionTest.cs +++ b/Jint.Tests/Runtime/Interpreter/JintFunctionDefinitionTest.cs @@ -19,4 +19,35 @@ public void ShouldDetectParameterExpression(string functionCode, bool hasExpress var state = JintFunctionDefinition.BuildState(function); state.HasParameterExpressions.Should().Be(hasExpressions); } + + [Theory] + [InlineData("function g() { }", false)] + [InlineData("function* g() { }", false)] + [InlineData("async function g() { }", false)] + [InlineData("() => { }", false)] + [InlineData("async () => { }", false)] + [InlineData("function g(a) { }", false)] + [InlineData("function* g(a) { }", false)] + [InlineData("async function g(a) { }", false)] + [InlineData("(a) => { }", false)] + [InlineData("async (a) => { }", false)] + [InlineData("function g(a) { _ = arguments[0] }", false)] + [InlineData("function* g(a) { _ = arguments[0] }", true)] + [InlineData("async function g(a) { _ = arguments[0] }", true)] + [InlineData("(a) => { _ = arguments[0] }", false)] + [InlineData("async (a) => { _ = arguments[0] }", true)] + public void ShouldIndicateArgumentsOwnershipIfNeeded(string functionCode, bool requiresOwnership) + { + var parser = new Parser(); + var script = parser.ParseScript(functionCode); + Node statement = script.Body.First(); + var function = (IFunction) ( + statement is ExpressionStatement expr + ? expr.Expression + : statement + ); + + var state = JintFunctionDefinition.BuildState(function); + state.RequiresInputArgumentsOwnership.Should().Be(requiresOwnership); + } } diff --git a/Jint/Engine.cs b/Jint/Engine.cs index c2f0417ead..80c4ef6914 100644 --- a/Jint/Engine.cs +++ b/Jint/Engine.cs @@ -1059,6 +1059,10 @@ private void GlobalDeclarationInstantiation( var hasDuplicates = configuration.HasDuplicates; var simpleParameterList = configuration.IsSimpleParameterList; var hasParameterExpressions = configuration.HasParameterExpressions; + if (configuration.RequiresInputArgumentsOwnership) + { + argumentsList = [.. argumentsList]; + } var canInitializeParametersOnDeclaration = simpleParameterList && !configuration.HasDuplicates; var arguments = canInitializeParametersOnDeclaration ? argumentsList : null; diff --git a/Jint/Native/Function/ScriptFunction.cs b/Jint/Native/Function/ScriptFunction.cs index 33be2471d8..d9b92669e6 100644 --- a/Jint/Native/Function/ScriptFunction.cs +++ b/Jint/Native/Function/ScriptFunction.cs @@ -75,13 +75,6 @@ protected internal override JsValue Call(JsValue thisObject, JsCallArguments arg // actual call var context = _engine._activeEvaluationContext ?? new EvaluationContext(_engine); - if (ArgumentsNeedToSurviveCall()) - { - // Take ownership of the array to allow the caller to - // modify/cache it if needed. - arguments = CloneArgumentsArray(arguments); - } - var result = _functionDefinition.EvaluateBody(context, this, arguments); result = calleeContext.LexicalEnvironment.DisposeResources(result); @@ -115,11 +108,6 @@ protected internal override JsValue Call(JsValue thisObject, JsCallArguments arg } } - private static JsCallArguments CloneArgumentsArray(JsCallArguments arguments) - { - return [.. arguments]; - } - internal override bool IsConstructor { get @@ -224,15 +212,4 @@ internal void MakeClassConstructor() { _isClassConstructor = true; } - - /// - /// Checks if an argument array passed to execute this function (see ) - /// might still be in use when the call returns. - /// - /// - /// This method will only check if the declaration itself has an indicator for that. - /// It will not check if the code running inside the function saves the arguments outside somewhere. - /// - private bool ArgumentsNeedToSurviveCall() - => FunctionDeclaration is not null && (FunctionDeclaration.Generator || FunctionDeclaration.Async); } diff --git a/Jint/Runtime/Interpreter/JintFunctionDefinition.cs b/Jint/Runtime/Interpreter/JintFunctionDefinition.cs index c97517562c..d2dcf6dcdb 100644 --- a/Jint/Runtime/Interpreter/JintFunctionDefinition.cs +++ b/Jint/Runtime/Interpreter/JintFunctionDefinition.cs @@ -192,6 +192,7 @@ internal sealed class State public bool IsSimpleParameterList; public bool HasParameterExpressions; public bool ArgumentsObjectNeeded; + public bool RequiresInputArgumentsOwnership; public List? VarNames; public LinkedList? FunctionsToInitialize; public readonly HashSet FunctionNames = new(); @@ -271,6 +272,17 @@ internal static State BuildState(IFunction function) parameterBindings.Add(KnownKeys.Arguments); } + if (function.Type == NodeType.ArrowFunctionExpression) + { + state.RequiresInputArgumentsOwnership = state.ArgumentsObjectNeeded || + (function.Async && ArgumentsUsageAstVisitor.HasArgumentsReference(function)); + } + else + { + state.RequiresInputArgumentsOwnership = state.ArgumentsObjectNeeded && + (function.Async || function.Generator); + } + state.ParameterBindings = parameterBindings; var varsToInitialize = new List();