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 for invalid streamID #6940

Merged
merged 28 commits into from
Feb 20, 2024
Merged
Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
24816f2
add test with invalid client id
AnandInguva Jan 12, 2024
4300406
add test for invalid frame
AnandInguva Jan 24, 2024
71b7ee7
Check if the err is nil or not
AnandInguva Jan 24, 2024
9815dbc
Add comment for the test
AnandInguva Jan 24, 2024
3bd77df
Run the test against a single env
AnandInguva Jan 26, 2024
bee7003
Remove redundant comments
AnandInguva Jan 26, 2024
6b13297
Remove funcServer and try to assert the error
AnandInguva Feb 1, 2024
5fff887
Add test for illegal stream id on server operateHeader
AnandInguva Feb 1, 2024
ac91174
assert for io.EOF error
AnandInguva Feb 1, 2024
577a1b7
Address comments
AnandInguva Feb 6, 2024
f25503b
Fix assertion
AnandInguva Feb 6, 2024
c68ded3
Add back new line
AnandInguva Feb 6, 2024
3e863de
Send GoAwayFrame when illegal streamID is received
AnandInguva Feb 8, 2024
4338789
Remove comment
AnandInguva Feb 8, 2024
f1855d3
Merge remote-tracking branch 'upstream/master' into add_test
AnandInguva Feb 8, 2024
34dd481
Use servertester wantGoFrame for assertion
AnandInguva Feb 9, 2024
14ecbb8
Return error and add GoAwayFrame
AnandInguva Feb 9, 2024
cfa206b
Update test/end2end_test.go
AnandInguva Feb 9, 2024
4d3bc72
Add a comment on error case
AnandInguva Feb 9, 2024
26c215b
Refactor test
AnandInguva Feb 9, 2024
fcff403
Break comment
AnandInguva Feb 10, 2024
ad742c3
Add space
AnandInguva Feb 12, 2024
7a1a6f8
Modify assertion
AnandInguva Feb 12, 2024
7bbea1f
Fix condition
AnandInguva Feb 12, 2024
fa61215
Add test for stream ID lower than previous frame's streamID
AnandInguva Feb 16, 2024
6b79110
Fix test
AnandInguva Feb 16, 2024
a1d33da
rewrite error message
AnandInguva Feb 16, 2024
3522f84
Update error message
AnandInguva Feb 17, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions internal/transport/http2_server.go
Original file line number Diff line number Diff line change
Expand Up @@ -642,8 +642,12 @@ func (t *http2Server) HandleStreams(ctx context.Context, handle func(*Stream)) {
switch frame := frame.(type) {
case *http2.MetaHeadersFrame:
if err := t.operateHeaders(ctx, frame, handle); err != nil {
t.Close(err)
break
t.controlBuf.put(&goAway{
AnandInguva marked this conversation as resolved.
Show resolved Hide resolved
code: http2.ErrCodeProtocol,
debugData: []byte(err.Error()),
closeConn: err,
})
continue
}
case *http2.DataFrame:
t.handleData(frame)
Expand Down
32 changes: 32 additions & 0 deletions test/end2end_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3901,6 +3901,38 @@ func (s) TestClientRequestBodyErrorCloseAfterLength(t *testing.T) {
}
}

// Tests gRPC server's behavior when a gRPC client sends a frame with an invalid
// streamID. Per [HTTP/2 spec]: Streams initiated by a client MUST use
// odd-numbered stream identifiers. This test sets up a test server and send a
AnandInguva marked this conversation as resolved.
Show resolved Hide resolved
// header frame with stream ID of 2. The test asserts that a subsequent read on
// the transport throws an error.
//
// [HTTP/2 spec]: https://httpwg.org/specs/rfc7540.html#StreamIdentifiers
func (s) TestClientInvalidStreamID(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

Since we have this test now, and if we don't already have tests for other invalid stream ID conditions (e.g. a lower ID than the previous, or a stream ID of zero for the first stream), then now might be a good time to add related test cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will take a look today and add if tests are not present.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

stream ID of zero for the first stream

For this case, the http2/frame itself throws an error when we write frames of streamID 0 using serverTester. So I am guessing we can't do this test end to end and ask for question if we need this test at all?

Copy link
Member

Choose a reason for hiding this comment

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

It would still be nice to have a test for this, but I'm fine with punting for now if it's difficult to write. Presumably this is a different code path on the server (the framer may error reading the frame), so a test would still be a good idea to have one day.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added one more test. PTAL

lis, err := net.Listen("tcp", "localhost:0")
if err != nil {
t.Fatalf("Failed to listen: %v", err)
}
defer lis.Close()
s := grpc.NewServer()
defer s.Stop()
go s.Serve(lis)

conn, err := net.DialTimeout("tcp", lis.Addr().String(), defaultTestTimeout)
if err != nil {
t.Fatalf("Failed to dial: %v", err)
}
st := newServerTesterFromConn(t, conn)
st.greet()
st.writeHeadersGRPC(2, "/grpc.testing.TestService/StreamingInputCall", true)
goAwayFrame := st.wantGoAway(http2.ErrCodeProtocol)
got := string(goAwayFrame.DebugData()[:])
AnandInguva marked this conversation as resolved.
Show resolved Hide resolved
want := "received an illegal stream id: 2. headers frame: [FrameHeader HEADERS flags=END_STREAM|END_HEADERS stream=2 len=60]"
AnandInguva marked this conversation as resolved.
Show resolved Hide resolved
if got != want {
Copy link
Member

Choose a reason for hiding this comment

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

If you want to be fancy:

if got, want := string(goAwayFrame.DebugData()), "received ..."; got != want {
}

t.Fatalf("Error received: %v, want: %v", got, want)
}
}

func testClientRequestBodyErrorCloseAfterLength(t *testing.T, e env) {
te := newTest(t, e)
te.declareLogNoise("Server.processUnaryRPC failed to write status")
Expand Down
Loading