Skip to content

Reject CR/LF's in Kestrel header decoding #34441

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.

Sign up for GitHub

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

Jump to bottom
Merged
wtgodbe merged 8 commits into from
Jul 28, 2021
Merged

Reject CR/LF's in Kestrel header decoding #34441

wtgodbe merged 8 commits into from
Jul 28, 2021

Conversation

@wtgodbe
Copy link
Member

@wtgodbe wtgodbe commented Jul 16, 2021

Fixes #33522 by rejecting CR/LF's in headers in Kestrel. Benchmark data before/after:

Before:

Method Type Mean Error StdDev Op/s Ratio RatioSD Gen 0 Gen 1 Gen 2 Allocated
AsciiBytesToString KeepAlive 16.76 ns 0.278 ns 0.260 ns 59,660,899.4 1.00 0.00 0.0006 - - 48 B
Utf8BytesToString KeepAlive 67.72 ns 0.403 ns 0.357 ns 14,766,770.9 4.04 0.07 0.0013 - - 104 B
AsciiBytesToString Accept 26.11 ns 0.491 ns 0.459 ns 38,296,680.3 1.00 0.00 0.0025 - - 200 B
Utf8BytesToString Accept 153.32 ns 2.618 ns 2.449 ns 6,522,365.0 5.87 0.14 0.0050 - - 408 B
AsciiBytesToString UserAgent 30.06 ns 0.521 ns 0.462 ns 33,262,624.7 1.00 0.00 0.0030 - - 240 B
Utf8BytesToString UserAgent 166.91 ns 0.801 ns 0.749 ns 5,991,283.3 5.55 0.10 0.0061 - - 504 B
AsciiBytesToString Cookie 50.06 ns 0.588 ns 0.491 ns 19,975,125.6 1.00 0.00 0.0068 - - 544 B
Utf8BytesToString Cookie 372.37 ns 1.479 ns 1.384 ns 2,685,524.5 7.44 0.09 0.0140 - - 1,144 B

After:

Method Type Mean Error StdDev Op/s Ratio RatioSD Gen 0 Gen 1 Gen 2 Allocated
AsciiBytesToString KeepAlive 15.56 ns 0.300 ns 0.357 ns 64,271,699.2 1.00 0.00 0.0006 - - 48 B
Utf8BytesToString KeepAlive 74.81 ns 1.029 ns 0.859 ns 13,366,906.2 4.76 0.14 0.0013 - - 104 B
AsciiBytesToString Accept 25.87 ns 0.460 ns 0.430 ns 38,648,372.4 1.00 0.00 0.0025 - - 200 B
Utf8BytesToString Accept 151.04 ns 0.674 ns 0.563 ns 6,620,945.7 5.84 0.10 0.0050 - - 408 B
AsciiBytesToString UserAgent 29.25 ns 0.377 ns 0.352 ns 34,187,918.0 1.00 0.00 0.0030 - - 240 B
Utf8BytesToString UserAgent 172.23 ns 0.881 ns 0.735 ns 5,806,140.1 5.88 0.08 0.0061 - - 504 B
AsciiBytesToString Cookie 49.61 ns 0.908 ns 1.081 ns 20,155,682.8 1.00 0.00 0.0068 - - 544 B
Utf8BytesToString Cookie 363.82 ns 3.277 ns 2.905 ns 2,748,632.6 7.33 0.15 0.0140 - - 1,144 B

The biggest loss was 10.4% in Utf8BytesToString KeepAlive, other benchmarks were between a 2% gain and a 3% loss.

Also fixes #34402

@wtgodbe wtgodbe requested review from JamesNK and Tratcher July 16, 2021 22:36
@ghost ghost added the area-runtime label Jul 16, 2021
@wtgodbe wtgodbe changed the title Wtgodbe/crlf Reject CR/LF's in Kestrel header decoding Jul 16, 2021
Tratcher
Tratcher reviewed Jul 16, 2021
View reviewed changes
@davidfowl
Copy link
Member

davidfowl commented Jul 17, 2021

cc @benaadams @gfoidl

gfoidl
gfoidl reviewed Jul 18, 2021
View reviewed changes
@wtgodbe
Copy link
Member Author

wtgodbe commented Jul 23, 2021

@halter73 @Tratcher could you take a look at the latest commit when you get a chance?

Tratcher
Tratcher approved these changes Jul 23, 2021
View reviewed changes
Copy link
Member

@Tratcher Tratcher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gfoidl's method splitting suggestion is worth trying.

@wtgodbe
Copy link
Member Author

wtgodbe commented Jul 27, 2021

After the most recent change, there's some marginal improvement across the board from the previous BytesToStringBenchmark run, except in Utf8BytesToString Cookie where there's now a <1% perf regression from before this PR:

Method Type Mean Error StdDev Op/s Ratio RatioSD Gen 0 Gen 1 Gen 2 Allocated
AsciiBytesToString KeepAlive 14.98 ns 0.082 ns 0.072 ns 66,755,499.0 1.00 0.00 0.0006 - - 48 B
Utf8BytesToString KeepAlive 73.12 ns 0.242 ns 0.215 ns 13,676,987.2 4.88 0.03 0.0013 - - 104 B
AsciiBytesToString Accept 25.28 ns 0.167 ns 0.156 ns 39,558,399.3 1.00 0.00 0.0025 - - 200 B
Utf8BytesToString Accept 149.03 ns 0.469 ns 0.416 ns 6,709,995.3 5.90 0.04 0.0050 - - 408 B
AsciiBytesToString UserAgent 26.80 ns 0.115 ns 0.102 ns 37,307,194.7 1.00 0.00 0.0030 - - 240 B
Utf8BytesToString UserAgent 168.82 ns 0.777 ns 0.727 ns 5,923,629.6 6.30 0.04 0.0061 - - 504 B
AsciiBytesToString Cookie 49.14 ns 0.214 ns 0.178 ns 20,348,028.6 1.00 0.00 0.0068 - - 544 B
Utf8BytesToString Cookie 374.41 ns 5.373 ns 5.026 ns 2,670,881.3 7.61 0.12 0.0140 - - 1,144 B

Tratcher
Tratcher reviewed Jul 27, 2021
View reviewed changes
Tratcher
Tratcher reviewed Jul 27, 2021
View reviewed changes
@wtgodbe
Copy link
Member Author

wtgodbe commented Jul 28, 2021

@Tratcher any other concerns?

@wtgodbe wtgodbe merged commit a7ca111 into main Jul 28, 2021
@wtgodbe wtgodbe deleted the wtgodbe/CRLF branch July 28, 2021 22:43
@ghost ghost added this to the 6.0-rc1 milestone Jul 28, 2021
@amcasey amcasey added the area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions label Jun 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@halter73 halter73 halter73 approved these changes

@Tratcher Tratcher Tratcher approved these changes

@JamesNK JamesNK Awaiting requested review from JamesNK JamesNK is a code owner

@BrennanConroy BrennanConroy Awaiting requested review from BrennanConroy BrennanConroy is a code owner

+2 more reviewers

@benaadams benaadams benaadams left review comments

@gfoidl gfoidl gfoidl left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions

Projects

None yet

Milestone

6.0-rc1

Development

Successfully merging this pull request may close these issues.

HTTP1 not trimming trailing CRLF's in final header Inconsistent header RFCs

8 participants

@wtgodbe @davidfowl @halter73 @benaadams @Tratcher @gfoidl @amcasey