Skip to content

Commit

Permalink
http2: wire up Transport and Server's CountError to frame parser code
Browse files Browse the repository at this point in the history 
So all frame parse errors are annotated and flow into the transport
or server's error counters.

Change-Id: I1e287fe33d422f97075029201976b009218852da
Reviewed-on: https://go-review.googlesource.com/c/net/+/352649
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
Trust: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Damien Neil <dneil@google.com>
  • Loading branch information
@bradfitz
bradfitz committed Sep 28, 2021
1 parent 4e4d966 commit 7d9f5e0
Show file tree
Hide file tree
Showing 3 changed files with 53 additions and 13 deletions.
60 changes: 47 additions & 13 deletions http2/frame.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ var flagName = map[FrameType]map[Flags]string{
// a frameParser parses a frame given its FrameHeader and payload
// bytes. The length of payload will always equal fh.Length (which
// might be 0).
type frameParser func(fc *frameCache, fh FrameHeader, payload []byte) (Frame, error)
type frameParser func(fc *frameCache, fh FrameHeader, countError func(string), payload []byte) (Frame, error)

var frameParsers = map[FrameType]frameParser{
FrameData: parseDataFrame,
Expand Down Expand Up @@ -267,6 +267,11 @@ type Framer struct {
lastFrame Frame
errDetail error

// countError is a non-nil func that's called on a frame parse
// error with some unique error path token. It's initialized
// from Transport.CountError or Server.CountError.
countError func(errToken string)

// lastHeaderStream is non-zero if the last frame was an
// unfinished HEADERS/CONTINUATION.
lastHeaderStream uint32
Expand Down Expand Up @@ -426,6 +431,7 @@ func NewFramer(w io.Writer, r io.Reader) *Framer {
fr := &Framer{
w: w,
r: r,
countError: func(string) {},
logReads: logFrameReads,
logWrites: logFrameWrites,
debugReadLoggerf: log.Printf,
Expand Down Expand Up @@ -500,7 +506,7 @@ func (fr *Framer) ReadFrame() (Frame, error) {
if _, err := io.ReadFull(fr.r, payload); err != nil {
return nil, err
}
f, err := typeFrameParser(fh.Type)(fr.frameCache, fh, payload)
f, err := typeFrameParser(fh.Type)(fr.frameCache, fh, fr.countError, payload)
if err != nil {
if ce, ok := err.(connError); ok {
return nil, fr.connError(ce.Code, ce.Reason)
Expand Down Expand Up @@ -588,13 +594,14 @@ func (f *DataFrame) Data() []byte {
return f.data
}

func parseDataFrame(fc *frameCache, fh FrameHeader, payload []byte) (Frame, error) {
func parseDataFrame(fc *frameCache, fh FrameHeader, countError func(string), payload []byte) (Frame, error) {
if fh.StreamID == 0 {
// DATA frames MUST be associated with a stream. If a
// DATA frame is received whose stream identifier
// field is 0x0, the recipient MUST respond with a
// connection error (Section 5.4.1) of type
// PROTOCOL_ERROR.
countError("frame_data_stream_0")
return nil, connError{ErrCodeProtocol, "DATA frame with stream ID 0"}
}
f := fc.getDataFrame()
Expand All @@ -605,6 +612,7 @@ func parseDataFrame(fc *frameCache, fh FrameHeader, payload []byte) (Frame, erro
var err error
payload, padSize, err = readByte(payload)
if err != nil {
countError("frame_data_pad_byte_short")
return nil, err
}
}
Expand All @@ -613,6 +621,7 @@ func parseDataFrame(fc *frameCache, fh FrameHeader, payload []byte) (Frame, erro
// length of the frame payload, the recipient MUST
// treat this as a connection error.
// Filed: https://github.com/http2/http2-spec/issues/610
countError("frame_data_pad_too_big")
return nil, connError{ErrCodeProtocol, "pad size larger than data payload"}
}
f.data = payload[:len(payload)-int(padSize)]
Expand Down Expand Up @@ -695,14 +704,15 @@ type SettingsFrame struct {
p []byte
}

func parseSettingsFrame(_ *frameCache, fh FrameHeader, p []byte) (Frame, error) {
func parseSettingsFrame(_ *frameCache, fh FrameHeader, countError func(string), p []byte) (Frame, error) {
if fh.Flags.Has(FlagSettingsAck) && fh.Length > 0 {
// When this (ACK 0x1) bit is set, the payload of the
// SETTINGS frame MUST be empty. Receipt of a
// SETTINGS frame with the ACK flag set and a length
// field value other than 0 MUST be treated as a
// connection error (Section 5.4.1) of type
// FRAME_SIZE_ERROR.
countError("frame_settings_ack_with_length")
return nil, ConnectionError(ErrCodeFrameSize)
}
if fh.StreamID != 0 {
Expand All @@ -713,14 +723,17 @@ func parseSettingsFrame(_ *frameCache, fh FrameHeader, p []byte) (Frame, error)
// field is anything other than 0x0, the endpoint MUST
// respond with a connection error (Section 5.4.1) of
// type PROTOCOL_ERROR.
countError("frame_settings_has_stream")
return nil, ConnectionError(ErrCodeProtocol)
}
if len(p)%6 != 0 {
countError("frame_settings_mod_6")
// Expecting even number of 6 byte settings.
return nil, ConnectionError(ErrCodeFrameSize)
}
f := &SettingsFrame{FrameHeader: fh, p: p}
if v, ok := f.Value(SettingInitialWindowSize); ok && v > (1<<31)-1 {
countError("frame_settings_window_size_too_big")
// Values above the maximum flow control window size of 2^31 - 1 MUST
// be treated as a connection error (Section 5.4.1) of type
// FLOW_CONTROL_ERROR.
Expand Down Expand Up @@ -832,11 +845,13 @@ type PingFrame struct {

func (f *PingFrame) IsAck() bool { return f.Flags.Has(FlagPingAck) }

func parsePingFrame(_ *frameCache, fh FrameHeader, payload []byte) (Frame, error) {
func parsePingFrame(_ *frameCache, fh FrameHeader, countError func(string), payload []byte) (Frame, error) {
if len(payload) != 8 {
countError("frame_ping_length")
return nil, ConnectionError(ErrCodeFrameSize)
}
if fh.StreamID != 0 {
countError("frame_ping_has_stream")
return nil, ConnectionError(ErrCodeProtocol)
}
f := &PingFrame{FrameHeader: fh}
Expand Down Expand Up @@ -872,11 +887,13 @@ func (f *GoAwayFrame) DebugData() []byte {
return f.debugData
}

func parseGoAwayFrame(_ *frameCache, fh FrameHeader, p []byte) (Frame, error) {
func parseGoAwayFrame(_ *frameCache, fh FrameHeader, countError func(string), p []byte) (Frame, error) {
if fh.StreamID != 0 {
countError("frame_goaway_has_stream")
return nil, ConnectionError(ErrCodeProtocol)
}
if len(p) < 8 {
countError("frame_goaway_short")
return nil, ConnectionError(ErrCodeFrameSize)
}
return &GoAwayFrame{
Expand Down Expand Up @@ -912,7 +929,7 @@ func (f *UnknownFrame) Payload() []byte {
return f.p
}

func parseUnknownFrame(_ *frameCache, fh FrameHeader, p []byte) (Frame, error) {
func parseUnknownFrame(_ *frameCache, fh FrameHeader, countError func(string), p []byte) (Frame, error) {
return &UnknownFrame{fh, p}, nil
}

Expand All @@ -923,8 +940,9 @@ type WindowUpdateFrame struct {
Increment uint32 // never read with high bit set
}

func parseWindowUpdateFrame(_ *frameCache, fh FrameHeader, p []byte) (Frame, error) {
func parseWindowUpdateFrame(_ *frameCache, fh FrameHeader, countError func(string), p []byte) (Frame, error) {
if len(p) != 4 {
countError("frame_windowupdate_bad_len")
return nil, ConnectionError(ErrCodeFrameSize)
}
inc := binary.BigEndian.Uint32(p[:4]) & 0x7fffffff // mask off high reserved bit
Expand All @@ -936,8 +954,10 @@ func parseWindowUpdateFrame(_ *frameCache, fh FrameHeader, p []byte) (Frame, err
// control window MUST be treated as a connection
// error (Section 5.4.1).
if fh.StreamID == 0 {
countError("frame_windowupdate_zero_inc_conn")
return nil, ConnectionError(ErrCodeProtocol)
}
countError("frame_windowupdate_zero_inc_stream")
return nil, streamError(fh.StreamID, ErrCodeProtocol)
}
return &WindowUpdateFrame{
Expand Down Expand Up @@ -988,7 +1008,7 @@ func (f *HeadersFrame) HasPriority() bool {
return f.FrameHeader.Flags.Has(FlagHeadersPriority)
}

func parseHeadersFrame(_ *frameCache, fh FrameHeader, p []byte) (_ Frame, err error) {
func parseHeadersFrame(_ *frameCache, fh FrameHeader, countError func(string), p []byte) (_ Frame, err error) {
hf := &HeadersFrame{
FrameHeader: fh,
}
Expand All @@ -997,28 +1017,33 @@ func parseHeadersFrame(_ *frameCache, fh FrameHeader, p []byte) (_ Frame, err er
// is received whose stream identifier field is 0x0, the recipient MUST
// respond with a connection error (Section 5.4.1) of type
// PROTOCOL_ERROR.
countError("frame_headers_zero_stream")
return nil, connError{ErrCodeProtocol, "HEADERS frame with stream ID 0"}
}
var padLength uint8
if fh.Flags.Has(FlagHeadersPadded) {
if p, padLength, err = readByte(p); err != nil {
countError("frame_headers_pad_short")
return
}
}
if fh.Flags.Has(FlagHeadersPriority) {
var v uint32
p, v, err = readUint32(p)
if err != nil {
countError("frame_headers_prio_short")
return nil, err
}
hf.Priority.StreamDep = v & 0x7fffffff
hf.Priority.Exclusive = (v != hf.Priority.StreamDep) // high bit was set
p, hf.Priority.Weight, err = readByte(p)
if err != nil {
countError("frame_headers_prio_weight_short")
return nil, err
}
}
if len(p)-int(padLength) < 0 {
countError("frame_headers_pad_too_big")
return nil, streamError(fh.StreamID, ErrCodeProtocol)
}
hf.headerFragBuf = p[:len(p)-int(padLength)]
Expand Down Expand Up @@ -1125,11 +1150,13 @@ func (p PriorityParam) IsZero() bool {
return p == PriorityParam{}
}

func parsePriorityFrame(_ *frameCache, fh FrameHeader, payload []byte) (Frame, error) {
func parsePriorityFrame(_ *frameCache, fh FrameHeader, countError func(string), payload []byte) (Frame, error) {
if fh.StreamID == 0 {
countError("frame_priority_zero_stream")
return nil, connError{ErrCodeProtocol, "PRIORITY frame with stream ID 0"}
}
if len(payload) != 5 {
countError("frame_priority_bad_length")
return nil, connError{ErrCodeFrameSize, fmt.Sprintf("PRIORITY frame payload size was %d; want 5", len(payload))}
}
v := binary.BigEndian.Uint32(payload[:4])
Expand Down Expand Up @@ -1172,11 +1199,13 @@ type RSTStreamFrame struct {
ErrCode ErrCode
}

func parseRSTStreamFrame(_ *frameCache, fh FrameHeader, p []byte) (Frame, error) {
func parseRSTStreamFrame(_ *frameCache, fh FrameHeader, countError func(string), p []byte) (Frame, error) {
if len(p) != 4 {
countError("frame_rststream_bad_len")
return nil, ConnectionError(ErrCodeFrameSize)
}
if fh.StreamID == 0 {
countError("frame_rststream_zero_stream")
return nil, ConnectionError(ErrCodeProtocol)
}
return &RSTStreamFrame{fh, ErrCode(binary.BigEndian.Uint32(p[:4]))}, nil
Expand All @@ -1202,8 +1231,9 @@ type ContinuationFrame struct {
headerFragBuf []byte
}

func parseContinuationFrame(_ *frameCache, fh FrameHeader, p []byte) (Frame, error) {
func parseContinuationFrame(_ *frameCache, fh FrameHeader, countError func(string), p []byte) (Frame, error) {
if fh.StreamID == 0 {
countError("frame_continuation_zero_stream")
return nil, connError{ErrCodeProtocol, "CONTINUATION frame with stream ID 0"}
}
return &ContinuationFrame{fh, p}, nil
Expand Down Expand Up @@ -1252,7 +1282,7 @@ func (f *PushPromiseFrame) HeadersEnded() bool {
return f.FrameHeader.Flags.Has(FlagPushPromiseEndHeaders)
}

func parsePushPromise(_ *frameCache, fh FrameHeader, p []byte) (_ Frame, err error) {
func parsePushPromise(_ *frameCache, fh FrameHeader, countError func(string), p []byte) (_ Frame, err error) {
pp := &PushPromiseFrame{
FrameHeader: fh,
}
Expand All @@ -1263,25 +1293,29 @@ func parsePushPromise(_ *frameCache, fh FrameHeader, p []byte) (_ Frame, err err
// with. If the stream identifier field specifies the value
// 0x0, a recipient MUST respond with a connection error
// (Section 5.4.1) of type PROTOCOL_ERROR.
countError("frame_pushpromise_zero_stream")
return nil, ConnectionError(ErrCodeProtocol)
}
// The PUSH_PROMISE frame includes optional padding.
// Padding fields and flags are identical to those defined for DATA frames
var padLength uint8
if fh.Flags.Has(FlagPushPromisePadded) {
if p, padLength, err = readByte(p); err != nil {
countError("frame_pushpromise_pad_short")
return
}
}

p, pp.PromiseID, err = readUint32(p)
if err != nil {
countError("frame_pushpromise_promiseid_short")
return
}
pp.PromiseID = pp.PromiseID & (1<<31 - 1)

if int(padLength) > len(p) {
// like the DATA frame, error out if padding is longer than the body.
countError("frame_pushpromise_pad_too_big")
return nil, ConnectionError(ErrCodeProtocol)
}
pp.headerFragBuf = p[:len(p)-int(padLength)]
Expand Down
3 changes: 3 additions & 0 deletions http2/server.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -411,6 +411,9 @@ func (s *Server) ServeConn(c net.Conn, opts *ServeConnOpts) {
sc.hpackEncoder = hpack.NewEncoder(&sc.headerWriteBuf)

fr := NewFramer(sc.bw, c)
if s.CountError != nil {
fr.countError = s.CountError
}
fr.ReadMetaHeaders = hpack.NewDecoder(initialHeaderTableSize, nil)
fr.MaxHeaderListSize = sc.maxHeaderListSize()
fr.SetMaxReadFrameSize(s.maxReadFrameSize())
Expand Down
3 changes: 3 additions & 0 deletions http2/transport.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -699,6 +699,9 @@ func (t *Transport) newClientConn(c net.Conn, singleUse bool) (*ClientConn, erro
cc.bw = bufio.NewWriter(stickyErrWriter{c, &cc.werr})
cc.br = bufio.NewReader(c)
cc.fr = NewFramer(cc.bw, cc.br)
if t.CountError != nil {
cc.fr.countError = t.CountError
}
cc.fr.ReadMetaHeaders = hpack.NewDecoder(initialHeaderTableSize, nil)
cc.fr.MaxHeaderListSize = t.maxHeaderListSize()

Expand Down

0 comments on commit 7d9f5e0

Please sign in to comment.