Skip to content
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

[release/7.0] Fix HTTP/3 and HTTTP/2 header decoder buffer allocation #85977

Merged
merged 1 commit into from
May 15, 2023

Conversation

ManickaP
Copy link
Member

@ManickaP ManickaP commented May 9, 2023

Fixes #78516

Backports #78862

Customer impact

Reliability problem in HTTP/2 and HTTP/3, where some requests/responses with large headers that should be accepted might end up throwing exception.

  • In HPack cases (HTTP/2 scenarios), the issue is much less likely to be hit as it requires 4KB of headers.
  • In QPack (HTTP/3), the header size required to hit this is much smaller and that's where this was caught by the original issue reporter.

This is a shared code with Kestrel so this affects server side as well - expect follow up PR in ASP.NET.

Testing

Added tests for the root cause and similar scenarios, increasing test coverage. All of those are ran in CI.

Risk

Low, as this affects only QPack (H/3 is still not as wide-spread as other HTTP versions) and HPack in (rare) case of 4KB+ sized headers data buffers.

* Add test for literal field without name reference

* Fix header name buffer allocation

* Add more tests

* Unified QPackDecoderTest test files

* Fix variable name

* Fixed HPackDecoder and ported QPack tests

* Feedback

---------

Co-authored-by: ManickaP <mapichov@microsoft.com>
@ghost
Copy link

ghost commented May 9, 2023

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #78516

Backports #78862

Customer impact

Reliability problem in HTTP/2 and HTTP/3, where some requests/responses with large headers that should be accepted might end up throwing exception.

  • In HPack cases (HTTP/2 scenarios), the issue is much less likely to be hit as it requires 4KB of headers.
  • In QPack (HTTP/3), the header size required to hit this is much smaller and that's where this was caught by the original issue reporter.

This is a shared code with Kestrel so this affects server side as well - expect follow up PR in ASP.NET.

Testing

Added tests for the root cause and similar scenarios, increasing test coverage. All of those are ran in CI.

Risk

Low, as this affects only QPack (H/3 is still not as wide-spread as other HTTP versions) and HPack in (rare) case of 4KB+ sized headers data buffers.

Author: ManickaP
Assignees: -
Labels:

area-System.Net.Http

Milestone: -

@ManickaP ManickaP requested a review from MihaZupan May 9, 2023 14:15
@ManickaP ManickaP added the Servicing-approved Approved for servicing release label May 9, 2023
@ManickaP
Copy link
Member Author

ManickaP commented May 9, 2023

From: #85337 (comment)

Approved by Tactics (@SteveMCarroll) on 4/27 via email. Marking as servicing-approved.

@MihaZupan MihaZupan merged commit e005374 into dotnet:release/7.0-staging May 15, 2023
@ManickaP ManickaP deleted the release/7.0-staging branch May 16, 2023 11:55
@karelz karelz added this to the 7.0.x milestone May 27, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Jun 26, 2023
@karelz karelz modified the milestones: 7.0.x, 7.0.7 Jul 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Net.Http Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants