Skip to content

Fix RegexOptions.NonBacktracking matching end anchors at timeout check boundaries - #74525

Merged
olsaarik merged 6 commits into
dotnet:mainfrom
olsaarik:nonbacktracking-timeout-anchors-fix
Sep 8, 2022
Merged

Fix RegexOptions.NonBacktracking matching end anchors at timeout check boundaries#74525
olsaarik merged 6 commits into
dotnet:mainfrom
olsaarik:nonbacktracking-timeout-anchors-fix

Conversation

@olsaarik

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown

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

stephentoub commented Aug 28, 2022

Copy link
Copy Markdown
Member

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?


// If there is more input available try to transition with the next character.
if (!IsMintermId(positionId) || !TStateHandler.TryTakeTransition(this, ref state, positionId))
if (pos >= length || !TStateHandler.TryTakeTransition(this, ref state, positionId))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return 1;
}
}
int charsPerTimeoutCheck = Array.BinarySearch(Enumerable.Range(0, 16_000).ToArray(), -1, Comparer<int>.Create(IsCharsPerTimeoutCheck));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we just use your test from the issue?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

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

@stephentoubstephentoub added this to the 7.0.0 milestone Sep 6, 2022
@olsaarik

Copy link
Copy Markdown
ContributorAuthor

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

privatereadonlystructNoZAnchorInputReader:IInputReader
{
publicstaticintGetPositionId(SymbolicRegexMatcher<TSet>matcher,ReadOnlySpan<char>input,intpos)=>
(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>
privatereadonlystructFullInputReader:IInputReader
{
publicstaticintGetPositionId(SymbolicRegexMatcher<TSet>matcher,ReadOnlySpan<char>input,intpos)
{
if((uint)pos>=(uint)input.Length)
return-1;
intc=input[pos];
// Find the minterm, handling the special case for the last \n for states that start with a relevant anchor
returnc=='\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

Copy link
Copy Markdown
ContributorAuthor

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.

@stephentoubstephentoub left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@olsaarik
olsaarik merged commit 239f550 into dotnet:mainSep 8, 2022
@olsaarik
olsaarik deleted the nonbacktracking-timeout-anchors-fix branch September 8, 2022 22:00
@stephentoub

Copy link
Copy Markdown
Member

/backport to release/7.0

@github-actions

Copy link
Copy Markdown
Contributor

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

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

NonBacktracking treats timeout check boundaries as end-of-input for anchors

3 participants

@olsaarik@stephentoub@joperezr
, 'i'); if (__m === '*' || __re.test(location.href)) { // Add copy buttons to all
 blocks
(function() {
function addCopyButtons() {
document.querySelectorAll('pre code').forEach(function(codeBlock) {
if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;
codeBlock.parentElement.setAttribute('data-copy-added', 'true');
var btn = document.createElement('button');
btn.textContent = 'Copy';
btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';
btn.onmouseover = function() { this.style.opacity = '1'; };
btn.onmouseout = function() { this.style.opacity = '0.7'; };
btn.onclick = function() {
navigator.clipboard.writeText(codeBlock.textContent).then(function() {
btn.textContent = 'Copied!';
setTimeout(function() { btn.textContent = 'Copy'; }, 1500);
});
};
codeBlock.parentElement.style.position = 'relative';
codeBlock.parentElement.appendChild(btn);
});
}
addCopyButtons();
// Re-run on dynamic content
var observer = new MutationObserver(addCopyButtons);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Fix RegexOptions.NonBacktracking matching end anchors at timeout check boundaries by olsaarik · Pull Request #74525 · dotnet/runtime · GitHub
Skip to content

Fix RegexOptions.NonBacktracking matching end anchors at timeout check boundaries - #74525

Merged
olsaarik merged 6 commits into
dotnet:mainfrom
olsaarik:nonbacktracking-timeout-anchors-fix
Sep 8, 2022
Merged

Fix RegexOptions.NonBacktracking matching end anchors at timeout check boundaries#74525
olsaarik merged 6 commits into
dotnet:mainfrom
olsaarik:nonbacktracking-timeout-anchors-fix

Conversation

@olsaarik

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown

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

stephentoub commented Aug 28, 2022

Copy link
Copy Markdown
Member

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?


// If there is more input available try to transition with the next character.
if (!IsMintermId(positionId) || !TStateHandler.TryTakeTransition(this, ref state, positionId))
if (pos >= length || !TStateHandler.TryTakeTransition(this, ref state, positionId))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return 1;
}
}
int charsPerTimeoutCheck = Array.BinarySearch(Enumerable.Range(0, 16_000).ToArray(), -1, Comparer<int>.Create(IsCharsPerTimeoutCheck));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we just use your test from the issue?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

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

@stephentoubstephentoub added this to the 7.0.0 milestone Sep 6, 2022
@olsaarik

Copy link
Copy Markdown
ContributorAuthor

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

privatereadonlystructNoZAnchorInputReader:IInputReader
{
publicstaticintGetPositionId(SymbolicRegexMatcher<TSet>matcher,ReadOnlySpan<char>input,intpos)=>
(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>
privatereadonlystructFullInputReader:IInputReader
{
publicstaticintGetPositionId(SymbolicRegexMatcher<TSet>matcher,ReadOnlySpan<char>input,intpos)
{
if((uint)pos>=(uint)input.Length)
return-1;
intc=input[pos];
// Find the minterm, handling the special case for the last \n for states that start with a relevant anchor
returnc=='\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

Copy link
Copy Markdown
ContributorAuthor

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.

@stephentoubstephentoub left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@olsaarik
olsaarik merged commit 239f550 into dotnet:mainSep 8, 2022
@olsaarik
olsaarik deleted the nonbacktracking-timeout-anchors-fix branch September 8, 2022 22:00
@stephentoub

Copy link
Copy Markdown
Member

/backport to release/7.0

@github-actions

Copy link
Copy Markdown
Contributor

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

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

NonBacktracking treats timeout check boundaries as end-of-input for anchors

3 participants

@olsaarik@stephentoub@joperezr
, 'i'); if (__m === '*' || __re.test(location.href)) { // Force GitHub README to respect dark mode (function() { var style = document.createElement('style'); style.textContent = ' .markdown-body { color-scheme: dark light; } .markdown-body pre { background: #161b22 !important; } .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; } .markdown-body table th, .markdown-body table td { border-color: #30363d !important; } .markdown-body img { background: #0d1117; } .markdown-body blockquote { border-left-color: #8b949e; } .markdown-body hr { border-color: #30363d; } '; document.head.appendChild(style); })(); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' Fix RegexOptions.NonBacktracking matching end anchors at timeout check boundaries by olsaarik · Pull Request #74525 · dotnet/runtime · GitHub
Skip to content

Fix RegexOptions.NonBacktracking matching end anchors at timeout check boundaries - #74525

Merged
olsaarik merged 6 commits into
dotnet:mainfrom
olsaarik:nonbacktracking-timeout-anchors-fix
Sep 8, 2022
Merged

Fix RegexOptions.NonBacktracking matching end anchors at timeout check boundaries#74525
olsaarik merged 6 commits into
dotnet:mainfrom
olsaarik:nonbacktracking-timeout-anchors-fix

Conversation

@olsaarik

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown

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

stephentoub commented Aug 28, 2022

Copy link
Copy Markdown
Member

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?


// If there is more input available try to transition with the next character.
if (!IsMintermId(positionId) || !TStateHandler.TryTakeTransition(this, ref state, positionId))
if (pos >= length || !TStateHandler.TryTakeTransition(this, ref state, positionId))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return 1;
}
}
int charsPerTimeoutCheck = Array.BinarySearch(Enumerable.Range(0, 16_000).ToArray(), -1, Comparer<int>.Create(IsCharsPerTimeoutCheck));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we just use your test from the issue?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

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

@stephentoubstephentoub added this to the 7.0.0 milestone Sep 6, 2022
@olsaarik

Copy link
Copy Markdown
ContributorAuthor

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

privatereadonlystructNoZAnchorInputReader:IInputReader
{
publicstaticintGetPositionId(SymbolicRegexMatcher<TSet>matcher,ReadOnlySpan<char>input,intpos)=>
(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>
privatereadonlystructFullInputReader:IInputReader
{
publicstaticintGetPositionId(SymbolicRegexMatcher<TSet>matcher,ReadOnlySpan<char>input,intpos)
{
if((uint)pos>=(uint)input.Length)
return-1;
intc=input[pos];
// Find the minterm, handling the special case for the last \n for states that start with a relevant anchor
returnc=='\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

Copy link
Copy Markdown
ContributorAuthor

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.

@stephentoubstephentoub left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@olsaarik
olsaarik merged commit 239f550 into dotnet:mainSep 8, 2022
@olsaarik
olsaarik deleted the nonbacktracking-timeout-anchors-fix branch September 8, 2022 22:00
@stephentoub

Copy link
Copy Markdown
Member

/backport to release/7.0

@github-actions

Copy link
Copy Markdown
Contributor

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

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

NonBacktracking treats timeout check boundaries as end-of-input for anchors

3 participants

@olsaarik@stephentoub@joperezr
, 'i'); if (__m === '*' || __re.test(location.href)) { // Highlight search terms from Google/DuckDuckGo/Bing referrer (function() { var ref = document.referrer; var terms = []; if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) { var url = new URL(ref); var q = url.searchParams.get('q') || url.searchParams.get('p'); if (q) { terms = q.split(/\s+/).filter(function(t) { return t.length > 2; }); } } if (terms.length === 0) return; var style = document.createElement('style'); style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }'; document.head.appendChild(style); function highlight(node) { if (node.nodeType === 3) { // text node var text = node.textContent; var found = false; terms.forEach(function(term) { var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\]\\]/g, '\\') + ')', 'gi'); if (regex.test(text)) { found = true; var frag = document.createDocumentFragment(); var parts = text.split(regex); parts.forEach(function(part, i) { if (i % 2 === 0) { frag.appendChild(document.createTextNode(part)); } else { var span = document.createElement('span'); span.className = 'userscript-highlight'; span.textContent = part; frag.appendChild(span); } }); node.parentNode.replaceChild(frag, node); } }); } else if (node.nodeType === 1 && node.childNodes) { // element var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT']; if (!skipTags.includes(node.tagName)) { Array.from(node.childNodes).forEach(highlight); } } } highlight(document.body); // Re-highlight on dynamic content var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1 || node.nodeType === 3) highlight(node); }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' Fix RegexOptions.NonBacktracking matching end anchors at timeout check boundaries by olsaarik · Pull Request #74525 · dotnet/runtime · GitHub
Skip to content

Fix RegexOptions.NonBacktracking matching end anchors at timeout check boundaries - #74525

Merged
olsaarik merged 6 commits into
dotnet:mainfrom
olsaarik:nonbacktracking-timeout-anchors-fix
Sep 8, 2022
Merged

Fix RegexOptions.NonBacktracking matching end anchors at timeout check boundaries#74525
olsaarik merged 6 commits into
dotnet:mainfrom
olsaarik:nonbacktracking-timeout-anchors-fix

Conversation

@olsaarik

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown

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

stephentoub commented Aug 28, 2022

Copy link
Copy Markdown
Member

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?


// If there is more input available try to transition with the next character.
if (!IsMintermId(positionId) || !TStateHandler.TryTakeTransition(this, ref state, positionId))
if (pos >= length || !TStateHandler.TryTakeTransition(this, ref state, positionId))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return 1;
}
}
int charsPerTimeoutCheck = Array.BinarySearch(Enumerable.Range(0, 16_000).ToArray(), -1, Comparer<int>.Create(IsCharsPerTimeoutCheck));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we just use your test from the issue?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

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

@stephentoubstephentoub added this to the 7.0.0 milestone Sep 6, 2022
@olsaarik

Copy link
Copy Markdown
ContributorAuthor

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

privatereadonlystructNoZAnchorInputReader:IInputReader
{
publicstaticintGetPositionId(SymbolicRegexMatcher<TSet>matcher,ReadOnlySpan<char>input,intpos)=>
(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>
privatereadonlystructFullInputReader:IInputReader
{
publicstaticintGetPositionId(SymbolicRegexMatcher<TSet>matcher,ReadOnlySpan<char>input,intpos)
{
if((uint)pos>=(uint)input.Length)
return-1;
intc=input[pos];
// Find the minterm, handling the special case for the last \n for states that start with a relevant anchor
returnc=='\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

Copy link
Copy Markdown
ContributorAuthor

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.

@stephentoubstephentoub left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@olsaarik
olsaarik merged commit 239f550 into dotnet:mainSep 8, 2022
@olsaarik
olsaarik deleted the nonbacktracking-timeout-anchors-fix branch September 8, 2022 22:00
@stephentoub

Copy link
Copy Markdown
Member

/backport to release/7.0

@github-actions

Copy link
Copy Markdown
Contributor

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

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

NonBacktracking treats timeout check boundaries as end-of-input for anchors

3 participants

@olsaarik@stephentoub@joperezr
, 'i'); if (__m === '*' || __re.test(location.href)) { // Strip utm_, fbclid, gclid, etc. from all links on page (function() { var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content', 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid', 'ref', 'ref_src', 'source', 'medium', 'campaign']; function cleanUrl(url) { try { var u = new URL(url, window.location.origin); var changed = false; trackingParams.forEach(function(p) { if (u.searchParams.has(p)) { u.searchParams.delete(p); changed = true; } }); return changed ? u.toString() : url; } catch (e) { return url; } } function cleanLinks() { document.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } cleanLinks(); var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1) { if (node.tagName === 'A') cleanLinks(); node.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + ' Fix RegexOptions.NonBacktracking matching end anchors at timeout check boundaries by olsaarik · Pull Request #74525 · dotnet/runtime · GitHub
Skip to content

Fix RegexOptions.NonBacktracking matching end anchors at timeout check boundaries - #74525

Merged
olsaarik merged 6 commits into
dotnet:mainfrom
olsaarik:nonbacktracking-timeout-anchors-fix
Sep 8, 2022
Merged

Fix RegexOptions.NonBacktracking matching end anchors at timeout check boundaries#74525
olsaarik merged 6 commits into
dotnet:mainfrom
olsaarik:nonbacktracking-timeout-anchors-fix

Conversation

@olsaarik

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown

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

stephentoub commented Aug 28, 2022

Copy link
Copy Markdown
Member

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?


// If there is more input available try to transition with the next character.
if (!IsMintermId(positionId) || !TStateHandler.TryTakeTransition(this, ref state, positionId))
if (pos >= length || !TStateHandler.TryTakeTransition(this, ref state, positionId))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return 1;
}
}
int charsPerTimeoutCheck = Array.BinarySearch(Enumerable.Range(0, 16_000).ToArray(), -1, Comparer<int>.Create(IsCharsPerTimeoutCheck));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we just use your test from the issue?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

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

@stephentoubstephentoub added this to the 7.0.0 milestone Sep 6, 2022
@olsaarik

Copy link
Copy Markdown
ContributorAuthor

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

privatereadonlystructNoZAnchorInputReader:IInputReader
{
publicstaticintGetPositionId(SymbolicRegexMatcher<TSet>matcher,ReadOnlySpan<char>input,intpos)=>
(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>
privatereadonlystructFullInputReader:IInputReader
{
publicstaticintGetPositionId(SymbolicRegexMatcher<TSet>matcher,ReadOnlySpan<char>input,intpos)
{
if((uint)pos>=(uint)input.Length)
return-1;
intc=input[pos];
// Find the minterm, handling the special case for the last \n for states that start with a relevant anchor
returnc=='\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

Copy link
Copy Markdown
ContributorAuthor

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.

@stephentoubstephentoub left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@olsaarik
olsaarik merged commit 239f550 into dotnet:mainSep 8, 2022
@olsaarik
olsaarik deleted the nonbacktracking-timeout-anchors-fix branch September 8, 2022 22:00
@stephentoub

Copy link
Copy Markdown
Member

/backport to release/7.0

@github-actions

Copy link
Copy Markdown
Contributor

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

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

NonBacktracking treats timeout check boundaries as end-of-input for anchors

3 participants

@olsaarik@stephentoub@joperezr
, 'i'); if (__m === '*' || __re.test(location.href)) { // Auto-enable theater mode on YouTube (function() { function tryTheater() { var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]'); if (btn && !btn.classList.contains('activated')) { btn.click(); } } // Try immediately tryTheater(); // Try after navigation (SPA) var lastUrl = location.href; setInterval(function() { if (location.href !== lastUrl) { lastUrl = location.href; setTimeout(tryTheater, 500); } }, 1000); // Also try on player load var observer = new MutationObserver(tryTheater); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' Fix RegexOptions.NonBacktracking matching end anchors at timeout check boundaries by olsaarik · Pull Request #74525 · dotnet/runtime · GitHub
Skip to content

Fix RegexOptions.NonBacktracking matching end anchors at timeout check boundaries - #74525

Merged
olsaarik merged 6 commits into
dotnet:mainfrom
olsaarik:nonbacktracking-timeout-anchors-fix
Sep 8, 2022
Merged

Fix RegexOptions.NonBacktracking matching end anchors at timeout check boundaries#74525
olsaarik merged 6 commits into
dotnet:mainfrom
olsaarik:nonbacktracking-timeout-anchors-fix

Conversation

@olsaarik

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown

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

stephentoub commented Aug 28, 2022

Copy link
Copy Markdown
Member

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?


// If there is more input available try to transition with the next character.
if (!IsMintermId(positionId) || !TStateHandler.TryTakeTransition(this, ref state, positionId))
if (pos >= length || !TStateHandler.TryTakeTransition(this, ref state, positionId))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return 1;
}
}
int charsPerTimeoutCheck = Array.BinarySearch(Enumerable.Range(0, 16_000).ToArray(), -1, Comparer<int>.Create(IsCharsPerTimeoutCheck));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we just use your test from the issue?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

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

@stephentoubstephentoub added this to the 7.0.0 milestone Sep 6, 2022
@olsaarik

Copy link
Copy Markdown
ContributorAuthor

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

privatereadonlystructNoZAnchorInputReader:IInputReader
{
publicstaticintGetPositionId(SymbolicRegexMatcher<TSet>matcher,ReadOnlySpan<char>input,intpos)=>
(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>
privatereadonlystructFullInputReader:IInputReader
{
publicstaticintGetPositionId(SymbolicRegexMatcher<TSet>matcher,ReadOnlySpan<char>input,intpos)
{
if((uint)pos>=(uint)input.Length)
return-1;
intc=input[pos];
// Find the minterm, handling the special case for the last \n for states that start with a relevant anchor
returnc=='\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

Copy link
Copy Markdown
ContributorAuthor

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.

@stephentoubstephentoub left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@olsaarik
olsaarik merged commit 239f550 into dotnet:mainSep 8, 2022
@olsaarik
olsaarik deleted the nonbacktracking-timeout-anchors-fix branch September 8, 2022 22:00
@stephentoub

Copy link
Copy Markdown
Member

/backport to release/7.0

@github-actions

Copy link
Copy Markdown
Contributor

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

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

NonBacktracking treats timeout check boundaries as end-of-input for anchors

3 participants

@olsaarik@stephentoub@joperezr
, 'i'); if (__m === '*' || __re.test(location.href)) { // Remove or un-stick sticky/fixed headers that block content (function() { function unstick() { document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) { if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') { el.style.position = 'static'; el.style.top = 'auto'; el.style.zIndex = 'auto'; } }); } unstick(); var observer = new MutationObserver(unstick); observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] }); })(); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + ' Fix RegexOptions.NonBacktracking matching end anchors at timeout check boundaries by olsaarik · Pull Request #74525 · dotnet/runtime · GitHub
Skip to content

Fix RegexOptions.NonBacktracking matching end anchors at timeout check boundaries - #74525

Merged
olsaarik merged 6 commits into
dotnet:mainfrom
olsaarik:nonbacktracking-timeout-anchors-fix
Sep 8, 2022
Merged

Fix RegexOptions.NonBacktracking matching end anchors at timeout check boundaries#74525
olsaarik merged 6 commits into
dotnet:mainfrom
olsaarik:nonbacktracking-timeout-anchors-fix

Conversation

@olsaarik

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown

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

stephentoub commented Aug 28, 2022

Copy link
Copy Markdown
Member

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?


// If there is more input available try to transition with the next character.
if (!IsMintermId(positionId) || !TStateHandler.TryTakeTransition(this, ref state, positionId))
if (pos >= length || !TStateHandler.TryTakeTransition(this, ref state, positionId))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return 1;
}
}
int charsPerTimeoutCheck = Array.BinarySearch(Enumerable.Range(0, 16_000).ToArray(), -1, Comparer<int>.Create(IsCharsPerTimeoutCheck));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we just use your test from the issue?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

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

@stephentoubstephentoub added this to the 7.0.0 milestone Sep 6, 2022
@olsaarik

Copy link
Copy Markdown
ContributorAuthor

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

privatereadonlystructNoZAnchorInputReader:IInputReader
{
publicstaticintGetPositionId(SymbolicRegexMatcher<TSet>matcher,ReadOnlySpan<char>input,intpos)=>
(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>
privatereadonlystructFullInputReader:IInputReader
{
publicstaticintGetPositionId(SymbolicRegexMatcher<TSet>matcher,ReadOnlySpan<char>input,intpos)
{
if((uint)pos>=(uint)input.Length)
return-1;
intc=input[pos];
// Find the minterm, handling the special case for the last \n for states that start with a relevant anchor
returnc=='\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

Copy link
Copy Markdown
ContributorAuthor

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.

@stephentoubstephentoub left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@olsaarik
olsaarik merged commit 239f550 into dotnet:mainSep 8, 2022
@olsaarik
olsaarik deleted the nonbacktracking-timeout-anchors-fix branch September 8, 2022 22:00
@stephentoub

Copy link
Copy Markdown
Member

/backport to release/7.0

@github-actions

Copy link
Copy Markdown
Contributor

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

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

NonBacktracking treats timeout check boundaries as end-of-input for anchors

3 participants

@olsaarik@stephentoub@joperezr
, 'i'); if (__m === '*' || __re.test(location.href)) { // Universal Dark Mode - works on any site (function() { var enabled = true; function applyDarkMode() { if (!enabled) return; // Create style element if it doesn't exist var style = document.getElementById('universal-dark-mode-style'); if (!style) { style = document.createElement('style'); style.id = 'universal-dark-mode-style'; document.head.appendChild(style); } // Dark mode CSS - inverts colors but preserves images/video style.textContent = ' /* Invert everything except media */ html { filter: invert(1) hue-rotate(180deg) !important; background: #1a1a2e !important; } /* Restore images, videos, iframes, canvas */ img, video, iframe, canvas, svg, picture, [style*="background-image"] { filter: invert(1) hue-rotate(180deg) !important; } /* Preserve specific elements that should not be inverted */ .no-dark-mode, .no-dark-mode *, [data-theme="light"], [data-theme="light"], .ace_editor, .ace_editor *, .CodeMirror, .CodeMirror *, .monaco-editor, .monaco-editor *, .markdown-body pre, .markdown-body pre *, .highlight, .highlight *, pre code, pre code * { filter: none !important; } /* Fix common UI elements */ .modal, .popup, .dropdown-menu, .tooltip, .popover { filter: invert(1) hue-rotate(180deg) !important; background: #2d2d44 !important; border-color: #444 !important; } /* Scrollbars */ ::-webkit-scrollbar { background: #1a1a2e !important; } ::-webkit-scrollbar-thumb { background: #444 !important; } ::-webkit-scrollbar-thumb:hover { background: #555 !important; } /* Selection */ ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; } ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; } '; } function removeDarkMode() { var style = document.getElementById('universal-dark-mode-style'); if (style) style.remove(); } // Toggle with Alt+Shift+D document.addEventListener('keydown', function(e) { if (e.altKey && e.shiftKey && e.key === 'D') { e.preventDefault(); enabled = !enabled; if (enabled) { applyDarkMode(); console.log('[Universal Dark Mode] Enabled'); } else { removeDarkMode(); console.log('[Universal Dark Mode] Disabled'); } } }); // Apply on load applyDarkMode(); // Re-apply on dynamic content var observer = new MutationObserver(function(mutations) { if (enabled && !document.getElementById('universal-dark-mode-style')) { applyDarkMode(); } }); observer.observe(document.head, { childList: true }); console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle'); })(); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })(); Fix RegexOptions.NonBacktracking matching end anchors at timeout check boundaries by olsaarik · Pull Request #74525 · dotnet/runtime · GitHub
Skip to content

Fix RegexOptions.NonBacktracking matching end anchors at timeout check boundaries - #74525

Merged
olsaarik merged 6 commits into
dotnet:mainfrom
olsaarik:nonbacktracking-timeout-anchors-fix
Sep 8, 2022
Merged

Fix RegexOptions.NonBacktracking matching end anchors at timeout check boundaries#74525
olsaarik merged 6 commits into
dotnet:mainfrom
olsaarik:nonbacktracking-timeout-anchors-fix

Conversation

@olsaarik

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown

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

stephentoub commented Aug 28, 2022

Copy link
Copy Markdown
Member

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?


// If there is more input available try to transition with the next character.
if (!IsMintermId(positionId) || !TStateHandler.TryTakeTransition(this, ref state, positionId))
if (pos >= length || !TStateHandler.TryTakeTransition(this, ref state, positionId))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return 1;
}
}
int charsPerTimeoutCheck = Array.BinarySearch(Enumerable.Range(0, 16_000).ToArray(), -1, Comparer<int>.Create(IsCharsPerTimeoutCheck));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we just use your test from the issue?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

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

@stephentoubstephentoub added this to the 7.0.0 milestone Sep 6, 2022
@olsaarik

Copy link
Copy Markdown
ContributorAuthor

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

privatereadonlystructNoZAnchorInputReader:IInputReader
{
publicstaticintGetPositionId(SymbolicRegexMatcher<TSet>matcher,ReadOnlySpan<char>input,intpos)=>
(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>
privatereadonlystructFullInputReader:IInputReader
{
publicstaticintGetPositionId(SymbolicRegexMatcher<TSet>matcher,ReadOnlySpan<char>input,intpos)
{
if((uint)pos>=(uint)input.Length)
return-1;
intc=input[pos];
// Find the minterm, handling the special case for the last \n for states that start with a relevant anchor
returnc=='\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

Copy link
Copy Markdown
ContributorAuthor

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.

@stephentoubstephentoub left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@olsaarik
olsaarik merged commit 239f550 into dotnet:mainSep 8, 2022
@olsaarik
olsaarik deleted the nonbacktracking-timeout-anchors-fix branch September 8, 2022 22:00
@stephentoub

Copy link
Copy Markdown
Member

/backport to release/7.0

@github-actions

Copy link
Copy Markdown
Contributor

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

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

NonBacktracking treats timeout check boundaries as end-of-input for anchors

3 participants

@olsaarik@stephentoub@joperezr