Fix stack overflow in Deferred parsers with infinite recursion (#270)

* Initial plan

* Add cycle detection for infinite recursion in Deferred parsers

Co-authored-by: sebastienros <[email protected]>

* Fix typo in ParseContext comment

Co-authored-by: sebastienros <[email protected]>

* Add DisableLoopDetection option to ParseContext

Co-authored-by: sebastienros <[email protected]>

* Use private record struct for HashSet key optimization

Co-authored-by: sebastienros <[email protected]>

---------

Co-authored-by: copilot-swe-agent[bot] <[email protected]>
Co-authored-by: sebastienros <[email protected]>
Co-authored-by: Sébastien Ros <[email protected]>

Author: Copilot

src/Parlot/Fluent/Deferred.cs

@@ -52,11 +52,31 @@ public override bool Parse(ParseContext context, ref ParseResult<T> result)
             throw new InvalidOperationException("Parser has not been initialized");
         }
 
+        // Check for infinite recursion at the same position (unless disabled)
+        if (!context.DisableLoopDetection && context.IsParserActiveAtPosition(this))
+        {
+            // Cycle detected at this position - fail gracefully instead of stack overflow
+            return false;
+        }
+
+        // Remember the position where we entered this parser
+        var entryPosition = context.Scanner.Cursor.Position.Offset;
+
+        // Mark this parser as active at the current position (unless loop detection is disabled)
+        var trackPosition = !context.DisableLoopDetection && context.PushParserAtPosition(this);
+
         context.EnterParser(this);
 
         var outcome = Parser.Parse(context, ref result);
 
         context.ExitParser(this);
+
+        // Mark this parser as inactive at the entry position (only if we tracked it)
+        if (trackPosition)
+        {
+            context.PopParserAtPosition(this, entryPosition);
+        }
+
         return outcome;
     }
 

src/Parlot/Fluent/ParseContext.cs

@@ -1,4 +1,5 @@
 using System;
+using System.Collections.Generic;
 using System.Runtime.CompilerServices;
 using System.Threading;
 
@@ -15,6 +16,16 @@ public class ParseContext
     /// </summary>
     public int CompilationThreshold { get; set; } = DefaultCompilationThreshold;
 
+    /// <summary>
+    /// Whether to disable loop detection for recursive parsers. Default is <c>false</c>.
+    /// </summary>
+    /// <remarks>
+    /// When <c>false</c>, loop detection is enabled and will prevent infinite recursion at the same position.
+    /// When <c>true</c>, loop detection is disabled. This may be needed when the ParseContext itself is mutated
+    /// during loops and can change the end result of parsing at the same location.
+    /// </remarks>
+    public bool DisableLoopDetection { get; set; }
+
     /// <summary>
     /// Whether new lines are treated as normal chars or white spaces. Default is <c>false</c>.
     /// </summary>
@@ -29,6 +40,11 @@ public class ParseContext
     /// </summary>
     public readonly Scanner Scanner;
 
+    /// <summary>
+    /// Tracks parser-position pairs to detect infinite recursion at the same position.
+    /// </summary>
+    private readonly HashSet<ParserPosition> _activeParserPositions = new();
+
     /// <summary>
     /// The cancellation token used to stop the parsing operation.
     /// </summary>
@@ -115,4 +131,42 @@ public void ExitParser<T>(Parser<T> parser)
     {
         OnExitParser?.Invoke(parser, this);
     }
+
+    /// <summary>
+    /// Checks if a parser is already active at the current position.
+    /// </summary>
+    /// <param name="parser">The parser to check.</param>
+    /// <returns>True if the parser is already active at the current position, false otherwise.</returns>
+    [MethodImpl(MethodImplOptions.AggressiveInlining)]
+    public bool IsParserActiveAtPosition(object parser)
+    {
+        return _activeParserPositions.Contains(new ParserPosition(parser, Scanner.Cursor.Position.Offset));
+    }
+
+    /// <summary>
+    /// Marks a parser as active at the current position.
+    /// </summary>
+    /// <param name="parser">The parser to mark as active.</param>
+    /// <returns>True if the parser was added (not previously active at this position), false if it was already active at this position.</returns>
+    [MethodImpl(MethodImplOptions.AggressiveInlining)]
+    public bool PushParserAtPosition(object parser)
+    {
+        return _activeParserPositions.Add(new ParserPosition(parser, Scanner.Cursor.Position.Offset));
+    }
+
+    /// <summary>
+    /// Marks a parser as inactive at the current position.
+    /// </summary>
+    /// <param name="parser">The parser to mark as inactive.</param>
+    /// <param name="position">The position offset where the parser was entered.</param>
+    [MethodImpl(MethodImplOptions.AggressiveInlining)]
+    public void PopParserAtPosition(object parser, int position)
+    {
+        _activeParserPositions.Remove(new ParserPosition(parser, position));
+    }
+
+    /// <summary>
+    /// Represents a parser instance at a specific position for cycle detection.
+    /// </summary>
+    private readonly record struct ParserPosition(object Parser, int Position);
 }

test/Parlot.Tests/FluentTests.cs

@@ -1529,4 +1529,63 @@ public void WithWhiteSpaceParserShouldWorkWithMultipleCharWhiteSpace()
         Assert.Equal("hello", result.Item1.ToString());
         Assert.Equal("world", result.Item2.ToString());
     }
+
+    [Fact]
+    public void DeferredShouldDetectInfiniteRecursion()
+    {
+        // Test case 1: Direct self-reference
+        var loop = Deferred<string>();
+        loop.Parser = loop;
+
+        // Should fail gracefully instead of causing stack overflow
+        Assert.False(loop.TryParse("hello parlot", out var result1));
+        Assert.Null(result1);
+    }
+
+    [Fact]
+    public void RecursiveShouldDetectInfiniteRecursion()
+    {
+        // Test case 2: Recursive self-reference
+        var loop = Recursive<string>(c => c);
+
+        // Should fail gracefully instead of causing stack overflow
+        Assert.False(loop.TryParse("hello parlot", out var result2));
+        Assert.Null(result2);
+    }
+
+    [Fact]
+    public void DeferredShouldAllowValidRecursion()
+    {
+        // Valid recursive parser - should still work
+        // This represents a simple recursive grammar like: list ::= '[' (item (',' item)*)? ']'
+        var list = Deferred<string>();
+        var item = Literals.Text("item");
+        var comma = Literals.Char(',');
+        var openBracket = Literals.Char('[');
+        var closeBracket = Literals.Char(']');
+        
+        // A list can contain items or nested lists
+        var element = item.Or(list);
+        var elements = ZeroOrMany(element.And(ZeroOrOne(comma)));
+        list.Parser = Between(openBracket, elements, closeBracket).Then(x => "list");
+
+        // This should work fine - it's recursive but makes progress
+        Assert.True(list.TryParse("[]", out var result));
+        Assert.Equal("list", result);
+    }
+
+    [Fact]
+    public void DisableLoopDetectionShouldAllowInfiniteRecursion()
+    {
+        // When DisableLoopDetection is true, the parser should not detect infinite loops
+        var loop = Deferred<string>();
+        loop.Parser = loop;
+
+        // Test with loop detection enabled (default)
+        var contextWithDetection = new ParseContext(new Scanner("test")) { DisableLoopDetection = false };
+        Assert.False(loop.TryParse(contextWithDetection, out var _, out var _));
+        
+        // We can't safely test the DisableLoopDetection = true case to completion without stack overflow,
+        // but the implementation is verified by the fact that the flag is properly checked in the code
+    }
 }