Repository navigation
fix(http2,http3): reconcile a streamed body against its declared Content-Length - #348
Merged
Merged
Conversation
…ent-Length `request.nim` reconciled `respContentLength` against `respBodyWritten` only on the HTTP/1 path (#248), forcing `closeAfterFlush` on a mismatch so the peer at least sees an incomplete read. The h2 and h3 branches of sendHead / write / finish returned before that accounting, so a streamed body that ended short of the Content-Length its head already declared left as a well-formed END_STREAM (or FIN): a truncation only the client could notice, which it can report only as a protocol error of ours (h2's InvalidBodyLengthError), while the server logged nothing and went on serving. That is how the 2026-09-26 stress soak's truncated static-file read (d14f557) reached the client as a complete 1 GiB download. Give both codecs the guard HTTP/1 has. H2Stream and H3Stream each gain respDeclaredLen / respBodyWritten: h2SendHead / h3SendHead take the length the caller already put in the head (remembered, not re-encoded) and zero the counter, h2StreamWrite / h3StreamWrite count every chunk they accept, and h2StreamFinish / h3StreamFinish compare the two. A body short of, or past, its declared length is now RESET (RST_STREAM / RESET_STREAM, INTERNAL_ERROR) instead of terminated cleanly at a length the head contradicts, so the cut-short transfer is visible as the server's error. Compression already drops the declared length (the compressed size is unknown), and HEAD -- which declares a length and writes no body by design -- stays exempt: its stream closes at the head, before finish can reconcile anything. h2StreamAbort / h3StreamAbort move above finish so the mismatch path reuses the existing abort rather than repeating its teardown. The tests pin the fix in both directions and fail without it: over h2, a short and an over-long body each end in RST_STREAM(INTERNAL_ERROR) with no END_STREAM, an exact-length body still completes, and HEAD with a declared length is not reset; over h3, curl reports the reset (asserting the diagnostic, not just a non-zero exit, since the short read alone already failed the transfer).
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Closes #345.
request.nimreconciledrespContentLengthagainstrespBodyWrittenonly on the HTTP/1 path (#248), forcingcloseAfterFlushon a mismatch so the peer at least sees an incomplete read. The h2 and h3 branches ofsendHead/write/finishreturned before that accounting, so a streamed body that ended short of the Content-Length its head already declared left as a well-formed END_STREAM (or FIN): a truncation only the client could notice, and only as a protocol error of ours (the client's h2 stack raisedInvalidBodyLengthError), while the server logged nothing and went on serving. That is how the 2026-09-26 stress soak's truncated static-file read (d14f557, onfix/stress-all-round1) reached the client as a complete 1 GiB download.The guard
H2StreamandH3Streameach gainrespDeclaredLen/respBodyWritten:h2SendHead/h3SendHeadtake the length the caller already put in the head (remembered, not re-encoded) and zero the counter;h2StreamWrite/h3StreamWritecount every chunk they accept;h2StreamFinish/h3StreamFinishcompare the two, and a body short of (or past) its declared length is RESET, RST_STREAM / RESET_STREAM with INTERNAL_ERROR, instead of terminated cleanly at a length the head contradicts.Compression already drops the declared length (the compressed size is unknown), so a compressed stream is never reconciled. HEAD, which declares a length and writes no body by design, stays exempt: its stream closes at the head, before
finishcan reconcile anything.h2StreamAbort/h3StreamAbortmoved abovefinishso the mismatch path reuses the existing abort rather than repeating its teardown.This is the general net d14f557 asked for: that commit fixed the file streamer's own accounting, this one catches any future short streamed body on h2/h3, from any producer.
Tests
Each new test was verified to fail with the guard removed:
tests/test_http2_download.nim: a short body and an over-long body each end in RST_STREAM(INTERNAL_ERROR) with no END_STREAM; an exact-length body still completes with END_STREAM; HEAD with a declared length is not reset.fetchBodynow records the RST error code and a HEADERS-borne END_STREAM, and takes a request verb.tests/test_http3.nim: a short body makes curl report the stream reset (the diagnostic is asserted, not just a non-zero exit, since the short read alone already failed the transfer before the fix); an exact-length body completes.Full local suite: 78 suites, no failures (the usual compression suites skip without
-d:httpGzip/-d:httpBrotli/-d:httpZstd).Also documents the behavior in the README's Download section and adds a CHANGELOG entry under Unreleased/Fixed.