Repository navigation
wsflate: handle short compressor EOF blocks - #227
sb123sb123 wants to merge 3 commits into
Conversation
| w.flushed = false | ||
| w.err = w.c.Flush() | ||
| w.checkTail() | ||
| if w.err == nil { |
There was a problem hiding this comment.
w.flushed = w.err == nil ?
|
Thanks for the review and approval. The Go CI matrix for this head is green. The separate Autobahn workflow fails while building its test image, before the Autobahn suite starts: installing Failing run: https://github.com/gobwas/ws/actions/runs/36542593276 |
|
Hey, @sb123sb123, thanks for the fix! If you don't mind, I'll push onto your branch a slight adjustment: Close() will Flush() first when the writer hasn't been flushed, the way flate/gzip/zlib Writers do (and then close the compressor into the discard, as you do)? That would cover close-without-flush from #221 as well, and makes checkTail live only in Flush(). The Autobahn CI failure is unrelated: the test image is outdated, I'll fix that in master soon. |
|
@prattmic @cristaloleg @fxfactorial kindly asking you to review this guys so we can 🚀 it :) |
bac48a2 to
3aad923
Compare
A PMCE message is complete once the compressor has been sync flushed and the trailing 0x00 0x00 0xff 0xff stripped (RFC 7692, 7.2.1). Whatever a compressor emits when it is closed (a final DEFLATE block) is not part of the message, and its exact bytes are an implementation detail: Go 1.26 emitted an empty stored block ending in the same four bytes as a sync flush, so dropping the last four bytes after Close() happened to work; Go 1.27 emits a short fixed-Huffman block, which broke the tail check. Build on the previous commit, which keeps the compressor's Close() output out of the message when Close() follows Flush(), and extend it to the other call sequences: - Close() flushes first unless nothing was written since the last successful Flush(), the way flate, gzip and zlib Writers do. This covers callers that finish a message with a bare Close(), including the autobahn example server. - Close() then always closes the compressor into io.Discard, so the tail check runs only in Flush() and does not depend on how a particular compressor terminates its stream. Flush(), Close() and Flush()+Close() now produce byte-identical messages with compress/flate from both Go 1.26 and Go 1.27, which the new tests assert; the Autobahn suite passes against a Go 1.27 server. Fixes gobwas#221.
3aad923 to
738dc65
Compare
Fixes #221
Problem
wsflate.Writerassumes that a compressor'sCloseleaves the four-byte stored-block tail (00 00 ff ff) in its buffer. Go 1.27'scompress/flateemits a short fixed-Huffman EOF block instead, so closing after the required PMCE flush can move buffered bytes to the destination and return a falsewsflate: bad compressorerror.Cause
The PMCE sync-flush marker and the compressor's final EOF block are different protocol concerns, but
Closechecked and forwarded them through the same four-byte tail buffer.Fix
Track whether the last successful operation was
Flush. When closing a flushed writer, still call the compressor'sClosemethod, but discard only its close-time output so the PMCE message remains free of the final block. A subsequentWriteclears the state and requires a new flush.The regression test models the short fixed-Huffman EOF block and verifies that the compressor is closed, no error is reported, and the flushed payload is unchanged.
Tests
go test -count=1 -run '^TestWriterCloseAfterFlushWithShortFinalBlock$' ./wsflate(passed with task-local dependency replacements)go test ./...(passed with task-local dependency replacements)go vet ./wsflate(passed)gofmt -d wsflate/writer.go wsflate/writer_test.go(clean)git diff --check(clean)Limitations
The remote Windows host runs Go 1.26.8, so it could not execute the Go 1.27 compressor directly. The Go module proxy and direct module fetches were unavailable; I used temporary local copies of the two small dependencies for testing and removed them before committing. Full-module vet also reports two pre-existing findings outside this change:
wsutil/writer_test.go:135(reflect.SliceHeader) andexample/autobahn/autobahn.go:58(discardedcontext.WithTimeoutcancel function). The race test was not run because the host has nogccC compiler.AI assistance disclosure
This PR was prepared with assistance from OpenAI Codex. Codex performed issue triage, implementation, regression-test authoring, and validation; the final patch and reported results were checked against the repository and command output.