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 deflate algorithm to adhere to RFC 1950 #824

Merged
merged 5 commits into from Mar 13, 2024

Conversation

jchadwick-buf
Copy link
Contributor

@jchadwick-buf jchadwick-buf commented Mar 12, 2024

We don't default to testing deflate right now, as far as I can tell, but this is a pretty potent footgun so I think it is definitely worth getting right.

Though it is not explicitly specified anywhere I can find, gRPC seems to follow HTTP in treating deflate as RFC 1950 (zlib) instead of RFC 1951 (deflate).

The deflateInit2 function's documentation can be found online here. But briefly:

  • If windowBits is [-15, -8], raw deflate (RFC 1951) is used.
  • If windowBits is [8, 15], deflate with zlib header (RFC 1950) is used.
  • If windowBits is [24, 31], deflate with gzip header (RFC 1952) is used.

Following this, we can see that gRPC will use either gzip (for the gzip encoding) or zlib (for the deflate encoding.)

zlib.Reader eagerly reads the zlib header, so it cannot be constructed without a reader. Instead of using Reset, just defer constructing the reader.
Copy link
Member

@jhump jhump left a comment

Choose a reason for hiding this comment

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

I think it would be nice to add minor safeguards against nil-dereference panics.

But otherwise, LGTM!

internal/compression/deflate.go Outdated Show resolved Hide resolved
internal/compression/deflate.go Show resolved Hide resolved
@jhump jhump enabled auto-merge (squash) March 13, 2024 17:11
@jhump jhump merged commit 4c078c7 into connectrpc:main Mar 13, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants