-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Check source and destination spans overlap in TryNormalize method #118047
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Check source and destination spans overlap in TryNormalize method #118047
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds validation to prevent undefined behavior in the TryNormalize
method by detecting and rejecting overlapping source and destination spans. The fix addresses a potential issue where overlapping spans could cause unpredictable results due to the underlying OS normalization components.
Key changes:
- Added overlap detection in the
TryNormalize
method with appropriate exception throwing - Introduced a new error message resource for the overlap validation
- Added comprehensive test cases to verify the overlap detection works correctly
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
File | Description |
---|---|
src/libraries/System.Private.CoreLib/src/System/Globalization/Normalization.cs | Added overlap validation check that throws ArgumentException when source and destination spans overlap |
src/libraries/System.Private.CoreLib/src/Resources/Strings.resx | Added new error message resource for overlapping spans validation |
src/libraries/System.Runtime/tests/System.Globalization.Extensions.Tests/Normalization/StringNormalizationTests.cs | Added test cases to verify overlap detection throws appropriate exceptions |
Comments suppressed due to low confidence (1)
src/libraries/System.Runtime/tests/System.Globalization.Extensions.Tests/Normalization/StringNormalizationTests.cs:133
- This test case doesn't actually test overlapping spans. The source span starts at index 0 with length 5, while the destination span starts at index 4 with length 1. These spans overlap by only 1 character at index 4, but this is a very minimal overlap case. Consider adding a test case where the spans have more significant overlap to ensure the validation works correctly in various overlap scenarios.
Assert.Throws<ArgumentException>("destination", () => overlappingDestination.AsSpan().TryNormalize(overlappingDestination.AsSpan(4), out int charsWritten, NormalizationForm.FormC));
Tagging subscribers to this area: @dotnet/area-system-globalization |
src/libraries/System.Private.CoreLib/src/System/Globalization/Normalization.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/Resources/Strings.resx
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Globalization/Normalization.cs
Show resolved
Hide resolved
…com/tarekgh/runtime into CheckSpansOverlapInsideTryNormalize
...untime/tests/System.Globalization.Extensions.Tests/Normalization/StringNormalizationTests.cs
Outdated
Show resolved
Hide resolved
…ions.Tests/Normalization/StringNormalizationTests.cs Co-authored-by: Stephen Toub <[email protected]>
Fixes #118017
In .NET we have introduced the extension method
runtime/src/libraries/System.Private.CoreLib/src/System/StringNormalizationExtensions.cs
Line 80 in 1e860a4
If the user provides overlapping source and destination spans, it leads to undefined behavior, since we rely on underlying OS components for normalization. The fix is straightforward: disallow overlapping spans and throw an exception. This API is new and hasn’t shipped in any released version yet, so there are no compatibility concerns at this point—we’re addressing it now to prevent future issues.