Check regex timeout in loops and repetitions #38091
Conversation
ViktorHofer
force-pushed
the
CVE-2019-0820
branch
from
10477fd
to
2fc8bf9
Compare
|
CI isn't triggering... |
Check the regex timeout in SetLoop and SetRepetition opcodes to avoid the timeout not being handled.
ViktorHofer
force-pushed
the
CVE-2019-0820
branch
from
2fc8bf9
to
98d4df2
Compare
src/System.Text.RegularExpressions/src/System/Text/RegularExpressions/RegexInterpreter.cs
Outdated
Show resolved
Hide resolved
src/System.Text.RegularExpressions/src/System/Text/RegularExpressions/RegexInterpreter.cs
Outdated
Show resolved
Hide resolved
| // Emit code to check the timeout every 2000th-iteration. | ||
| Ldloc(_loopV); | ||
| Ldc(LoopTimeoutCheckCount); | ||
| Rem(); |
There was a problem hiding this comment.
See comment on the non-emit version.
There was a problem hiding this comment.
This comment wasn't address. Fine for this PR, but consider addressing it subsequently.
| var regex = new Regex(@"a\s+", options, TimeSpan.FromSeconds(1)); | ||
| string input = @"a" + new string(' ', 800_000_000) + @"b"; | ||
|
|
||
| Assert.Throws<RegexMatchTimeoutException>(() => regex.Match(input)); |
There was a problem hiding this comment.
Should we validate that the exception is thrown within some window of time?
There was a problem hiding this comment.
It would have to have significant leeway (eg., within 2x or 3x) to account for vagaries of CI machines.
There was a problem hiding this comment.
Oh, I see it's a second. Maybe within 30 sec?
There was a problem hiding this comment.
Assuming 800M will take much longer.
There was a problem hiding this comment.
30 seconds, 1 minute, whatever we think is reasonable. My goal would just be that the test doesn't pass after the regex runs for an hour and then upon completion sees there was a timeout requested and throws :)
src/System.Text.RegularExpressions/src/System/Text/RegularExpressions/RegexInterpreter.cs
Outdated
Show resolved
Hide resolved
|
The added test cases are OOM-ing on Windows:
@ViktorHofer will you be able to resolve this by noon PST (~2 hours)? It may be alright if we need to push this out until preview7. Alternatively, given that the Core stack is branching 3 days before the rest of the stack, it may be that we can sneak this in over the weekend if it's critical enough (and if @mmitche, @danmosemsft, @leecow give it the green light) |
|
@wtgodbe feel free to merge this as soon as CI is "green". @stpehentoub @danmosemsft I addressed most feedback except the timeout which I will do in a follow-up PR to not block the branch-off. |
|
I do not think it is critical to get into Preview 6. It is not go live. |
|
Hello @stephentoub! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
|
CI is actually done here, but the reporting isn't showing it. Merging. |
* Check regex timeout in loops and repetitions Check the regex timeout in SetLoop and SetRepetition opcodes to avoid the timeout not being handled. * PR feedback and fix OOM Commit migrated from dotnet/corefx@40f031a
This is a cherry pick of the commit from .NET Core 2.2, resolving the issue disclosed in CVE-2019-0820.
Check the regex timeout in SetLoop and SetRepetition opcodes to avoid
the timeout not being handled.
@wtgodbe this needs to be in before branching of, the change was reviewed and approved in the servicing branches already, this is a simple cherry-pick. Therefore I'm merging this and if you notice any issues I'm happy to revert the change.
cc @danmosemsft