Check cyclic references when evaluating ToObject (#936)

Jint.Tests/Jint.Tests.csproj

@@ -5,6 +5,7 @@
     <AssemblyOriginatorKeyFile>..\Jint\Jint.snk</AssemblyOriginatorKeyFile>
     <SignAssembly>true</SignAssembly>
     <IsPackable>false</IsPackable>
+    <LangVersion>latest</LangVersion>
   </PropertyGroup>
   <ItemGroup>
     <EmbeddedResource Include="Runtime\Scripts\*.*;Parser\Scripts\*.*" />

Jint.Tests/Runtime/InteropTests.cs

@@ -2563,5 +2563,33 @@ namespace Jint.Tests.Runtime
             var wrapper = new ObjectWrapper(_engine, new Dictionary<string, object>());
             Assert.False(wrapper.IsArrayLike);
         }
+
+        [Fact]
+        public void ShouldHandleCyclicReferences()
+        {
+            var engine=new Engine();
+
+            static void Test(string message, object value) { Console.WriteLine(message); }
+
+            engine.Realm.GlobalObject.FastAddProperty("global", engine.Realm.GlobalObject, true, true, true);
+            engine.Realm.GlobalObject.FastAddProperty("test", new DelegateWrapper(engine, (Action<string, object>) Test), true, true, true);
+
+            var ex = Assert.Throws<JavaScriptException>(() => engine.Realm.GlobalObject.ToObject());
+            Assert.Equal("Cyclic reference detected.", ex.Message);
+
+            ex = Assert.Throws<JavaScriptException>(() =>
+                engine.Execute(@"
+                    var demo={};
+                    demo.value=1;
+                    test('Test 1', demo.value===1);
+                    test('Test 2', demo.value);
+                    demo.demo=demo;
+                    test('Test 3', demo);
+                    test('Test 4', global);"
+                )
+            );
+
+            Assert.Equal("Cyclic reference detected.", ex.Message);
+        }
     }
 }

Jint/Collections/ObjectTraverseStack.cs

@@ -0,0 +1,40 @@
+using System.Collections.Generic;
+using Jint.Native;
+using Jint.Runtime;
+
+namespace Jint.Collections
+{
+    /// <summary>
+    /// Helps traversing objects and checks for cyclic references.
+    /// </summary>
+    internal sealed class ObjectTraverseStack
+    {
+        private readonly Engine _engine;
+        private readonly Stack<object> _stack = new();
+
+        public ObjectTraverseStack(Engine engine)
+        {
+            _engine = engine;
+        }
+
+        public void Enter(JsValue value)
+        {
+            if (value is null)
+            {
+                ExceptionHelper.ThrowArgumentNullException(nameof(value));
+            }
+
+            if (_stack.Contains(value))
+            {
+                ExceptionHelper.ThrowTypeError(_engine.Realm, "Cyclic reference detected.");
+            }
+
+            _stack.Push(value);
+        }
+
+        public void Exit()
+        {
+            _stack.Pop();
+        }
+    }
+}

Jint/Native/Iterator/IteratorInstance.cs

@@ -26,7 +26,8 @@ namespace Jint.Native.Iterator
 
         public override object ToObject()
         {
-            throw new System.NotImplementedException();
+            ExceptionHelper.ThrowNotImplementedException();
+            return null;
         }
 
         public override bool Equals(JsValue other)

Jint/Native/Json/JsonSerializer.cs

@@ -1,5 +1,6 @@
 using System.Collections.Generic;
 using System.Linq;
+using Jint.Collections;
 using Jint.Native.Global;
 using Jint.Native.Object;
 using Jint.Runtime;
@@ -16,14 +17,14 @@ namespace Jint.Native.Json
             _engine = engine;
         }
 
-        private Stack<object> _stack;
+        private ObjectTraverseStack _stack;
         private string _indent, _gap;
         private List<JsValue> _propertyList;
         private JsValue _replacerFunction = Undefined.Instance;
 
         public JsValue Serialize(JsValue value, JsValue replacer, JsValue space)
         {
-            _stack = new Stack<object>();
+            _stack = new ObjectTraverseStack(_engine);
 
             // for JSON.stringify(), any function passed as the first argument will return undefined
             // if the replacer is not defined. The function is not called either.
@@ -247,8 +248,7 @@ namespace Jint.Native.Json
 
         private string SerializeArray(JsValue value)
         {
-            EnsureNonCyclicity(value);
-            _stack.Push(value);
+            _stack.Enter(value);
             var stepback = _indent;
             _indent = _indent + _gap;
             var partial = new List<string>();
@@ -262,7 +262,7 @@ namespace Jint.Native.Json
             }
             if (partial.Count == 0)
             {
-                _stack.Pop();
+                _stack.Exit();
                 return "[]";
             }
 
@@ -280,30 +280,16 @@ namespace Jint.Native.Json
                 final = "[\n" + _indent + properties + "\n" + stepback + "]";
             }
 
-            _stack.Pop();
+            _stack.Exit();
             _indent = stepback;
             return final;
         }
 
-        private void EnsureNonCyclicity(object value)
-        {
-            if (value == null)
-            {
-                ExceptionHelper.ThrowArgumentNullException(nameof(value));
-            }
-
-            if (_stack.Contains(value))
-            {
-                ExceptionHelper.ThrowTypeError(_engine.Realm, "Cyclic reference detected.");
-            }
-        }
-
         private string SerializeObject(ObjectInstance value)
         {
             string final;
 
-            EnsureNonCyclicity(value);
-            _stack.Push(value);
+            _stack.Enter(value);
             var stepback = _indent;
             _indent += _gap;
 
@@ -346,7 +332,7 @@ namespace Jint.Native.Json
                     final = "{\n" + _indent + properties + "\n" + stepback + "}";
                 }
             }
-            _stack.Pop();
+            _stack.Exit();
             _indent = stepback;
             return final;
         }

Jint/Native/Object/ObjectInstance.cs

@@ -891,11 +891,19 @@ namespace Jint.Native.Object
 
         public override object ToObject()
         {
+            return ToObject(new ObjectTraverseStack(_engine));
+        }
+
+        private object ToObject(ObjectTraverseStack stack)
+        {
+            stack.Enter(this);
             if (this is IObjectWrapper wrapper)
             {
+                stack.Exit();
                 return wrapper.Target;
             }
 
+            object converted = null;
             switch (Class)
             {
                 case ObjectClass.Array:
@@ -910,49 +918,47 @@ namespace Jint.Native.Object
                             if (kpresent)
                             {
                                 var kvalue = arrayInstance.Get(k);
-                                result[k] = kvalue.ToObject();
+                                var value = kvalue is ObjectInstance oi
+                                    ? oi.ToObject(stack)
+                                    : kvalue.ToObject();
+                                result[k] = value;
                             }
                             else
                             {
                                 result[k] = null;
                             }
                         }
-
-                        return result;
+                        converted = result;
                     }
-
                     break;
 
                 case ObjectClass.String:
                     if (this is StringInstance stringInstance)
                     {
-                        return stringInstance.PrimitiveValue.ToString();
+                        converted = stringInstance.PrimitiveValue.ToString();
                     }
-
                     break;
 
                 case ObjectClass.Date:
                     if (this is DateInstance dateInstance)
                     {
-                        return dateInstance.ToDateTime();
+                        converted = dateInstance.ToDateTime();
                     }
-
                     break;
 
                 case ObjectClass.Boolean:
                     if (this is BooleanInstance booleanInstance)
                     {
-                        return ((JsBoolean) booleanInstance.PrimitiveValue)._value
-                             ? JsBoolean.BoxedTrue
-                             : JsBoolean.BoxedFalse;
+                        converted = ((JsBoolean) booleanInstance.PrimitiveValue)._value
+                            ? JsBoolean.BoxedTrue
+                            : JsBoolean.BoxedFalse;
                     }
-
                     break;
 
                 case ObjectClass.Function:
                     if (this is FunctionInstance function)
                     {
-                        return (Func<JsValue, JsValue[], JsValue>) function.Call;
+                        converted = (Func<JsValue, JsValue[], JsValue>) function.Call;
                     }
 
                     break;
@@ -960,17 +966,15 @@ namespace Jint.Native.Object
                 case ObjectClass.Number:
                     if (this is NumberInstance numberInstance)
                     {
-                        return numberInstance.NumberData._value;
+                        converted = numberInstance.NumberData._value;
                     }
-
                     break;
 
                 case ObjectClass.RegExp:
                     if (this is RegExpInstance regeExpInstance)
                     {
-                        return regeExpInstance.Value;
+                        converted = regeExpInstance.Value;
                     }
-
                     break;
 
                 case ObjectClass.Arguments:
@@ -983,14 +987,23 @@ namespace Jint.Native.Object
                             continue;
                         }
 
-                        o.Add(p.Key.ToString(), Get(p.Key, this).ToObject());
+                        var key = p.Key.ToString();
+                        var propertyValue = Get(p.Key, this);
+                        var value = propertyValue is ObjectInstance oi
+                            ? oi.ToObject(stack)
+                            : propertyValue.ToObject();
+                        o.Add(key, value);
                     }
 
-                    return o;
+                    converted = o;
+                    break;
+                default:
+                    converted = this;
+                    break;
             }
 
-
-            return this;
+            stack.Exit();
+            return converted;
         }
 
         /// <summary>