-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Remove unsafe code from the Uri.UnescapeString helper #121261
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
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 refactors the URI unescaping logic by:
- Introducing a simpler
Unescapemethod that unconditionally unescapes percent-encoded sequences - Removing the
UnescapeAllflag fromUnescapeModeenum - Renaming
CopyOnlytoNoneinUnescapeModeenum - Removing unused
unsafecode and theSystem.Runtime.InteropServicesimport - Consolidating various unescape code paths to use the new
Unescapemethod where full unescaping is desired
Key changes:
- New
UriHelper.Unescape()method provides simple unconditional unescaping - Callsites that previously used
UnescapeMode.Unescape | UnescapeMode.UnescapeAllnow use the simplerUnescape()method - The more complex
UnescapeString()method is retained for cases requiring conditional escaping/unescaping with reserved character handling
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 |
|---|---|
| UriHelper.cs | Adds new Unescape() method, refactors UnescapeString() to use span-based operations instead of pointers, removes unsafe code |
| UriExt.cs | Updates UnescapeDataString and TryUnescapeDataString to use new Unescape() method |
| UriEnumTypes.cs | Renames CopyOnly to None, removes UnescapeAll flag |
| Uri.cs | Updates multiple callsites to use new Unescape() method or simplified logic, removes pointer-based operations |
| PercentEncodingHelper.cs | Adds scoped modifier to parameter, minor cleanup |
Comments suppressed due to low confidence (1)
src/libraries/System.Private.Uri/src/System/Uri.cs:1081
- Potential index out of bounds exception for UNC paths. This code assumes index 1 contains a drive letter separator, but for UNC paths,
result[1]would be the second backslash character. This check should be conditional onIsDosPathor include bounds checking.
if (result[1] == '|')
result[1] = ':';
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.
Nice!
|
/ba-g android timing out |
Closes #31173
We have an
UnescapeStringhelper that escapes/unescapes input based on various flags (copy input as-is, unescape only, escape reserved chars, or both).This PR removes the use of unsafe code from this helper.
I also removed the "copy input as-is" and "unescape only" modes from this helper. For copy-only we just forward to
CopyTo/Appenddirectly, whereas "unescape only" gets a dedicated helper since it's a reasonably hot path (UnescapeDataStringcalls into this).Initial benchmarks look good (no meaningful difference): MihuBot/runtime-utils#1612
Alternating escaped/unescaped
Only unescaped
Escaped at start followed by unescaped