Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Fix handling of backtracking stack with some loops
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 authored and github-actions committed Dec 7, 2022
commit f3e9ca1ba4af71c308db3955e7a5bf25a10847f7
Original file line number Diff line number Diff line change
Expand Up @@ -3873,8 +3873,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 @@ -4069,6 +4074,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 @@ -4613,8 +4613,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 @@ -4802,7 +4807,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 @@ -4821,7 +4825,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 @@ -4831,10 +4835,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 @@ -363,6 +363,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