-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Remove unsafe code from Uri PrivateParseMinimal, CheckAuthorityHelper, and related helpers #121671
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
base: main
Are you sure you want to change the base?
Conversation
…, and related helpers
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 removes unsafe code from URI parsing methods by converting pointer-based implementations to use ReadOnlySpan<char>. The changes eliminate the use of fixed statements and pointer arithmetic while preserving the original parsing logic.
Key Changes:
- Converted
IriHelper.EscapeUnescapeIrifrom unsafe pointer-based to safe span-based implementation - Converted
UncNameHelper.IsValidto acceptReadOnlySpan<char>instead ofchar*pointer - Converted
Uri.PrivateParseMinimal,CheckAuthorityHelper, andGetHostViaCustomSyntaxto use spans instead of pointers
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/libraries/System.Private.Uri/tests/UnitTests/IriEscapeUnescapeTest.cs | Simplified tests by removing unsafe code and HeapCheck buffer validation class, now calling IriHelper.EscapeUnescapeIri directly |
| src/libraries/System.Private.Uri/src/System/UriExt.cs | Updated EscapeUnescapeIri wrapper to use span-based API instead of unsafe pointer-based call |
| src/libraries/System.Private.Uri/src/System/IriHelper.cs | Converted EscapeUnescapeIri from unsafe pointer arithmetic to safe span indexing while preserving all parsing logic |
| src/libraries/System.Private.Uri/src/System/UncNameHelper.cs | Converted IsValid from unsafe pointer-based to span-based, using uint cast for bounds checking and returning output parameter for name length |
| src/libraries/System.Private.Uri/src/System/Uri.cs | Removed unsafe code from PrivateParseMinimal, CheckAuthorityHelper, and GetHostViaCustomSyntax by converting to span-based parsing while maintaining all parsing behavior and edge cases |
| if (ch == '%') | ||
| { | ||
| if (end - i > 2) | ||
| if ((uint)(i + 2) < (uint)span.Length) |
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.
| if ((uint)(i + 2) < (uint)span.Length) | |
| if ((uint)i < (uint)(span.Length - 2)) |
To avoid re-computing i + 2 on every iteration. But does this also elide the bound check?
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.
But does this also elide the bound check?
It does not with the proposed change.
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.
@EgorBo can the JIT learn to elide the bound checks here (or in M2 in this demo)?
Removes unsafe code from
PrivateParseMinimal,EscapeUnescapeIri,UncNameHelper.IsValid,GetHostViaCustomSyntax,CheckAuthorityHelper.