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

test: add test of malformed gzip payload #3141

Merged
merged 5 commits into from Nov 5, 2019
Merged

Conversation

dfawley
Copy link
Member

@dfawley dfawley commented Nov 5, 2019

Adds a regression test inspired by #3135.

Fixes #3135

}
err := gzw.Close()
bs := buf.Bytes()
bs[len(bs)-4] -= 1 // modify checksum (big endian) at end by 1 byte
Copy link
Contributor

Choose a reason for hiding this comment

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

lint: should replace bs[len(bs)-4] -= 1 with bs[len(bs)-4]--

Copy link
Member Author

Choose a reason for hiding this comment

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

Done


p, err := newPayload(testpb.PayloadType_COMPRESSABLE, int32(1024))
if err != nil {
panic(err)
Copy link
Contributor

Choose a reason for hiding this comment

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

t.Fatalf

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

if err != nil {
panic(err)
}
if _, err := ss.client.UnaryCall(ctx, &testpb.SimpleRequest{Payload: p}); err == nil || status.Code(err) != codes.Internal {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we expect the error to contain gzip error message (gzip.ErrChecksum https://golang.org/pkg/compress/gzip/#pkg-variables)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

return "gzip"
}

func (s) TestGzipWithoutFooter(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is footer same as checksum?
And is this without footer or with invalid footer?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. I was originally planning to remove it entirely, but that caused a different error to occur. (We read the last 4 bytes as the size before decoding and detect a message that would be too large upon decompression, so signal ResourceExhausted.) I changed the test to modify the checksum, which is in the previous 4 bytes of the footer.

test/end2end_test.go Show resolved Hide resolved
bs := buf.Bytes()
bs[len(bs)-4] -= 1 // modify checksum (big endian) at end by 1 byte
buf = bytes.NewBuffer(bs)
buf.WriteTo(w)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this same as w.Write(bs)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, that's simpler, thanks.

Copy link
Member Author

@dfawley dfawley left a comment

Choose a reason for hiding this comment

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

Github appears to be having problems. I've addressed all comments. PTAL

@dfawley dfawley merged commit 6dac020 into grpc:master Nov 5, 2019
@dfawley dfawley deleted the gzipcsum branch November 5, 2019 19:11
@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 this pull request may close these issues.

Go should verify gziped messages are correctly terminated
2 participants