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 HTTP/2 header decoder buffer allocation #47950

Merged
merged 3 commits into from
May 11, 2023

Conversation

ManickaP
Copy link
Member

@ManickaP ManickaP commented Apr 28, 2023

Manual backport of #47793
Backport of runtime PR in main dotnet/runtime#78862
Port of runtime 6.0 PR dotnet/runtime#85341

Fixes dotnet/runtime#78516

/cc @Tratcher @JamesNK

Fix HTTTP/2 header decoder buffer allocation

Description

We resize an internal buffer by an incorrect amount, and subsequent copies to that buffer will throw. The problem occurs in HPack since 5.0.

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 runtime so this affects both client and server - existing PR in runtime: dotnet/runtime#85341.

Regression?

  • Yes
  • No

Risk

  • High
  • Medium
  • Low

Low, as this affects only (rare) case of 4KB+ headers data buffers in HTTP/2 (HPack).

Verification

  • Manual (required)
  • Automated

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

Packaging changes reviewed?

  • Yes
  • No
  • N/A

@ghost ghost added this to the 6.0.x milestone Apr 28, 2023
@ghost ghost added this to In Progress in Servicing Apr 28, 2023
@ghost
Copy link

ghost commented Apr 28, 2023

Hi @ManickaP. If this is not a tell-mode PR, please make sure to follow the instructions laid out in the servicing process document.
Otherwise, please add tell-mode label.

@ManickaP ManickaP added the Servicing-consider Shiproom approval is required for the issue label Apr 28, 2023
@ghost
Copy link

ghost commented Apr 28, 2023

Hi @ManickaP. Please make sure you've updated the PR description to use the Shiproom Template. Also, make sure this PR is not marked as a draft and is ready-to-merge.

To learn more about how to prepare a servicing PR click here.

@JamesNK JamesNK changed the title [release/6.0] Fix HTTTP/2 header decoder buffer allocation [release/6.0] Fix HTTP/2 header decoder buffer allocation Apr 28, 2023
@karelz
Copy link
Member

karelz commented Apr 30, 2023

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

@karelz karelz added Servicing-approved Shiproom has approved the issue and removed Servicing-consider Shiproom approval is required for the issue labels Apr 30, 2023
@ghost
Copy link

ghost commented Apr 30, 2023

Hi @ManickaP. This PR was just approved to be included in the upcoming servicing release. Somebody from the @dotnet/aspnet-build team will get it merged when the branches are open. Until then, please make sure all the CI checks pass and the PR is reviewed.

@JamesNK
Copy link
Member

JamesNK commented Apr 30, 2023

Need changes I added to the code sync PR to fix warnings - #47793

@wtgodbe
Copy link
Member

wtgodbe commented May 2, 2023

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@adityamandaleeka
Copy link
Member

@ManickaP here's a PR to this branch in your fork to fix the CI failures: ManickaP#124

@ManickaP
Copy link
Member Author

ManickaP commented May 9, 2023

@ManickaP Marie Píchová FTE here's a PR to this branch in your fork to fix the CI failures: ManickaP#124

Merged and the pipelines are running, thank you for the fix!

@wtgodbe
Copy link
Member

wtgodbe commented May 11, 2023

Components-e2e failure is known & unrelated

@wtgodbe wtgodbe merged commit b389117 into dotnet:release/6.0 May 11, 2023
21 of 23 checks passed
Servicing automation moved this from In Progress to Done May 11, 2023
@ghost ghost modified the milestones: 6.0.x, 6.0.18 May 11, 2023
@ghost ghost added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label May 11, 2023
@ManickaP ManickaP deleted the release/6.0 branch May 13, 2023 08:46
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Aug 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions Servicing-approved Shiproom has approved the issue
Projects
Development

Successfully merging this pull request may close these issues.

None yet

6 participants