Skip to content

Commit

Permalink
fix handling of unknown frames in the stream hijacker
Browse files Browse the repository at this point in the history
  • Loading branch information
marten-seemann committed May 27, 2022
1 parent 3088865 commit 5cb2e82
Show file tree
Hide file tree
Showing 4 changed files with 4 additions and 8 deletions.
3 changes: 1 addition & 2 deletions http3/frames.go
Expand Up @@ -33,11 +33,10 @@ func parseNextFrame(r io.Reader, unknownFrameHandler unknownFrameHandlerFunc) (f
if err != nil {
return nil, err
}
// If the unknownFrameHandler didn't process the frame, it is our responsibility to skip it.
if hijacked {
return nil, errHijacked
}
continue
// If the unknownFrameHandler didn't process the frame, it is our responsibility to skip it.

This comment has been minimized.

Copy link
@MarcoPolo

MarcoPolo May 27, 2022

Collaborator

I don't follow how removing the continue means we're skipping the frame? @marten-seemann

This comment has been minimized.

Copy link
@MarcoPolo

MarcoPolo May 27, 2022

Collaborator

ah I see, the skip happens later in this file. Great 👍

This comment has been minimized.

Copy link
@marten-seemann

marten-seemann May 27, 2022

Author Member

HTTP/3 frames are TLV. We've read the type, then called the hijacker callback. If the hijacker looks at the type and decides it doesn't want to handle this frame, we still need to read the length and the value of the frame.

This is very likely to happen in practice, since most implementations send greasing frames (frames that have no meaning and are just meant to be skipped over, introduced to uncover bugs in the parsing logic).

}
l, err := quicvarint.Read(qr)
if err != nil {
Expand Down
5 changes: 1 addition & 4 deletions http3/frames_test.go
Expand Up @@ -206,6 +206,7 @@ var _ = Describe("Frames", func() {
buf := &bytes.Buffer{}
quicvarint.Write(buf, 1337)
customFrameContents := []byte("custom frame")
quicvarint.Write(buf, uint64(len(customFrameContents)))
buf.Write(customFrameContents)
(&dataFrame{Length: 6}).Write(buf)
buf.WriteString("foobar")
Expand All @@ -214,10 +215,6 @@ var _ = Describe("Frames", func() {
frame, err := parseNextFrame(buf, func(ft FrameType) (hijacked bool, err error) {
Expect(ft).To(BeEquivalentTo(1337))
called = true
b := make([]byte, len(customFrameContents))
_, err = io.ReadFull(buf, b)
Expect(err).ToNot(HaveOccurred())
Expect(string(b)).To(Equal(string(customFrameContents)))
return false, nil
})
Expect(err).ToNot(HaveOccurred())
Expand Down
2 changes: 1 addition & 1 deletion http3/roundtrip.go
Expand Up @@ -53,7 +53,7 @@ type RoundTripper struct {

// When set, this callback is called for the first unknown frame parsed on a bidirectional stream.
// It is called right after parsing the frame type.
// Callers can either process the frame and return control of the stream back to HTTP/3
// Callers can either ignore the frame and return control of the stream back to HTTP/3
// (by returning hijacked false).
// Alternatively, callers can take over the QUIC stream (by returning hijacked true).
StreamHijacker func(FrameType, quic.Connection, quic.Stream) (hijacked bool, err error)
Expand Down
2 changes: 1 addition & 1 deletion http3/server.go
Expand Up @@ -178,7 +178,7 @@ type Server struct {

// StreamHijacker, when set, is called for the first unknown frame parsed on a bidirectional stream.
// It is called right after parsing the frame type.
// Callers can either process the frame and return control of the stream back to HTTP/3
// Callers can either ignore the frame and return control of the stream back to HTTP/3
// (by returning hijacked false).
// Alternatively, callers can take over the QUIC stream (by returning hijacked true).
StreamHijacker func(FrameType, quic.Connection, quic.Stream) (hijacked bool, err error)
Expand Down

0 comments on commit 5cb2e82

Please sign in to comment.