-
Notifications
You must be signed in to change notification settings - Fork 5.1k
Remove Uri length limits #117287
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?
Remove Uri length limits #117287
Conversation
@MihuBot benchmark Perf_Uri -medium |
System.Tests.Perf_Uri
|
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 the hard limits on URI component lengths by replacing ushort
‐based offsets with int
, dropping c_MaxUriBufferSize
checks, and expanding many tests to validate behavior with much larger inputs.
- Removed all size‐limit checks and related error cases from
System.Private.Uri
internals; updated offsets and flag definitions to useint
instead ofushort
. - Refactored or added tests across
System.Runtime
andSystem.Private.Uri
to exercise URIs, whitespace handling, escaping, and headers at sizes up to 1 000 000 characters. - Fixed a bug in integer decoding in
QPackTestDecoder
by masking off the continuation bit, and added a new HTTP test for large URIs and headers.
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
Uri.MethodsTests.cs | Replaced 64 KB+ test limit with 100 000 |
Uri.CreateUriTests.cs | Added test data with 100 000 spaces prepended to URI strings |
Uri.CreateStringTests.cs | Refactored Path/Query/Fragment tests to include very long inputs |
UriExt.cs / Uri.cs / UriEnumTypes.cs / IriHelper.cs | Dropped size checks, switched offsets to int , updated flags, added debug invariants |
UriTests.cs (FunctionalTests) | Added manual OOM test for extremely long inputs |
UriEscapingTest.cs & IriTest.cs | Updated long‐input data arrays to 64 000/66 000/1 000 000 scenarios |
QPackTestDecoder.cs | Corrected integer decoding to mask out the high bit |
HttpClientHandlerTest.cs | Added test validating large URI paths and header names/values |
src/libraries/Common/tests/System/Net/Http/HttpClientHandlerTest.cs
Outdated
Show resolved
Hide resolved
One issue I see with this is our path compression (e.g. removing
|
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.
Please review that enum LastRelativeUriOkErrIndex
HttpRequestData requestData = await server.HandleRequestAsync(); | ||
|
||
Assert.Equal(longPath, requestData.Path); | ||
Assert.Equal(longHeaderValue, requestData.GetSingleHeaderValue(longHeaderName)); |
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.
NIT: in theory if there would be header name truncated in both set and get headed it would not detect this bug
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.
The GetSingleHeaderValue
is effectively a dictionary lookup, so I'm not sure it's a real concern
runtime/src/libraries/Common/tests/System/Net/Http/GenericLoopbackServer.cs
Lines 283 to 285 in 6d869a8
return Headers.Where(h => h.Name.Equals(headerName, StringComparison.OrdinalIgnoreCase)) | |
.Select(h => h.Value) | |
.ToArray(); |
BadScheme, | ||
BadAuthority, | ||
EmptyUriString, | ||
LastRelativeUriOkErrIndex, |
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.
It is equal 5 now, but it was 4. From naming it looks wrong to me. Is it as intended?
Update: I looked at the current usages and it is not a bug, only the name of enum LastRelativeUriOkErrIndex
is now little bit misleading as its current meaning is EndOfRelativeUriOkErrIndex
or something. Consider this comment as NIT please
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.
The important thing is the position of LastRelative
relative to other values.
All the checks are err <= ParsingError.LastRelativeUriOkErrIndex
or err > ParsingError.LastRelativeUriOkErrIndex
, so it doesn't matter if it's the same or higher than EmptyUriString
.
I agree the name is odd, I can drop the -Index
suffix.
Closes #96544
The current approach grows the
UriInfo
allocation by 16 bytes.If we want to keep the 64 KB limit on the UserInfo/Host, we can rather easily cut that to just 8.
Gist:
UriInfo.Offset
fields fromushort
toint
and removed any remaining casts toushort
left after Replace ushort usage with int #32694Uri.Flags
enum values around to make space for 32 bits (instead of 16) at the start since we store the index inside the flags before doing the full parse