Prevent IndexOutOfRangeException in RegexInterpreter #97916
Conversation
This update fixes the IndexOutOfRangeException in RegexInterpreter by enhancing the `TrackPush` and `TrackPush2` methods. The adjustment involves checking the runtrack position before decrementing it, ensuring that it doesn't become negative, which was the root cause of the exception. This prevents potential out-of-range errors when handling large numbers of repetitions in regular expressions. Fix dotnet#62094
ghost
added
the
community-contribution
|
Tagging subscribers to this area: @dotnet/area-system-text-regularexpressions Issue DetailsThis update fixes the IndexOutOfRangeException in RegexInterpreter by enhancing the Fix #62094
|
|
Thank you, but I'm not convinced this is the right fix. Why are the numbers becoming negative in the first place? That would seem to suggest a logic error in how the stack is balanced. Seems like this is addressing a symptom rather than the cause. |
|
Thanks for your reply. |
steveharter
added
the
needs-author-action
|
This pull request has been automatically marked |
If EnsureStorage() is called unconditionally, the array will be expanded, so the position will never become negative. When the conditions inside EnsureStorage() are true, it might be necessary to expand the array, regardless of the comparison between newpos and codepos. https://github.com/dotnet/runtime/blob/6ebc8bd86dbc780b2a2a7daf3ab6020f9104f09e/src/libraries/System.Text.RegularExpressions/tests/FunctionalTests/Regex.MultipleMatches.Tests.cs#L461-L469 Before the change, in this test case, EnsureStorage() is not called because newpos == codepos == 6 from the first time until an exception occurs. Fix dotnet#62049
dotnet-policy-service
bot
removed
needs-author-action
|
I made changes because it seemed good for RegexInterpreter's Goto() and Backtrack() to call EnsureStorage() unconditionally. If EnsureStorage() is called unconditionally, the array will be extended, so it seems that the array's position will never become negative. I think there might be a need to extend when the condition inside EnsureStorage() is true. In the case of this test case, EnsureStorage() is not called since newpos == codepos == 6 is always true from the first instance until an exception occurs. |
Thanks. Are you able to measure what impact that has on perf? Hopefully the call would be inlined, and we should end up with effectivlely two comparisons instead of one and it shouldn't be measurable. As long as perf looks good, this general form of fix makes sense. |
|
@stephentoub
I'm sorry. I don't understand how to do it. runtime/docs/workflow/README.md Lines 89 to 92 in 7740dbd |
The common ways to verify perf:
|
|
@stephentoub // * Summary * BenchmarkDotNet v0.13.13-nightly.20240311.145, Windows 11 (10.0.22621.3374/22H2/2022Update/SunValley2) PowerPlanMode=00000000-0000-0000-0000-000000000000 IterationTime=250ms MaxIterationCount=20
// * Hints * // * Legends * // * Diagnostic Output - MemoryDiagnoser * // ***** BenchmarkRunner: End ***** Global total time: 01:05:22 (3922.2 sec), executed benchmarks: 558 |
|
This looks good. Thanks for the fix! |
* Prevent IndexOutOfRangeException in RegexInterpreter This update fixes the IndexOutOfRangeException in RegexInterpreter by enhancing the `TrackPush` and `TrackPush2` methods. The adjustment involves checking the runtrack position before decrementing it, ensuring that it doesn't become negative, which was the root cause of the exception. This prevents potential out-of-range errors when handling large numbers of repetitions in regular expressions. Fix dotnet#62094 * Changed to call EnsureStorage() unconditionally. If EnsureStorage() is called unconditionally, the array will be expanded, so the position will never become negative. When the conditions inside EnsureStorage() are true, it might be necessary to expand the array, regardless of the comparison between newpos and codepos. https://github.com/dotnet/runtime/blob/6ebc8bd86dbc780b2a2a7daf3ab6020f9104f09e/src/libraries/System.Text.RegularExpressions/tests/FunctionalTests/Regex.MultipleMatches.Tests.cs#L461-L469 Before the change, in this test case, EnsureStorage() is not called because newpos == codepos == 6 from the first time until an exception occurs. Fix dotnet#62049
This update fixes the IndexOutOfRangeException in RegexInterpreter by enhancing the
TrackPushandTrackPush2methods. The adjustment involves checking the runtrack position before decrementing it, ensuring that it doesn't become negative, which was the root cause of the exception. This prevents potential out-of-range errors when handling large numbers of repetitions in regular expressions.Fix #62094