Fix inadvertently case sensitive Boyer-Moore #39420
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label. |
|
We need to get this into Preview 8. |
|
Separate from this PR, it would probably be good to add a test that searches for a random string against a text that may or may not contain it somewhere, and compare compiled with non compiled. Such a test could quickly have found this bug and might protect us against others. The comparison with non compiled is interesting because the implementation is so different. |
This was referenced Jul 16, 2020
stephentoub
approved these changes
Jul 16, 2020
...libraries/System.Text.RegularExpressions/src/System/Text/RegularExpressions/RegexCompiler.cs
Outdated
Show resolved
Hide resolved
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix #39390
In this case the pattern "H#" would not match "#H#" iff RegexOptions.IgnoreCase | RegexOptions.Compiled.
Because the pattern contains a literal prefix (indeed it is the entire pattern) we will use Boyer-Moore to find the first instance of it. (One could imagine a more efficient way to search for a 2-character prefix.) Because the IgnoreCase was passed, we lowercase the pattern immediately to "h#", and when we match against a character in the text, we must lower case that character to compare it.
As a performance optimization, in the Compiled path, we avoid calling ToLower on the text candidate if we can cheaply verify that the character we are searching for is not be affected by case conversion. In this case, for example, we need not bother to lower case the text candidate character when we are searching for "#" because it is in a UnicodeCategory ("OtherPunctuation") which we know is not affected by case conversion. This optimization, like many others, does not exist in the non Compiled path.
The bug was that when deciding whether to lowercase the text candidate, instead of examining the character we were searching for, we were examining the last character of the prefix instead. In this repro case that is "#" so when searching for "H" we would not lower case it.
I added a test that fails without this fix.