Skip to content

Commit

Permalink
[release/7.0] Fix handling of backtracking stack with some loops (#79361
Browse files Browse the repository at this point in the history 
)

* 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.

* Fix declaration of local in regex source generator

* Fix startingStackpos tracking for loop in loop

This extends the fix made earlier around proper handling of the backtracking stack in EmitLoop.  If this loop is inside of another loop, then we need to preserve the startingStackpos on the backtracking stack upon successfully completing the loop, as the outer loop might iterate and cause another occurrence of this loop to run, which will then overwrite our single startingStackpos local.  If that iteration backtracks, we then need to be able to pop the previous iterations startingStackpos and restore its value.

Co-authored-by: Stephen Toub <stoub@microsoft.com>
  • Loading branch information
@github-actions @stephentoub
github-actions[bot] and stephentoub committed Jan 4, 2023
1 parent b65e78a commit 235508d
Show file tree
Hide file tree
Showing 3 changed files with 86 additions and 12 deletions.
38 changes: 32 additions & 6 deletions src/libraries/System.Text.RegularExpressions/gen/RegexGenerator.Emitter.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3873,10 +3873,16 @@ 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;");
additionalDeclarations.Add($"int {startingStackpos} = 0;");
writer.WriteLine($"{startingStackpos} = stackpos;");
}

string originalDoneLabel = doneLabel;
Expand Down Expand Up @@ -4069,6 +4075,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 Expand Up @@ -4124,8 +4146,10 @@ void EmitLoop(RegexNode node)
writer.WriteLine();

// Store the loop's state
EmitStackPush(iterationMayBeEmpty ?
new[] { startingPos!, iterationCount } :
EmitStackPush(
startingPos is not null && startingStackpos is not null ? new[] { startingPos, startingStackpos, iterationCount } :
startingPos is not null ? new[] { startingPos, iterationCount } :
startingStackpos is not null ? new[] { startingStackpos, iterationCount } :
new[] { iterationCount });

// Skip past the backtracking section
Expand All @@ -4136,8 +4160,10 @@ void EmitLoop(RegexNode node)
// Emit a backtracking section that restores the loop's state and then jumps to the previous done label
string backtrack = ReserveName("LoopBacktrack");
MarkLabel(backtrack, emitSemicolon: false);
EmitStackPop(iterationMayBeEmpty ?
new[] { iterationCount, startingPos! } :
EmitStackPop(
startingPos is not null && startingStackpos is not null ? new[] { iterationCount, startingStackpos, startingPos } :
startingPos is not null ? new[] { iterationCount, startingPos } :
startingStackpos is not null ? new[] { iterationCount, startingStackpos } :
new[] { iterationCount });

// We're backtracking. Check the timeout.
Expand Down
51 changes: 45 additions & 6 deletions ...raries/System.Text.RegularExpressions/src/System/Text/RegularExpressions/RegexCompiler.cs
View file
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 Expand Up @@ -4888,11 +4918,15 @@ void EmitLoop(RegexNode node)
if (analysis.IsInLoop(node))
{
// Store the loop's state
EmitStackResizeIfNeeded(1 + (startingPos is not null ? 1 : 0));
EmitStackResizeIfNeeded(1 + (startingPos is not null ? 1 : 0) + (startingStackpos is not null ? 1 : 0));
if (startingPos is not null)
{
EmitStackPush(() => Ldloc(startingPos));
}
if (startingStackpos is not null)
{
EmitStackPush(() => Ldloc(startingStackpos));
}
EmitStackPush(() => Ldloc(iterationCount));

// Skip past the backtracking section
Expand All @@ -4911,6 +4945,11 @@ void EmitLoop(RegexNode node)
// startingPos = base.runstack[--runstack];
EmitStackPop();
Stloc(iterationCount);
if (startingStackpos is not null)
{
EmitStackPop();
Stloc(startingStackpos);
}
if (startingPos is not null)
{
EmitStackPop();
Expand Down
9 changes: 9 additions & 0 deletions src/libraries/System.Text.RegularExpressions/tests/FunctionalTests/Regex.Match.Tests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -363,6 +363,15 @@ 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");
yield return (@"^( | )?((\w\d){3,}){3,}", " 12345678901234567", RegexOptions.None, 0, 18, false, "");
yield return (@"^( | )?((\w\d){3,}){3,}", " 123456789012345678", RegexOptions.None, 0, 19, true, " 123456789012345678");
yield return (@"^( | )?((\w\d){3,}){3,}", " 123456789012345678", RegexOptions.None, 0, 20, true, " 123456789012345678");
yield return (@"^( | )?((\w\d){3,}){3,}", " 123456789012345678", RegexOptions.None, 0, 21, false, "");

// 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 235508d

Please sign in to comment.