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

Merged
merged 4 commits into from
May 13, 2023

Conversation

ManickaP
Copy link
Member

Manual backport of #47793
Backport of runtime PR in main dotnet/runtime#78862
Port of runtime 7.0 PR dotnet/runtime#85337

Fixes dotnet/runtime#78516

/cc @Tratcher @JamesNK

Fix HTTP/3 and 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 and in QPack since 7.0.

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

Regression?

  • Yes
  • No

Regressed in 7.0 for QPack.

Risk

  • High
  • Medium
  • Low

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.

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 7.0.x milestone 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/7.0] Fix HTTP/3 and HTTTP/2 header decoder buffer allocation [release/7.0] Fix HTTP/3 and HTTP/2 header decoder buffer allocation Apr 28, 2023
@BrennanConroy
Copy link
Member

FYI you need to manually make the change in main too. There is no automated forward porting.

@Tratcher
Copy link
Member

This is already in main #47793

@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 3 pipeline(s).

@adityamandaleeka
Copy link
Member

Just noticed this is still red. I'll fix the CI issues here.

@adityamandaleeka
Copy link
Member

Er, looks like I don't have permission to push to your fork @ManickaP. Opened a PR here instead to update this branch: ManickaP#123

@ManickaP
Copy link
Member Author

ManickaP commented May 9, 2023

Er, looks like I don't have permission to push to your fork @ManickaP Marie Píchová FTE. Opened a PR here instead to update this branch: ManickaP#123

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

@wtgodbe
Copy link
Member

wtgodbe commented May 11, 2023

Looks like we've still got failures, do we need another fix? @JamesNK @ManickaP @adityamandaleeka

@wtgodbe wtgodbe merged commit 25360e7 into dotnet:release/7.0 May 13, 2023
@ghost ghost modified the milestones: 7.0.x, 7.0.7 May 13, 2023
@ghost ghost added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label May 13, 2023
@ManickaP ManickaP deleted the release/7.0 branch May 13, 2023 08:45
@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
None yet
Development

Successfully merging this pull request may close these issues.

8 participants