Make header parsing "safe" #20562

benaadams · 2020-04-06T02:46:43Z

Contributes to #4720

Results #20562 (comment)

Before:

Method Mean Error StdDev Op/s

PlaintextTechEmpower 4,214.6 ns 81.241 ns 72.018 ns 237,268.2

JsonTechEmpower 271.9 ns 1.483 ns 1.314 ns 3,677,397.6

After:

Method Mean Error StdDev Op/s

PlaintextTechEmpower 3,766.7 ns 4.94 ns 3.86 ns 265,486.6

JsonTechEmpower 252.5 ns 0.55 ns 0.48 ns 3,960,332.8

run it against the JsonPlatform benchmark and it gives something around +3-5k RPS

benaadams · 2020-04-06T02:47:26Z

@aspnet-hello benchmark

blowdart · 2020-04-06T02:47:43Z

How did you get Ben's password hacker person?

pr-benchmarks · 2020-04-06T02:48:02Z

Starting 'Default' pipelined plaintext benchmark with session ID '0418e4f483af43ab88ed6c514379f11b'. This could take up to 30 minutes...

pr-benchmarks · 2020-04-06T02:51:17Z

Baseline

Starting baseline run on '23a1b8b3c412513eff0ffa8b0c55afdfa94b722a'...
RequestsPerSecond:           340,766
Max CPU (%):                 99
WorkingSet (MB):             89
Avg. Latency (ms):           6.8
Startup (ms):                505
First Request (ms):          120.32
Latency (ms):                0.77
Total Requests:              5,145,605
Duration: (ms)               15,100
Socket Errors:               0
Bad Responses:               0
Build Time (ms):             10,004
Published Size (KB):         120,289
SDK:                         5.0.100-preview.2.20120.3
Runtime:                     5.0.0-preview.4.20201.1
ASP.NET Core:                5.0.0-preview.4.20203.11

PR

Starting PR run on 'd65ec3482aaf32e98ef02545cbdd4a20ba5f4579'...
| Description |     RPS | CPU (%) | Memory (MB) | Avg. Latency (ms) | Startup (ms) | Build Time (ms) | Published Size (KB) | First Request (ms) | Latency (ms) | Errors | Ratio |
| ----------- | ------- | ------- | ----------- | ----------------- | ------------ | --------------- | ------------------- | ------------------ | ------------ | ------ | ----- |
|      Before | 340,766 |      99 |          89 |               6.8 |          505 |           10004 |              120289 |             120.32 |         0.77 |      0 |  1.00 |
|       After | 336,138 |      99 |          87 |              6.87 |          511 |            7502 |              120289 |             128.36 |         0.54 |      0 |  0.99 |

benaadams · 2020-04-06T02:53:34Z

How did you get Ben's password hacker person?

Hmm... I think it can go faster...

benaadams · 2020-04-06T04:11:17Z

@aspnet-hello benchmark

pr-benchmarks · 2020-04-06T04:12:02Z

Starting 'Default' pipelined plaintext benchmark with session ID 'e14a4ddadca14a32a69d2c229c35de81'. This could take up to 30 minutes...

pr-benchmarks · 2020-04-06T04:15:23Z

Baseline

Starting baseline run on '23a1b8b3c412513eff0ffa8b0c55afdfa94b722a'...
RequestsPerSecond:           339,301
Max CPU (%):                 99
WorkingSet (MB):             88
Avg. Latency (ms):           6.84
Startup (ms):                481
First Request (ms):          119.93
Latency (ms):                0.66
Total Requests:              5,122,198
Duration: (ms)               15,100
Socket Errors:               0
Bad Responses:               0
Build Time (ms):             10,003
Published Size (KB):         120,289
SDK:                         5.0.100-preview.2.20120.3
Runtime:                     5.0.0-preview.4.20201.1
ASP.NET Core:                5.0.0-preview.4.20203.11

PR

Starting PR run on '10fd8cfaa66aab8124e7296493a932af7f40c943'...
| Description |     RPS | CPU (%) | Memory (MB) | Avg. Latency (ms) | Startup (ms) | Build Time (ms) | Published Size (KB) | First Request (ms) | Latency (ms) | Errors | Ratio |
| ----------- | ------- | ------- | ----------- | ----------------- | ------------ | --------------- | ------------------- | ------------------ | ------------ | ------ | ----- |
|      Before | 339,301 |      99 |          88 |              6.84 |          481 |           10003 |              120289 |             119.93 |         0.66 |      0 |  1.00 |
|       After | 333,614 |      99 |          89 |                 7 |          480 |            7508 |              120289 |             124.77 |         0.75 |      0 |  0.98 |

gfoidl

Nits

src/Servers/Kestrel/Core/src/Internal/Http/HttpParser.cs

benaadams · 2020-04-06T11:41:47Z

Meh skipping bounds check for one space doesn't make a great difference and doesn't look very pretty; so reverted

src/Servers/Kestrel/Core/src/Internal/Http/HttpParser.cs

benaadams · 2020-04-06T15:34:40Z

@aspnet-hello benchmark

pr-benchmarks · 2020-04-06T15:35:02Z

Starting 'Default' pipelined plaintext benchmark with session ID 'd03e9c0016c24d1dbe7c5d2298b203f8'. This could take up to 30 minutes...

pr-benchmarks · 2020-04-06T15:39:09Z

Baseline

Starting baseline run on '23a1b8b3c412513eff0ffa8b0c55afdfa94b722a'...
RequestsPerSecond:           335,344
Max CPU (%):                 99
WorkingSet (MB):             88
Avg. Latency (ms):           6.88
Startup (ms):                507
First Request (ms):          157.9
Latency (ms):                0.59
Total Requests:              5,062,897
Duration: (ms)               15,100
Socket Errors:               0
Bad Responses:               0
Build Time (ms):             21,509
Published Size (KB):         120,289
SDK:                         5.0.100-preview.2.20120.3
Runtime:                     5.0.0-preview.4.20201.1
ASP.NET Core:                5.0.0-preview.4.20203.11

PR

Starting PR run on 'e31989b274e627c654b563b2bda3f92ec90eb460'...
| Description |     RPS | CPU (%) | Memory (MB) | Avg. Latency (ms) | Startup (ms) | Build Time (ms) | Published Size (KB) | First Request (ms) | Latency (ms) | Errors | Ratio |
| ----------- | ------- | ------- | ----------- | ----------------- | ------------ | --------------- | ------------------- | ------------------ | ------------ | ------ | ----- |
|      Before | 335,344 |      99 |          88 |              6.88 |          507 |           21509 |              120289 |              157.9 |         0.59 |      0 |  1.00 |
|       After | 342,796 |      99 |          88 |              6.73 |          492 |            7503 |              120289 |             126.63 |         1.58 |      0 |  1.02 |

benaadams · 2020-04-06T15:49:42Z

How did you get Ben's password hacker person?

@blowdart you'll be pleased to know normal service has been resumed 😉

Before | 335,344
After  | 342,796

benaadams · 2020-04-07T02:27:08Z

@aspnet-hello benchmark

pr-benchmarks · 2020-04-07T02:28:01Z

Starting 'Default' pipelined plaintext benchmark with session ID 'fdcf5ba5ae7b4edabd81a28624c093b3'. This could take up to 30 minutes...

pr-benchmarks · 2020-04-07T02:31:50Z

Baseline

Starting baseline run on 'a5ee4fd34dd65bee73f78a08776d231ef6b893f0'...
RequestsPerSecond:           341,860
Max CPU (%):                 99
WorkingSet (MB):             92
Avg. Latency (ms):           6.82
Startup (ms):                482
First Request (ms):          119.82
Latency (ms):                0.83
Total Requests:              5,159,530
Duration: (ms)               15,090
Socket Errors:               0
Bad Responses:               0
Build Time (ms):             9,503
Published Size (KB):         120,289
SDK:                         5.0.100-preview.2.20120.3
Runtime:                     5.0.0-preview.4.20201.1
ASP.NET Core:                5.0.0-preview.4.20203.11

PR

Starting PR run on '515fb08f80d8fa20f7894580fdd4b7e17e837bb5'...
| Description |     RPS | CPU (%) | Memory (MB) | Avg. Latency (ms) | Startup (ms) | Build Time (ms) | Published Size (KB) | First Request (ms) | Latency (ms) | Errors | Ratio |
| ----------- | ------- | ------- | ----------- | ----------------- | ------------ | --------------- | ------------------- | ------------------ | ------------ | ------ | ----- |
|      Before | 341,860 |      99 |          92 |              6.82 |          482 |            9503 |              120289 |             119.82 |         0.83 |      0 |  1.00 |
|       After | 338,369 |      99 |          92 |              6.82 |          510 |            7513 |              120289 |             126.52 |         0.57 |      0 |  0.99 |

benaadams · 2020-04-07T14:28:14Z

@aspnet-hello benchmark

pr-benchmarks · 2020-04-07T14:29:01Z

Starting 'Default' pipelined plaintext benchmark with session ID 'ab1c5b8a298040fbacbbd70f1a2c2c90'. This could take up to 30 minutes...

pr-benchmarks · 2020-04-07T14:33:24Z

Baseline

Starting baseline run on 'a5ee4fd34dd65bee73f78a08776d231ef6b893f0'...
RequestsPerSecond:           338,286
Max CPU (%):                 99
WorkingSet (MB):             91
Avg. Latency (ms):           6.92
Startup (ms):                510
First Request (ms):          155.42
Latency (ms):                0.79
Total Requests:              5,106,679
Duration: (ms)               15,100
Socket Errors:               0
Bad Responses:               0
Build Time (ms):             21,013
Published Size (KB):         120,330
SDK:                         5.0.100-preview.2.20120.3
Runtime:                     5.0.0-preview.4.20205.13
ASP.NET Core:                5.0.0-preview.4.20206.19

PR

Starting PR run on '9b867f2737c2225997134d167d9cb56b71796b2a'...
| Description |     RPS | CPU (%) | Memory (MB) | Avg. Latency (ms) | Startup (ms) | Build Time (ms) | Published Size (KB) | First Request (ms) | Latency (ms) | Errors | Ratio |
| ----------- | ------- | ------- | ----------- | ----------------- | ------------ | --------------- | ------------------- | ------------------ | ------------ | ------ | ----- |
|      Before | 338,286 |      99 |          91 |              6.92 |          510 |           21013 |              120330 |             155.42 |         0.79 |      0 |  1.00 |
|       After | 341,698 |      99 |          92 |              6.76 |          481 |            7505 |              120330 |             124.09 |          0.5 |      0 |  1.01 |

benaadams · 2020-04-07T17:31:27Z

@aspnet-hello benchmark

pr-benchmarks · 2020-04-07T17:32:01Z

Starting 'Default' pipelined plaintext benchmark with session ID 'e38f2758627943d1824c07f26bba7134'. This could take up to 30 minutes...

adamsitnik · 2020-04-08T09:57:57Z

I've fetched these changes and run them against the microbenchmarks from #20518

Before:

Method	Mean	Error	StdDev	Op/s
PlaintextTechEmpower	4,214.6 ns	81.241 ns	72.018 ns	237,268.2
JsonTechEmpower	271.9 ns	1.483 ns	1.314 ns	3,677,397.6

After:

Method	Mean	Error	StdDev	Op/s
PlaintextTechEmpower	3,766.7 ns	4.94 ns	3.86 ns	265,486.6
JsonTechEmpower	252.5 ns	0.55 ns	0.48 ns	3,960,332.8

I've also run it against the JsonPlatform benchmark and it gives something around +3-5k RPS improvement for JSON. @benaadams thank you!

KrzysztofCwalina · 2020-04-08T18:30:19Z

Impressive!

davidfowl · 2020-04-08T18:54:30Z

Safe and faster

benaadams · 2020-04-09T01:57:25Z

Squashed to single commit and rebased

benaadams · 2020-04-09T14:39:23Z

Errors from running out of space

npm WARN tar ENOSPC: no space left on device, write

And publish error which might be the same

Project new mvc --razor-runtime-compilation failed to publish.

src/Servers/Kestrel/Core/src/Internal/Http/HttpParser.cs

benaadams · 2020-04-09T22:16:02Z

Ubuntu System.IO.IOException: No space left on device

src/Servers/Kestrel/Core/src/Internal/Http/HttpParser.cs

halter73 · 2020-04-09T22:21:32Z

src/Servers/Kestrel/Core/src/Internal/Http/HttpParser.cs

@@ -236,159 +234,229 @@ public unsafe bool ParseHeaders(TRequestHandler handler, ref SequenceReader<byte
                    // in the span to contain a header.
                    if (readAhead == 0)
                    {
-                        length = span.IndexOf(ByteLF) + 1;
-                        if (length > 0)
+                        length = span.IndexOfAny(ByteCR, ByteLF);


benaadams · 2020-04-11T13:37:32Z

Rebased to see if ci is alive

benaadams · 2020-04-11T18:59:36Z

It's alive 😎

halter73 · 2020-04-13T16:46:52Z

Thanks @benaadams!

benaadams · 2020-04-23T22:33:34Z

@anurse looks like release notes are done by milestones; want to stick one on this?

benaadams requested review from analogrelay, halter73, jkotalik and Tratcher as code owners April 6, 2020 02:46

ghost added the area-servers label Apr 6, 2020

gfoidl reviewed Apr 6, 2020

View reviewed changes

src/Servers/Kestrel/Core/src/Internal/Http/HttpParser.cs Outdated Show resolved Hide resolved

src/Servers/Kestrel/Core/src/Internal/Http/HttpParser.cs Outdated Show resolved Hide resolved

src/Servers/Kestrel/Core/src/Internal/Http/HttpParser.cs Outdated Show resolved Hide resolved

benaadams mentioned this pull request Apr 6, 2020

Is it possible to tune request parsing any further? #20518

Closed

davidfowl reviewed Apr 6, 2020

View reviewed changes

src/Servers/Kestrel/Core/src/Internal/Http/HttpParser.cs Show resolved Hide resolved

benaadams force-pushed the safe-header-parsing branch from 6f6f452 to 515fb08 Compare April 7, 2020 02:26

benaadams force-pushed the safe-header-parsing branch from 917e315 to 02debfc Compare April 9, 2020 01:56

halter73 reviewed Apr 9, 2020

View reviewed changes

src/Servers/Kestrel/Core/src/Internal/Http/HttpParser.cs Outdated Show resolved Hide resolved

halter73 reviewed Apr 9, 2020

View reviewed changes

src/Servers/Kestrel/Core/src/Internal/Http/HttpParser.cs Outdated Show resolved Hide resolved

danmoseley requested a review from GrabYourPitchforks April 9, 2020 21:14

halter73 reviewed Apr 9, 2020

View reviewed changes

src/Servers/Kestrel/Core/src/Internal/Http/HttpParser.cs Show resolved Hide resolved

halter73 reviewed Apr 9, 2020

View reviewed changes

halter73 approved these changes Apr 9, 2020

View reviewed changes

benaadams added 6 commits April 11, 2020 14:36

Make header parsing "safe"

b2ac2b4

OnHeadersComplete improve speculation and memory ordering (VTune)

d3b1b03

Remove gotos

6892f4a

Remove result variable

58d2231

Remove gotos

108529f

Unsafe call through to safe exception

0ba3a52

benaadams force-pushed the safe-header-parsing branch from 5e58983 to 0ba3a52 Compare April 11, 2020 13:37

halter73 merged commit 6016e70 into dotnet:master Apr 13, 2020

benaadams deleted the safe-header-parsing branch April 13, 2020 17:11

benaadams mentioned this pull request Apr 17, 2020

Make HTTP/1.1 startline parsing "safe" #20885

Merged

analogrelay added this to the 5.0.0-preview4 milestone Apr 23, 2020

amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Jun 6, 2023

Make header parsing "safe" #20562

Make header parsing "safe" #20562

Uh oh!

Conversation

benaadams commented Apr 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benaadams commented Apr 6, 2020

Uh oh!

blowdart commented Apr 6, 2020

Uh oh!

pr-benchmarks bot commented Apr 6, 2020

Uh oh!

pr-benchmarks bot commented Apr 6, 2020

Baseline

PR

Uh oh!

benaadams commented Apr 6, 2020

Uh oh!

benaadams commented Apr 6, 2020

Uh oh!

pr-benchmarks bot commented Apr 6, 2020

Uh oh!

pr-benchmarks bot commented Apr 6, 2020

Baseline

PR

Uh oh!

gfoidl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

benaadams commented Apr 6, 2020

Uh oh!

Uh oh!

benaadams commented Apr 6, 2020

Uh oh!

pr-benchmarks bot commented Apr 6, 2020

Uh oh!

pr-benchmarks bot commented Apr 6, 2020

Baseline

PR

Uh oh!

benaadams commented Apr 6, 2020

Uh oh!

benaadams commented Apr 7, 2020

Uh oh!

pr-benchmarks bot commented Apr 7, 2020

Uh oh!

pr-benchmarks bot commented Apr 7, 2020

Baseline

PR

Uh oh!

benaadams commented Apr 7, 2020

Uh oh!

pr-benchmarks bot commented Apr 7, 2020

Uh oh!

pr-benchmarks bot commented Apr 7, 2020

Baseline

PR

Uh oh!

benaadams commented Apr 7, 2020

Uh oh!

pr-benchmarks bot commented Apr 7, 2020

Uh oh!

adamsitnik commented Apr 8, 2020

Uh oh!

KrzysztofCwalina commented Apr 8, 2020

Uh oh!

davidfowl commented Apr 8, 2020

Uh oh!

benaadams commented Apr 9, 2020

Uh oh!

benaadams commented Apr 9, 2020

Uh oh!

Uh oh!

Uh oh!

benaadams commented Apr 9, 2020

Uh oh!

benaadams commented Apr 6, 2020 •

edited

Loading