Skip to content

Conversation

@JamesNK
Copy link
Member

@JamesNK JamesNK commented Aug 18, 2021

Cherry-picked from #35434


Description

An HTTP/3 stream is pooled and reused by Kestrel if it completed gracefully. The check for graceful complete is wrong. A stream that was aborted could be pooled.

Customer Impact

An HTTP/3 request that uses an incorrectly pooled stream will have undefined behavior. Various bad things could happen depending upon the stream state when it was pooled. Makes HTTP/3 unstable. IMO this is a must-fix bug.

Risk

Low. Changes are to QUIC which is only used by HTTP/3 which is shipping as a preview feature in .NET 6.

Original issue and/or the PR to master.

@JamesNK JamesNK added area-runtime tell-mode Indicates a PR which is being merged during tell-mode labels Aug 18, 2021
@JamesNK JamesNK added this to the 6.0-rc1 milestone Aug 18, 2021
@JamesNK JamesNK changed the title [RC1] HTTP/3: Fix incorrectly pooling aborted streams [6.0-rc1] HTTP/3: Fix incorrectly pooling aborted streams Aug 18, 2021
@JamesNK JamesNK force-pushed the jamesnk/6rc1-http3-quicstream-pooling-fix branch from ccbd5fc to 4f6084e Compare August 18, 2021 20:31
@JamesNK JamesNK changed the title [6.0-rc1] HTTP/3: Fix incorrectly pooling aborted streams [release/6.0-rc1] HTTP/3: Fix incorrectly pooling aborted streams Aug 18, 2021
@wtgodbe
Copy link
Member

wtgodbe commented Aug 18, 2021

@JamesNK let me know if/when this gets tactics approval & I can hit merge for you

@JamesNK JamesNK added ask-mode This issue / PR is a patch candidate which we will bar-check internally before patching it. and removed tell-mode Indicates a PR which is being merged during tell-mode labels Aug 18, 2021
@adityamandaleeka
Copy link
Member

@davidfowl

Copy link
Member

@halter73 halter73 left a comment

Choose a reason for hiding this comment

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

Though we should probably just set _serverAborted whenever _shutdownReadReason or _shutdownWriteReason is set to an exception.

Copy link
Member

@davidfowl davidfowl left a comment

Choose a reason for hiding this comment

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

LGTM

@davidfowl davidfowl added the Servicing-approved Shiproom has approved the issue label Aug 20, 2021
@wtgodbe wtgodbe merged commit 670cfd0 into release/6.0-rc1 Aug 20, 2021
@wtgodbe wtgodbe deleted the jamesnk/6rc1-http3-quicstream-pooling-fix branch August 20, 2021 20:23
dougbu added a commit that referenced this pull request Aug 24, 2021
* [release/6.0-rc1] Update dependencies from dotnet/runtime dotnet/efcore (#35513)

* Set HttpSys read error log levels to debug #35490 (#35542)

Co-authored-by: Chris R <[email protected]>

* [release/6.0-rc1] Treat reference type parameters in oblivious nullability context as optional (#35526)

* Treat parameters in oblivious nullability context as optional

* Only apply fix for reference types

* Update optionality check in API descriptor

* Update check in BindAsync and Mvc.ApiExplorer test

* HTTP/3: Use new QuicStream.ReadsCompleted property in transport (#35483)

Co-authored-by: James Newton-King <[email protected]>

* HTTP/3: Fix incorrectly pooling aborted streams (#35441)

* [release/6.0-rc1] Binding support for 'bool' values with InputRadioGroup and InputSelect (#35523)

* Binding support for 'bool' values with InputRadioGroup and InputSelect (#35318)

* Update CodeCheck.ps1

Co-authored-by: Brennan <[email protected]>

* [release/6.0-rc1] Update dependencies from dotnet/efcore dotnet/runtime (#35558)

* Update dependencies from https://github.com/dotnet/efcore build 20210820.19

Microsoft.EntityFrameworkCore.Tools , dotnet-ef , Microsoft.EntityFrameworkCore , Microsoft.EntityFrameworkCore.SqlServer , Microsoft.EntityFrameworkCore.InMemory , Microsoft.EntityFrameworkCore.Relational , Microsoft.EntityFrameworkCore.Sqlite , Microsoft.EntityFrameworkCore.Design
 From Version 6.0.0-rc.1.21420.8 -> To Version 6.0.0-rc.1.21420.19

* Update dependencies from https://github.com/dotnet/efcore build 20210820.22

Microsoft.EntityFrameworkCore.Tools , dotnet-ef , Microsoft.EntityFrameworkCore , Microsoft.EntityFrameworkCore.SqlServer , Microsoft.EntityFrameworkCore.InMemory , Microsoft.EntityFrameworkCore.Relational , Microsoft.EntityFrameworkCore.Sqlite , Microsoft.EntityFrameworkCore.Design
 From Version 6.0.0-rc.1.21420.8 -> To Version 6.0.0-rc.1.21420.22

* Update dependencies from https://github.com/dotnet/efcore build 20210820.30

Microsoft.EntityFrameworkCore.Tools , dotnet-ef , Microsoft.EntityFrameworkCore , Microsoft.EntityFrameworkCore.SqlServer , Microsoft.EntityFrameworkCore.InMemory , Microsoft.EntityFrameworkCore.Relational , Microsoft.EntityFrameworkCore.Sqlite , Microsoft.EntityFrameworkCore.Design
 From Version 6.0.0-rc.1.21420.8 -> To Version 6.0.0-rc.1.21420.30

* Update dependencies from https://github.com/dotnet/runtime build 20210820.15

Microsoft.NETCore.Platforms , Microsoft.NETCore.BrowserDebugHost.Transport , Microsoft.NETCore.App.Runtime.win-x64 , Microsoft.Win32.SystemEvents , Microsoft.NET.Runtime.MonoAOTCompiler.Task , Microsoft.Internal.Runtime.AspNetCore.Transport , Microsoft.Extensions.Primitives , Microsoft.Extensions.Options.DataAnnotations , Microsoft.Extensions.Options.ConfigurationExtensions , Microsoft.NET.Runtime.WebAssembly.Sdk , Microsoft.Extensions.Options , Microsoft.Extensions.Logging.TraceSource , Microsoft.Extensions.Logging.EventSource , Microsoft.Extensions.Configuration.UserSecrets , Microsoft.Extensions.Configuration.Json , Microsoft.Extensions.Configuration.Ini , Microsoft.Extensions.Configuration.FileExtensions , Microsoft.Extensions.Configuration.EnvironmentVariables , Microsoft.Extensions.Configuration.CommandLine , Microsoft.Extensions.Configuration.Binder , Microsoft.Extensions.Configuration.Xml , Microsoft.Extensions.Configuration.Abstractions , Microsoft.Extensions.Caching.Memory , Microsoft.Extensions.Caching.Abstractions , Microsoft.Extensions.Configuration , Microsoft.Extensions.DependencyInjection , Microsoft.Extensions.DependencyInjection.Abstractions , Microsoft.Extensions.Logging.EventLog , Microsoft.Extensions.Logging.Debug , Microsoft.Extensions.Logging.Console , Microsoft.Extensions.Logging.Configuration , Microsoft.Extensions.Logging.Abstractions , Microsoft.Extensions.Logging , Microsoft.Extensions.Http , Microsoft.Extensions.Hosting.Abstractions , Microsoft.Extensions.Hosting , Microsoft.Extensions.HostFactoryResolver.Sources , Microsoft.Extensions.FileSystemGlobbing , Microsoft.Extensions.FileProviders.Physical , Microsoft.Extensions.FileProviders.Composite , Microsoft.Extensions.FileProviders.Abstractions , Microsoft.Extensions.DependencyModel , Microsoft.NETCore.App.Runtime.AOT.win-x64.Cross.browser-wasm , Microsoft.NETCore.App.Ref , System.Windows.Extensions , System.Threading.Channels , System.Text.Json , System.Text.Encodings.Web , System.ServiceProcess.ServiceController , System.Drawing.Common , System.DirectoryServices.Protocols , System.Diagnostics.EventLog , System.Diagnostics.DiagnosticSource , System.IO.Pipelines , System.Security.Permissions , System.Security.Cryptography.Xml , System.Security.Cryptography.Pkcs , System.Runtime.CompilerServices.Unsafe , System.Resources.Extensions , System.Reflection.Metadata , System.Net.Http.WinHttpHandler , System.Net.Http.Json
 From Version 6.0.0-rc.1.21420.7 -> To Version 6.0.0-rc.1.21420.15

Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com>

* Improve Minimal APIs support for request media types #35082 (#35230) (#35579)

* add support for request media types

* [release/6.0-rc1] Update dependencies from dotnet/efcore dotnet/runtime (#35573)

Co-authored-by: dotnet-maestro[bot] <42748379+dotnet-maestro[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Chris R <[email protected]>
Co-authored-by: Safia Abdalla <[email protected]>
Co-authored-by: James Newton-King <[email protected]>
Co-authored-by: Mackinnon Buck <[email protected]>
Co-authored-by: Brennan <[email protected]>
Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com>
Co-authored-by: Rafiki Assumani <[email protected]>
@amcasey 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions ask-mode This issue / PR is a patch candidate which we will bar-check internally before patching it. Servicing-approved Shiproom has approved the issue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants