Skip to content

Commit

Permalink
Fix handling of backtracking stack with some loops (#79353)
Browse files Browse the repository at this point in the history
With both RegexOptions.Compiled and the Regex source generator, Regex greedy loops with
- a minimum bound of at least 2
- no child constructs that backtrack
- and a child that's more than a one/notone/set (aka things that match a single character)

are possibly leaving state on the backtracking stack when:
- at least one iteration of the loop successfully matches
- but not enough iterations match to make the loop successful such that matching the loop fails

In that case, if a previous construct in the pattern pushed any state onto the backtracking stack such that it expects to be able to pop off and use that state upon backtracking to it, it will potentially pop the erroneously leftover state.  This can then cause execution to go awry, as it's getting back an unexpected value.  That can lead to false positives, false negatives, or exceptions such as an IndexOutOfRangeException due to trying to pop too much from the backtracking stack.

We already have the ability to remember the backtracking stack position when we initially enter the loop so that we can reset to that position later on.  The fix is simply to extend that to also perform that reset when failing the match of such a loop in such circumstances.
  • Loading branch information
stephentoub committed Dec 7, 2022
1 parent e22e456 commit 13a9a3c
Show file tree
Hide file tree
Showing 3 changed files with 62 additions and 6 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -3862,8 +3862,13 @@ void EmitLoop(RegexNode node)

bool isAtomic = rm.Analysis.IsAtomicByAncestor(node);
string? startingStackpos = null;
if (isAtomic)
if (isAtomic || minIterations > 1)
{
// If the loop is atomic, constructs will need to backtrack around it, and as such any backtracking
// state pushed by the loop should be removed prior to exiting the loop. Similarly, if the loop has
// a minimum iteration count greater than 1, we might end up with at least one successful iteration
// only to find we can't iterate further, and will need to clear any pushed state from the backtracking
// stack. For both cases, we need to store the starting stack index so it can be reset to that position.
startingStackpos = ReserveName("startingStackpos");
writer.WriteLine($"int {startingStackpos} = stackpos;");
}
Expand Down Expand Up @@ -4058,6 +4063,22 @@ void EmitLoop(RegexNode node)
using (EmitBlock(writer, $"if ({CountIsLessThan(iterationCount, minIterations)})"))
{
writer.WriteLine($"// All possible iterations have matched, but it's below the required minimum of {minIterations}. Fail the loop.");

// If the minimum iterations is 1, then since we're only here if there are fewer, there must be 0
// iterations, in which case there's nothing to reset. If, however, the minimum iteration count is
// greater than 1, we need to check if there was at least one successful iteration, in which case
// any backtracking state still set needs to be reset; otherwise, constructs earlier in the sequence
// trying to pop their own state will erroneously pop this state instead.
if (minIterations > 1)
{
Debug.Assert(startingStackpos is not null);
using (EmitBlock(writer, $"if ({iterationCount} != 0)"))
{
writer.WriteLine($"// Ensure any stale backtracking state is removed.");
writer.WriteLine($"stackpos = {startingStackpos};");
}
}

Goto(originalDoneLabel);
}
writer.WriteLine();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4543,8 +4543,13 @@ void EmitLoop(RegexNode node)

bool isAtomic = analysis.IsAtomicByAncestor(node);
LocalBuilder? startingStackpos = null;
if (isAtomic)
if (isAtomic || minIterations > 1)
{
// If the loop is atomic, constructs will need to backtrack around it, and as such any backtracking
// state pushed by the loop should be removed prior to exiting the loop. Similarly, if the loop has
// a minimum iteration count greater than 1, we might end up with at least one successful iteration
// only to find we can't iterate further, and will need to clear any pushed state from the backtracking
// stack. For both cases, we need to store the starting stack index so it can be reset to that position.
startingStackpos = DeclareInt32();
Ldloc(stackpos);
Stloc(startingStackpos);
Expand Down Expand Up @@ -4732,7 +4737,6 @@ void EmitLoop(RegexNode node)
}
EmitUncaptureUntilPopped();


// If there's a required minimum iteration count, validate now that we've processed enough iterations.
if (minIterations > 0)
{
Expand All @@ -4751,7 +4755,7 @@ void EmitLoop(RegexNode node)
// since the only value that wouldn't meet that is 0.
if (minIterations > 1)
{
// if (iterationCount < minIterations) goto doneLabel/originalDoneLabel;
// if (iterationCount < minIterations) goto doneLabel;
Ldloc(iterationCount);
Ldc(minIterations);
BltFar(doneLabel);
Expand All @@ -4761,10 +4765,36 @@ void EmitLoop(RegexNode node)
{
// The child doesn't backtrack, which means there's no other way the matched iterations could
// match differently, so if we haven't already greedily processed enough iterations, fail the loop.
// if (iterationCount < minIterations) goto doneLabel/originalDoneLabel;
// if (iterationCount < minIterations)
// {
// if (iterationCount != 0) stackpos = startingStackpos;
// goto originalDoneLabel;
// }

Label enoughIterations = DefineLabel();
Ldloc(iterationCount);
Ldc(minIterations);
BltFar(originalDoneLabel);
Bge(enoughIterations);

// If the minimum iterations is 1, then since we're only here if there are fewer, there must be 0
// iterations, in which case there's nothing to reset. If, however, the minimum iteration count is
// greater than 1, we need to check if there was at least one successful iteration, in which case
// any backtracking state still set needs to be reset; otherwise, constructs earlier in the sequence
// trying to pop their own state will erroneously pop this state instead.
if (minIterations > 1)
{
Debug.Assert(startingStackpos is not null);

Ldloc(iterationCount);
Ldc(0);
BeqFar(originalDoneLabel);

Ldloc(startingStackpos);
Stloc(stackpos);
}
BrFar(originalDoneLabel);

MarkLabel(enoughIterations);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -388,6 +388,11 @@ public static IEnumerable<object[]> Match_MemberData()
yield return ("a*(?:a[ab]*)*", "aaaababbbbbbabababababaaabbb", RegexOptions.None, 0, 28, true, "aaaa");
yield return ("a*(?:a[ab]*?)*?", "aaaababbbbbbabababababaaabbb", RegexOptions.None, 0, 28, true, "aaaa");

// Sequences of loops
yield return (@"(ver\.? |[_ ]+)?\d+(\.\d+){2,3}$", " Ver 2.0", RegexOptions.IgnoreCase, 0, 8, false, "");
yield return (@"(?:|a)?(?:\b\d){2,}", " a 0", RegexOptions.None, 0, 4, false, "");
yield return (@"(?:|a)?(\d){2,}", " a00a", RegexOptions.None, 0, 5, true, "a00");

// Using beginning/end of string chars \A, \Z: Actual - "\\Aaaa\\w+zzz\\Z"
yield return (@"\Aaaa\w+zzz\Z", "aaaasdfajsdlfjzzz", RegexOptions.IgnoreCase, 0, 17, true, "aaaasdfajsdlfjzzz");
yield return (@"\Aaaaaa\w+zzz\Z", "aaaa", RegexOptions.IgnoreCase, 0, 4, false, string.Empty);
Expand Down

0 comments on commit 13a9a3c

Please sign in to comment.