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.

Sign up for GitHub

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

Jump to bottom

Chunked HTTP/1.1 doesn't send the last zero-chunk when body is empty #3252

Closed
XOR-op opened this issue Jun 16, 2023 · 3 comments · Fixed by #3254
Closed

Chunked HTTP/1.1 doesn't send the last zero-chunk when body is empty #3252

XOR-op opened this issue Jun 16, 2023 · 3 comments · Fixed by #3254
Assignees
@seanmonstar
Labels
C-bug Category: bug. Something is wrong. This is bad!

Comments

@XOR-op
Copy link

XOR-op commented Jun 16, 2023

Version
hyper 0.14.26, h2 0.3.19, http 0.2.9

Platform
Linux 5.15, Ubuntu 22.04

Description
In HTTP/1.1, when transfer-encoding: chunked is set, all the data should be sent at a format of chunk. However, in hyper, if the body part is empty, e.g. Body::empty() or Body::from_static(b""), hyper server doesn't send b"0\r\n\r\n" to the client, causing infitite wait on the client side. When the body contains some bytes, e.g. Body::from_static(b"0"), hyper will send a non-zero size chunk as well as a zero-size last chunk. According to pseudo code from RFC9112 7.1.3, zero size body should be allowed.

I expect hyper sends valid last chunk b"0\r\n\r\n" to the client when the body is empty. Manually sending it is impossible with public API so I have no workaround now.

Source code to reproduce the behavior:

async fn get_302() -> anyhow::Result<Response<Body>> {
    let resp_builder = http::response::Builder::new();
    let (mut parts, body) = resp_builder
        .status(302)
        .version(Version::HTTP_11)
        .header(header::LOCATION, "127.0.0.1:1234/ok")
        .header(header::TRANSFER_ENCODING, "chunked")
        .body(Body::empty())
        .unwrap().into_parts();
    Ok(Response::from_parts(parts, body))
}

async fn run_server() {
    let listener = TcpListener::bind("127.0.0.1:1234").await.unwrap();
    let service = service_fn(|req| {
        get_302()
    });
    while let (conn, _) = listener.accept().await.unwrap() {
        let _ = Http::new().http1_only(true).serve_connection(conn, service).await;
    }
}

#[tokio::main]
async fn main() {
    run_server().await
}
@XOR-op XOR-op added the C-bug Category: bug. Something is wrong. This is bad! label Jun 16, 2023
@XOR-op XOR-op changed the title Chunked HTTP/1.1 doesn't send zero frame when body is empty Chunked HTTP/1.1 doesn't send the last zero-chunk when body is empty Jun 16, 2023
@seanmonstar
Copy link
Member

Oh, interesting. This is supposed to work, and we have a matrix of response_body_lengths tests for a bunch of combinations (headers existing, body type knows it's own length, etc), and I noticed this specific combination wasn't included (chunked header set, known empty). Adding it, and the test does indeed hang. I'll look into why.

@seanmonstar seanmonstar mentioned this issue Jun 16, 2023
Merged
@seanmonstar seanmonstar self-assigned this Jun 16, 2023
@seanmonstar
Copy link
Member

Ok, I've got a fix up in #3254, after it merges, I'll backport it to v0.14.x.

@XOR-op
Copy link
Author

XOR-op commented Jun 17, 2023

Thanks for your quick fix! I'll wait for it to merge.

seanmonstar added a commit that referenced this issue Jun 19, 2023
@seanmonstar
fix(http1): properly end chunked bodies when it was known to be empty (…
fec64cf 
…#3254)

Closes #3252
seanmonstar added a commit that referenced this issue Jun 19, 2023
@seanmonstar
fix(http1): properly end chunked bodies when it was known to be empty (…
91d4db7 
…#3254)

Closes #3252
seanmonstar added a commit that referenced this issue Jun 20, 2023
@seanmonstar
fix(http1): properly end chunked bodies when it was known to be empty (…
32422c4 
…#3254)

Closes #3252
mmastrac added a commit to denoland/deno that referenced this issue Jul 31, 2023
@mmastrac
feat(ext/http): Upgrade to hyper1.0-rc4 (#19987)
990ecc9 
Includes a lightly-modified version of hyper-util's `TokioIo` utility. 

Hyper changes:

v1.0.0-rc.4 (2023-07-10)
Bug Fixes

    http1:
http1 server graceful shutdown fix (#3261)
([f4b51300](hyperium/hyper@f4b5130))
send error on Incoming body when connection errors (#3256)
([52f19259](hyperium/hyper@52f1925),
closes hyperium/hyper#3253)
properly end chunked bodies when it was known to be empty (#3254)
([fec64cf0](hyperium/hyper@fec64cf),
closes hyperium/hyper#3252)

Features

client: Make clients able to use non-Send executor (#3184)
([d977f209](hyperium/hyper@d977f20),
closes hyperium/hyper#3017)
    rt:
replace IO traits with hyper::rt ones (#3230)
([f9f65b7a](hyperium/hyper@f9f65b7),
closes hyperium/hyper#3110)
add downcast on Sleep trait (#3125)
([d92d3917](hyperium/hyper@d92d391),
closes hyperium/hyper#3027)
service: change Service::call to take &self (#3223)
([d894439e](hyperium/hyper@d894439),
closes hyperium/hyper#3040)

Breaking Changes

Any IO transport type provided must not implement hyper::rt::{Read,
Write} instead of tokio::io traits. You can grab a helper type from
hyper-util to wrap Tokio types, or implement the traits yourself, if
it's a custom type.
([f9f65b7a](hyperium/hyper@f9f65b7))
client::conn::http2 types now use another generic for an Executor. Code
that names Connection needs to include the additional generic parameter.
([d977f209](hyperium/hyper@d977f20))
The Service::call function no longer takes a mutable reference to self.
The FnMut trait bound on the service::util::service_fn function and the
trait bound on the impl for the ServiceFn struct were changed from FnMut
to Fn.
0xE282B0 pushed a commit to 0xE282B0/hyper that referenced this issue Jan 12, 2024
@seanmonstar @0xE282B0
fix(http1): properly end chunked bodies when it was known to be empty (…
56265f5 
…hyperium#3254)

Closes hyperium#3252
0xE282B0 pushed a commit to 0xE282B0/hyper that referenced this issue Jan 16, 2024
@seanmonstar @0xE282B0
fix(http1): properly end chunked bodies when it was known to be empty (…
6579a47 
…hyperium#3254)

Closes hyperium#3252

Signed-off-by: Sven Pfennig <s.pfennig@reply.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@seanmonstar seanmonstar

Labels
C-bug Category: bug. Something is wrong. This is bad!
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix(http1): properly end chunked bodies when it was known to be empty hyperium/hyper
2 participants
@seanmonstar @XOR-op