Ensure body reads only return 0-length at the end #483
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This demonstrates the underlying issue in #479. The origin writes maliciously-sized chunks - zero-length for the uncompressed body, two bytes at a time for the compressed body - and the test fixture asserts that the expected data are read. This passes for the uncompressed body (yay!); without #480, this fails for the uncompressed body (i.e. it's a reproducer for #479).
A more general set of tests for #479. Assume that the origin can send chunks of arbitrary length - in the case of #479, a gzip header in a separate chunk from the content. These show up as separate chunks in a `hyper::Body`. Ensure that, when splitting a body at arbitrary chunk lengths, the output still reproduces the body. This causes failures without #480 (or similar changes). This validates the fix in #480 (the gzip test fails without #480).
Issue 479 has more details on the "zero-length chunk" issue. It comes from an API divergence between `http::Body`, which has an explicit "end of stream" signal, and `std::io::Read` (and its async analogues), which use "zero bytes read" to indicate an end-of-stream. Providing a zero-length chunk results in confusion when moving from the HTTP API to the Read API, as we do in `fastly_http_body::read`. Modeled after the code in Compute Platform, here we introduce an analogue to the AsyncRead traits. The helper `check_body_read` shows how to safely use this to read the whole body, as with AsyncRead users. The HTTP body read hostcalls are updated to use the new reader, hiding details of `Body` and sending them through the happy path. This is an alternative fix for #479, that should cover any lingering bugs in existing/future `Body` implementations as well.
acme
approved these changes
May 29, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Tests and a slightly broader preventative measure for #479.
The Rust
Readtrait uses a return value ofOk(0)to indicate a (permanent/temporary) end-of-stream, e.g. end-of-file.We use the same convention for the
fastly_http_body::readhostcall.The
Bodytrait is a little more careful, with a separateis_stream_endmethod distinct from the "length of frame" property.However, this means if our
Bodyimplementations ever yield a zero-length chunk, thefastly_http_body::readhostcall will look like an end-of-stream to the guest. That's at the root of #479.#480 addresses the observed cause- that the gzip decoder in particular can yield zero-length chunks when the header + body are split. However, it's in principle possible for another (current or future) variant of
Bodyto reintroduce the issue.http::Bodystream-style API to thestd::io::Read-style API. Add aBody::readmethod that acts more like theAsyncReadtraits; use it from the hostcalls, instead of puttingBodyinternals in those hostcalls.Fixed #479 (to my satisfaction)