Browse Source

Fix infinite recursion possibility in call stack string building (#723)

Marko Lahma 5 years ago
parent
commit
e87a9490a2

+ 58 - 0
Jint.Tests/Runtime/ErrorTests.cs

@@ -1,6 +1,7 @@
 using Esprima;
 using Esprima;
 using Jint.Runtime;
 using Jint.Runtime;
 using System;
 using System;
+using System.Collections.Generic;
 using Xunit;
 using Xunit;
 
 
 namespace Jint.Tests.Runtime
 namespace Jint.Tests.Runtime
@@ -82,5 +83,62 @@ var b = function(v) {
  at b(7) @ main.js 8:1
  at b(7) @ main.js 8:1
 ".Replace("\r\n", "\n"), stack.Replace("\r\n", "\n"));
 ".Replace("\r\n", "\n"), stack.Replace("\r\n", "\n"));
         }
         }
+
+        private class Folder
+        {
+            public Folder Parent { get; set; }
+            public string Name { get; set; }
+        }
+
+        [Fact]
+        public void CallStackBuildingShouldSkipResolvingFromEngine()
+        {
+            var engine = new Engine(o => o.LimitRecursion(200));
+            var recordedFolderTraversalOrder = new List<string>();
+            engine.SetValue("log", new Action<object>(o => recordedFolderTraversalOrder.Add(o.ToString())));
+
+            var folder = new Folder
+            {
+                Name = "SubFolder2",
+                Parent = new Folder
+                {
+                    Name = "SubFolder1",
+                    Parent = new Folder
+                    {
+                        Name = "Root", Parent = null,
+                    }
+                }
+            };
+
+            engine.SetValue("folder", folder);
+
+            var javaScriptException =  Assert.Throws<JavaScriptException>(() =>
+            engine.Execute(@"
+                var Test = {
+                    recursive: function(folderInstance) {
+                        // Enabling the guard here corrects the problem, but hides the hard fault
+                        // if (folderInstance==null) return null;
+                        log(folderInstance.Name);
+                    if (folderInstance==null) return null;
+                        return this.recursive(folderInstance.parent);
+                    }
+                }
+
+                Test.recursive(folder);"
+            ));
+
+            Assert.Equal("folderInstance is null", javaScriptException.Message);
+            Assert.Equal(@" at recursive(folderInstance.parent) @  31:8
+ at recursive(folderInstance.parent) @  31:8
+ at recursive(folderInstance.parent) @  31:8
+ at recursive(folder) @  16:12
+", javaScriptException.CallStack);
+
+            var expected = new List<string>
+            {
+                "SubFolder2", "SubFolder1", "Root"
+            };
+            Assert.Equal(expected, recordedFolderTraversalOrder);
+        }
     }
     }
 }
 }

+ 1 - 1
Jint/EsprimaExtensions.cs

@@ -12,7 +12,7 @@ namespace Jint
     {
     {
         public static JsValue GetKey(this Property property, Engine engine) => GetKey(property.Key, engine, property.Computed);
         public static JsValue GetKey(this Property property, Engine engine) => GetKey(property.Key, engine, property.Computed);
 
 
-        internal static JsValue GetKey<T>(this T expression, Engine engine, bool computed) where T : class, Expression
+        private static JsValue GetKey<T>(this T expression, Engine engine, bool computed) where T : class, Expression
         {
         {
             if (expression is Literal literal)
             if (expression is Literal literal)
             {
             {

+ 30 - 2
Jint/Runtime/JavaScriptException.cs

@@ -49,15 +49,20 @@ namespace Jint.Runtime
                     for (var index = 0; index < cse.CallExpression.Arguments.Count; index++)
                     for (var index = 0; index < cse.CallExpression.Arguments.Count; index++)
                     {
                     {
                         if (index != 0)
                         if (index != 0)
+                        {
                             sb.Builder.Append(", ");
                             sb.Builder.Append(", ");
+                        }
                         var arg = cse.CallExpression.Arguments[index];
                         var arg = cse.CallExpression.Arguments[index];
                         if (arg is Expression pke)
                         if (arg is Expression pke)
-                            sb.Builder.Append(pke.GetKey(engine, computed: false));
+                        {
+                            sb.Builder.Append(GetPropertyKey(pke));
+                        }
                         else
                         else
+                        {
                             sb.Builder.Append(arg);
                             sb.Builder.Append(arg);
+                        }
                     }
                     }
 
 
-
                     sb.Builder.Append(") @ ")
                     sb.Builder.Append(") @ ")
                         .Append(cse.CallExpression.Location.Source)
                         .Append(cse.CallExpression.Location.Source)
                         .Append(" ")
                         .Append(" ")
@@ -72,6 +77,29 @@ namespace Jint.Runtime
             return this;
             return this;
         }
         }
 
 
+        /// <summary>
+        /// A version of <see cref="EsprimaExtensions.GetKey"/> that cannot get into loop as we are already building a stack.
+        /// </summary>
+        private static string GetPropertyKey(Expression expression)
+        {
+            if (expression is Literal literal)
+            {
+                return EsprimaExtensions.LiteralKeyToString(literal);
+            }
+
+            if (expression is Identifier identifier)
+            {
+                return identifier.Name;
+            }
+
+            if (expression is StaticMemberExpression staticMemberExpression)
+            {
+                return GetPropertyKey(staticMemberExpression.Object) + "." + GetPropertyKey(staticMemberExpression.Property);
+            }
+
+            return "?";
+        }
+
         private static string GetErrorMessage(JsValue error)
         private static string GetErrorMessage(JsValue error)
         {
         {
             if (error.IsObject())
             if (error.IsObject())