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

HTTP/3: Fix variable length encoding #51486

Merged
merged 1 commit into from
Apr 19, 2021
Merged

Conversation

JamesNK
Copy link
Member

@JamesNK JamesNK commented Apr 19, 2021

I noticed two byte encoding is wrong. Usable bits is 14, not 16 - https://tools.ietf.org/html/draft-ietf-quic-transport-16#section-16

Also adds const field that aspnetcore will use to calculate header sizes.

@JamesNK JamesNK requested a review from a team April 19, 2021 10:14
@ghost
Copy link

ghost commented Apr 19, 2021

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

Issue Details

I noticed two byte encoding is wrong. Usable bits is 14, not 16 - https://tools.ietf.org/html/draft-ietf-quic-transport-16#section-16

Also adds const field that aspnetcore will use to calculate header sizes.

Author: JamesNK
Assignees: -
Labels:

area-System.Net.Http

Milestone: -

Copy link
Member

@ManickaP ManickaP left a comment

Choose a reason for hiding this comment

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

LGTM, thanks.

@halter73
Copy link
Member

Is it possible to add a regression test for this?

@JamesNK
Copy link
Member Author

JamesNK commented Apr 19, 2021

Is it possible to add a regression test for this?

There are tests in aspnetcore that check this scenario. I've created an issue for adding proper unit tests for this type - #51519

@JamesNK JamesNK merged commit 1f89aba into dotnet:main Apr 19, 2021
@JamesNK JamesNK deleted the jamesnk/http3-fixes branch April 19, 2021 21:30
@ghost ghost locked as resolved and limited conversation to collaborators May 19, 2021
@karelz karelz added this to the 6.0.0 milestone May 20, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants