8000 Remove Uri length limits by MihaZupan · Pull Request #117287 · dotnet/runtime · GitHub
[go: up one dir, main page]

Skip to content

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Conversation

MihaZupan
Copy link
Member
@MihaZupan MihaZupan commented Jul 3, 2025

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:

  • Removed explicit length checks
  • Changed UriInfo.Offset fields from ushort to int and removed any remaining casts to ushort left after Replace ushort usage with int #32694
  • Shuffled some of the internal Uri.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

@MihaZupan MihaZupan added this to the 10.0.0 milestone Jul 3, 2025
@MihaZupan MihaZupan self-assigned this Jul 3, 2025
@MihaZupan
Copy link
Member Author

@MihuBot benchmark Perf_Uri -medium

@MihuBot
Copy link
MihuBot commented Jul 3, 2025
System.Tests.Perf_Uri
BenchmarkDotNet v0.14.1-nightly.20250107.205, Linux Ubuntu 22.04.5 LTS (Jammy Jellyfish)
AMD EPYC 9V74, 1 CPU, 8 logical and 4 physical cores
MediumRun : .NET 10.0.0 (42.42.42.42424), X64 RyuJIT AVX-512F+CD+BW+DQ+VL+VBMI
Job=MediumRun  IterationCount=15  LaunchCount=2
WarmupCount=10
Method Toolchain input Mean Error Ratio Allocated Alloc Ratio
ParseAbsoluteUri Main ? 166.1378 ns 0.2119 ns 1.00 288 B 1.00
ParseAbsoluteUri PR ? 169.8616 ns 3.6570 ns 1.02 304 B 1.06
DnsSafeHost Main ? 139.7371 ns 0.1848 ns 1.00 216 B 1.00
DnsSafeHost PR ? 139.7220 ns 0.5842 ns 1.00 232 B 1.07
BuilderToString Main ? 71.4366 ns 0.1590 ns 1.00 216 B 1.00
BuilderToString PR ? 70.7919 ns 0.1016 ns 0.99 216 B 1.00
UriBuilderReplacePort Main ? 65.4056 ns 0.5989 ns 1.00 216 B 1.00
UriBuilderReplacePort PR ? 64.4720 ns 0.6052 ns 0.99 216 B 1.00
GetComponents Main ? 10.8175 ns 0.2389 ns 1.00 80 B 1.00
GetComponents PR ? 10.4836 ns 0.1327 ns 0.97 80 B 1.00
PathAndQuery Main ? 0.7822 ns 0.0788 ns 1.02 - NA
PathAndQuery PR ? 0.9000 ns 0.0649 ns 1.18 - NA
EscapeDataString Main {{{{{{{{{{{{(...){{{{{{{{{{{{ [1000] 4,022.0624 ns 8.3071 ns 1.00 6024 B 1.00
EscapeDataString PR {{{{{{{{{{{{(...){{{{{{{{{{{{ [1000] 4,011.5934 ns 12.1372 ns 1.00 6024 B 1.00
CombineAbsoluteRelative Main /new/path 93.7111 ns 0.0817 ns 1.00 200 B 1.00
CombineAbsoluteRelative PR /new/path 94.1515 ns 0.6260 ns 1.00 200 B 1.00
UnescapeDataString Main %E4%BD%A0%E5%A5%BD 35.8653 ns 0.0719 ns 1.00 32 B 1.00
UnescapeDataString PR %E4%BD%A0%E5%A5%BD 36.0837 ns 0.0941 ns 1.01 32 B 1.00
EscapeDataString Main a{üa{üa{üa{ü(...)a{üa{üa{üa{ü [999] 6,890.9262 ns 46.9684 ns 1.00 6688 B 1.00
EscapeDataString PR a{üa{üa{üa{ü(...)a{üa{üa{üa{ü [999] 6,857.4003 ns 24.8218 ns 1.00 6688 B 1.00
EscapeDataString Main aaaaaaaaaaaa(...)aaaaaaaaaaaa [1000] 26.3397 ns 0.0200 ns 1.00 - NA
EscapeDataString PR aaaaaaaaaaaa(...)aaaaaaaaaaaa [1000] 26.5843 ns 0.0317 ns 1.01 - NA
UnescapeDataString Main abc%20def%20ghi%20 30.1199 ns 0.1540 ns 1.00 48 B 1.00
UnescapeDataString PR abc%20def%20ghi%20 29.7204 ns 0.2563 ns 0.99 48 B 1.00
Ctor Main http://dot.net 49.6887 ns 0.4095 ns 1.00 56 B 1.00
Ctor PR http://dot.net 49.5710 ns 0.0958 ns 1.00 56 B 1.00
CtorIdnHostPathAndQuery Main http://dot.ne(...)alue#fragment [43] 180.8881 ns 0.4491 ns 1.00 240 B 1.00
CtorIdnHostPathAndQuery PR http://dot.ne(...)alue#fragment [43] 183.1507 ns 0.6040 ns 1.01 256 B 1.07
Ctor Main http://höst.with.ünicode 227.5639 ns 2.9140 ns 1.00 304 B 1.00
Ctor PR http://höst.with.ünicode 238.1650 ns 0.3838 ns 1.05 320 B 1.05
CtorIdnHostPathAndQuery Main http://höst.w(...)alue#fragment [53] 1,065.3126 ns 6.0198 ns 1.00 984 B 1.00
CtorIdnHostPathAndQuery PR http://höst.w(...)alue#fragment [53] 1,106.9752 ns 7.1210 ns 1.04 1000 B 1.02
CtorIdnHostPathAndQuery Main http://host/ 110.2350 ns 3.1492 ns 1.00 192 B 1.00
CtorIdnHostPathAndQuery PR http://host/ 107.7886 ns 0.1479 ns 0.98 208 B 1.08
CtorIdnHostPathAndQuery Main http://host/p(...)s?key=ünicode [50] 471.4345 ns 4.2401 ns 1.00 776 B 1.00
CtorIdnHostPathAndQuery PR http://host/p(...)s?key=ünicode [50] 462.1865 ns 2.4785 ns 0.98 792 B 1.02
CtorIdnHostPathAndQuery Main http://host/p(...)es?key=va lue [49] 285.6154 ns 4.4001 ns 1.00 288 B 1.00
CtorIdnHostPathAndQuery PR http://host/p(...)es?key=va lue [49] 279.5793 ns 5.0009 ns 0.98 304 B 1.06
CtorIdnHostPathAndQuery Main http://host/p(...)3&key4=value4 [64] 235.1861 ns 2.2816 ns 1.00 296 B 1.00
CtorIdnHostPathAndQuery PR http://host/p(...)3&key4=value4 [64] 227.2213 ns 1.0872 ns 0.97 312 B 1.05
CtorIdnHostPathAndQuery Main http://host/p(...)=%C3%BCnicode [61] 510.0278 ns 1.2008 ns 1.00 776 B 1.00
CtorIdnHostPathAndQuery PR http://host/p(...)=%C3%BCnicode [61] 545.6278 ns 1.0666 ns 1.07 792 B 1.02
CtorIdnHostPathAndQuery Main http://host/p(...)?key=va%20lue [57] 232.1828 ns 0.4585 ns 1.00 288 B 1.00
CtorIdnHostPathAndQuery PR http://host/p(...)?key=va%20lue [57] 234.8167 ns 3.8520 ns 1.01 304 B 1.06
Ctor Main http://xn--hs(...)n--nicode-2ya [38] 74.5342 ns 1.0694 ns 1.00 56 B 1.00
Ctor PR http://xn--hs(...)n--nicode-2ya [38] 74.6229 ns 1.1247 ns 1.00 56 B 1.00
CtorIdnHostPathAndQuery Main http://xn--hs(...)alue#fragment [67] 232.7291 ns 1.4535 ns 1.00 288 B 1.00
CtorIdnHostPathAndQuery PR http://xn--hs(...)alue#fragment [67] 231.5637 ns 0.1099 ns 1.00 304 B 1.06
Ctor Main https://a.much.longer.domain.name 85.6344 ns 0.7703 ns 1.00 56 B 1.00
Ctor PR https://a.much.longer.domain.name 87.5019 ns 0.5504 ns 1.02 56 B 1.00
CtorIdnHostPathAndQuery Main https://a.muc(...)alue#fragment [62] 237.6351 ns 1.2594 ns 1.00 272 B 1.00
CtorIdnHostPathAndQuery PR https://a.muc(...)alue#fragment [62] 240.2293 ns 3.0482 ns 1.01 288 B 1.06
Ctor Main https://contoso.com 58.1769 ns 6.6203 ns 1.02 56 B 1.00
Ctor PR https://contoso.com 49.8450 ns 0.2761 ns 0.88 56 B 1.00
Ctor Main https://CONTOSO.com 49.7327 ns 0.0730 ns 1.00 56 B 1.00
Ctor PR https://CONTOSO.com 50.0304 ns 0.1181 ns 1.01 56 B 1.00
CtorIdnHostPathAndQuery Main https://conto(...)alue#fragment [48] 186.2589 ns 1.7418 ns 1.00 248 B 1.00
CtorIdnHostPathAndQuery PR https://conto(...)alue#fragment [48] 191.0147 ns 2.6363 ns 1.03 264 B 1.06
CtorIdnHostPathAndQuery Main https://CONTO(...)alue#fragment [48] 185.6292 ns 1.2705 ns 1.00 248 B 1.00
CtorIdnHostPathAndQuery PR https://CONTO(...)alue#fragment [48] 190.0354 ns 2.8241 ns 1.02 264 B 1.06
EscapeDataString Main üüüüüüüüüüüü(...)üüüüüüüüüüüü [1000] 8,758.8392 ns 12.0933 ns 1.00 12024 B 1.00
EscapeDataString PR üüüüüüüüüüüü(...)üüüüüüüüüüüü [1000] 8,793.5775 ns 19.3634 ns 1.00 12024 B 1.00

@MihaZupan MihaZupan marked this pull request as ready for review July 8, 2025 03:31
@Copilot Copilot AI review requested due to automatic review settings July 8, 2025 03:31
Copy link
Contributor
@Copilot Copilot AI left a 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 use int instead of ushort.
  • Refactored or added tests across System.Runtime and System.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

@MihaZupan
Copy link
Member Author

One issue I see with this is our path compression (e.g. removing /../) algorithm being O(n^2) right now.
We should rewrite that into something linear before .NET 10 ships (or we could choose to block this PR until we do so).

private static int Compress(Span<char> span, UriParser syntax)

@MihaZupan MihaZupan requested a review from a team July 8, 2025 12:56
Copy link
Member
@rokonec rokonec left a 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));
Copy link
Member

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

Copy link
Member Author

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

return Headers.Where(h => h.Name.Equals(headerName, StringComparison.OrdinalIgnoreCase))
.Select(h => h.Value)
.ToArray();

BadScheme,
BadAuthority,
EmptyUriString,
LastRelativeUriOkErrIndex,
Copy link
Member
@rokonec rokonec Jul 8, 2025

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

Copy link
Member Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-System.Net breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove or relax the length limitation of Uri
3 participants
0