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

Go should verify gziped messages are correctly terminated #3135

Closed
ejona86 opened this issue Oct 31, 2019 · 5 comments · Fixed by #3141
Closed

Go should verify gziped messages are correctly terminated #3135

ejona86 opened this issue Oct 31, 2019 · 5 comments · Fixed by #3141
Assignees

Comments

@ejona86
Copy link
Member

ejona86 commented Oct 31, 2019

Gzip has a footer containing an adler32 (think CRC32). grpc-dotnet recently introduced a bug where this was missing (grpc/grpc#20884). However, the Go client did not fail. The gzip decoder should have failed with ErrChecksum. It's unclear why this wasn't reported to the client, but seems like a bug. (I poked around a bit and rpc_util.go's decompress() looked fine. The gzip decompressor itself didn't seem to have much to it.)

@dfawley
Copy link
Member

dfawley commented Nov 1, 2019

Looks like we do not call Close on the decompressor, which is when it would check the checksum.

@dfawley dfawley self-assigned this Nov 1, 2019
@dfawley dfawley added the P2 label Nov 1, 2019
@dfawley
Copy link
Member

dfawley commented Nov 1, 2019

Actually, the decompressor does check when reading: https://golang.org/src/compress/zlib/reader.go#102

This may be related to a recent change that makes us allocate and read only exactly the size of the decompressed message. #3048

@dfawley
Copy link
Member

dfawley commented Nov 1, 2019

@ejona86 are you sure this doesn't already fail as expected? I tried adding a test that uses a custom compressor which slightly modifies the final 4 bytes, and I see the following result:

--- FAIL: Test/GzipWithoutFooter (0.00s)
        end2end_test.go:7569: ss.client.UnaryCall(_) = _, rpc error: code = Internal desc = grpc: failed to decompress the received message gzip: invalid checksum; want _, nil

@ejona86
Copy link
Member Author

ejona86 commented Nov 4, 2019

It clearly didn't fail: https://source.cloud.google.com/results/invocations/371fa7e2-b35d-4895-a2a8-93da03e475e2/targets/github%2Fgrpc%2Finterop_test/tests

As I said, I poked at Go's code and it looked correct. Digging deeper, I filtered the test results for "go:aspnetcore_server:server_compressed" from that run and didn't see any results. It appears compression interop is (accidentally?) disabled for Go: https://github.com/grpc/grpc/blob/461510571cf7a694f166f8d9a0f87ad86e39afdb/tools/run_tests/run_interop_tests.py#L335

It also appears the server-side compression tests are wrongly disabled for Java and Go.

@ejona86
Copy link
Member Author

ejona86 commented Nov 6, 2019

@dfawley, your change didn't really address my concern that there was no interop testing that would detect this failure. Thanks for pointing out #1357; it seems that is the cause.

@lock lock bot locked as resolved and limited conversation to collaborators May 5, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants