Fix RegexOptions.NonBacktracking matching end anchors at timeout check boundaries

[olsaarik]

This PR adds a test for and fixes #74467.

The test is the more complicated part, as it needs to find how many characters the innermost matching loop is processing at a time. Since that value is not public it first does a binary search over different input lengths with a 1-tick timeout to find the minimum input length to trigger the timeout. Then with that a pattern of ^a*$ needs to not match an input of form "aaa...ab" where there are exactly enough a's to pop out of the loop right before the b.

Including the test in the unit test project would have been another option, but the matcher's sources aren't built into that currently.

The fix itself is quite simple, with an integer bound passed in instead of slicing the input (since the slice length is used to indicate where the whole input ends for anchors).

[ghost]

Tagging subscribers to this area: @dotnet/area-system-text-regularexpressions
See info in area-owners.md if you want to be subscribed.

Issue Details

---

This PR adds a test for and fixes #74467.

The test is the more complicated part, as it needs to find how many characters the innermost matching loop is processing at a time. Since that value is not public it first does a binary search over different input lengths with a 1-tick timeout to find the minimum input length to trigger the timeout. Then with that a pattern of ^a*$ needs to not match an input of form "aaa...ab" where there are exactly enough a's to pop out of the loop right before the b.

Including the test in the unit test project would have been another option, but the matcher's sources aren't built into that currently.

The fix itself is quite simple, with an integer bound passed in instead of slicing the input (since the slice length is used to indicate where the whole input ends for anchors).

Author:	olsaarik
Assignees:	-
Labels:	area-System.Text.RegularExpressions
Milestone:	-

[stephentoub]

I'm missing where exactly the problem is. To save me the trouble of debugging, can you point me to where it is and why passing in the length separately rather than slicing fixes it?

[joperezr]

So if I understood correctly the bug was here where we were not checking if we are at the end of the input, right? I'm still not understanding why the problem is only happening for anchors

[olsaarik]

The problem was specifically with end anchors, as they rely on the position ID of the next character being -1 exactly when it is the end of the whole input. When they inadvertently matched, they would cause the whole search to end prematurely instead of the outer loop just checking the timeout and going back in for more input.

[stephentoub]

This doesn't seem sound. BinarySearch expects a consistent comparer, but this comparer can change answer from call to call.

[stephentoub]

Can we just use your test from the issue?

[olsaarik]

I'll simplify the test by hardcoding the charsPerTimeoutCheck and including asserts that validate it's indeed the right value before the actual test.

[olsaarik]

I simplified the test even further, now there's just a constant that has to be large enough to trigger timeout checks. I realized it doesn't have to be exact for the test pattern ^a*$, as treating a timeout check in any point of a string "aaaaaaa...aaab" as an end-of-input would cause a match. I verified that the test indeed does fail if I reintroduce the slicing.

[stephentoub]

@olsaarik, where are we with this? I assume it's important this make it into .NET 7.

[olsaarik]

@stephentoub The source of the bug was an interaction with the two (uint)pos >= (uint)input.Length checks in the IInputReader implementations here:

runtime/src/libraries/System.Text.RegularExpressions/src/System/Text/RegularExpressions/Symbolic/SymbolicRegexMatcher.cs

Lines 1218 to 1239 in 8a3a702

	private readonly struct NoZAnchorInputReader : IInputReader
	{
	public static int GetPositionId(SymbolicRegexMatcher<TSet> matcher, ReadOnlySpan<char> input, int pos) =>
	(uint)pos >= (uint)input.Length ? -1 : matcher._mintermClassifier.GetMintermID(input[pos]);
	}
	/// <summary>This reader includes full handling of an \n as the last character of input for the \Z anchor.</summary>
	private readonly struct FullInputReader : IInputReader
	{
	public static int GetPositionId(SymbolicRegexMatcher<TSet> matcher, ReadOnlySpan<char> input, int pos)
	{
	if ((uint)pos >= (uint)input.Length)
	return -1;
	int c = input[pos];
	// Find the minterm, handling the special case for the last \n for states that start with a relevant anchor
	return c == '\n' && pos == input.Length - 1 ?
	matcher._minterms.Length : // mintermId = minterms.Length represents an \n at the very end of input
	matcher._mintermClassifier.GetMintermID(c);
	}
	}

Anchors sensitive to the end-of-input use the -1 position ID coming out of that check to decide that they match.

[olsaarik]

If the simplified test looks good I think this PR should be good to merge as soon as the tests pass. I reran that failing test, which was due to something unrelated to regex.

[stephentoub]

Thanks!

[stephentoub]

/backport to release/7.0

[github-actions]

Started backporting to release/7.0: https://github.com/dotnet/runtime/actions/runs/3018567091