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

Fix #4815 by adjusting length n before checking overflow #4816

Closed

Conversation

minghuaw
Copy link

@minghuaw minghuaw commented Jul 8, 2022

Motivation

This is to solve the issue #4815, which finds that the encoded bytes of a max-sized frame with a negative-valued length adjustment cannot be decoded with the codec.

Solution

The problem is caused by checking overflow before making length adjustment to the decoded length n (#4815 (comment)). This PR simply reverses the order (ie. it now makes adjustment to the length n before checking overflow).

A new unit test read_single_frame_negative_length_adjusted_and_max_sized has been added to tokio-util/tests/length_delimited.rs to test this scenario.

@minghuaw minghuaw changed the title Fix #4815 be adjusting n before checking overflow Fix #4815 by adjusting length n before checking overflow Jul 8, 2022
@Darksonn Darksonn added A-tokio-util Area: The tokio-util crate M-codec Module: tokio-util/codec labels Jul 9, 2022
Copy link
Contributor

@Darksonn Darksonn left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@minghuaw
Copy link
Author

minghuaw commented Jul 9, 2022

The failed tests seem like a "bug" with the h2 crate. (h2_connect_large_body test)

It uses a LengthDelimitedCodec in its own h2::codec::FramedRead but uses a custom encoder for its own h2::codec::FramedWrite. The LengthDelimitedCodec in the reader has a length adjustment of positive 9; however, the custom encoder doesn't make any length adjustment when it encodes the payload length (see encoder, head).

One workaround is to add a positive 9 when setting the h2::codec::FramedRead's max frame size

minghuaw and others added 2 commits July 9, 2022 06:16
@Darksonn Darksonn added the S-breaking-change A breaking change that requires manual coordination to be released. label Jul 16, 2022
@Darksonn
Copy link
Contributor

We might want to consider waiting until the next breaking release of tokio-util. We are not going to make a release that breaks hyper.

@Darksonn
Copy link
Contributor

I'm closing this for now. I'll reopen it when we make the tokio-util breaking release.

@Darksonn Darksonn closed this Apr 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio-util Area: The tokio-util crate M-codec Module: tokio-util/codec S-breaking-change A breaking change that requires manual coordination to be released.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants