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/6.0] Fix HTTTP/2 header decoder buffer allocation #85976

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 (HPack), where some requests/responses with large headers (4KB+) that should be accepted might end up throwing exception.

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 (rare) case of 4KB+ headers data buffers in HTTP/2 (HPack).

* 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 (HPack), where some requests/responses with large headers (4KB+) that should be accepted might end up throwing exception.

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 (rare) case of 4KB+ headers data buffers in HTTP/2 (HPack).

Author: ManickaP
Assignees: -
Labels:

area-System.Net.Http

Milestone: -

@ManickaP
Copy link
Member Author

ManickaP commented May 9, 2023

From: #85341 (comment)

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

@ManickaP ManickaP requested a review from MihaZupan May 9, 2023 14:13
@carlossanlop
Copy link
Member

@ManickaP @MihaZupan @karelz today is code complete for the June Release. Is this ready to merge?

  • Approved by Tactics.
  • Signed-off by area owner.
  • CI failures confirmed as unrelated.
  • Confirmed if OOB package changes are needed or not, and included them if they're needed.

@MihaZupan
Copy link
Member

@carlossanlop Yes, these should be ready to merge.
This and 7.0 PR (#85977) have no OOB package changes.
Test failures on both PRs are only in runtime-staging. @ManickaP investigated test failures on previous PRs (identical but against release/6.0 instead of staging) as unrelated: #85341 (comment).

@MihaZupan MihaZupan merged commit c93a40e into dotnet:release/6.0-staging May 15, 2023
@ManickaP ManickaP deleted the release/6.0-staging branch May 16, 2023 11:54
@karelz karelz added this to the 6.0.x milestone May 27, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Jun 26, 2023
@karelz karelz modified the milestones: 6.0.x, 6.0.18 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.

5 participants