Skip to content

Commit 0cb26f7

Browse files
bradfitzbradfitz
committed
http2: move HEADERS/CONTINUATION order checking into Framer
Removes state machine complication and duplication out of Server & Transport and puts it into the Framer instead (where it's nicely tested). Also, for testing, start tracking the reason for errors. Later we'll use it in GOAWAY frames' debug data too. Change-Id: Ic933654a33edb62b4432c28fe09f7bfdb6f9b334 Reviewed-on: https://go-review.googlesource.com/18101 Reviewed-by: Blake Mizerany <[email protected]>
1 parent 5d0a0f8 commit 0cb26f7

File tree

5 files changed

+234
-59
lines changed

5 files changed

+234
-59
lines changed

‎http2/errors.go‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -75,3 +75,16 @@ func (e StreamError) Error() string {
7575
type goAwayFlowError struct{}
7676

7777
func (goAwayFlowError) Error() string { return "connection exceeded flow control window size" }
78+
79+
// connErrorReason wraps a ConnectionError with an informative error about why it occurs.
80+
81+
// Errors of this type are only returned by the frame parser functions
82+
// and converted into ConnectionError(ErrCodeProtocol).
83+
type connError struct {
84+
Code ErrCode
85+
Reason string
86+
}
87+
88+
func (e connError) Error() string {
89+
return fmt.Sprintf("http2: connection error: %v: %v", e.Code, e.Reason)
90+
}

‎http2/frame.go‎

Lines changed: 74 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -255,6 +255,11 @@ type Frame interface {
255255
type Framer struct {
256256
r io.Reader
257257
lastFrame Frame
258+
errReason string
259+
260+
// lastHeaderStream is non-zero if the last frame was an
261+
// unfinished HEADERS/CONTINUATION.
262+
lastHeaderStream uint32
258263

259264
maxReadSize uint32
260265
headerBuf [frameHeaderLen]byte
@@ -271,13 +276,19 @@ type Framer struct {
271276
wbuf []byte
272277

273278
// AllowIllegalWrites permits the Framer's Write methods to
274-
// write frames that do not conform to the HTTP/2 spec. This
279+
// write frames that do not conform to the HTTP/2 spec. This
275280
// permits using the Framer to test other HTTP/2
276281
// implementations' conformance to the spec.
277282
// If false, the Write methods will prefer to return an error
278283
// rather than comply.
279284
AllowIllegalWrites bool
280285

286+
// AllowIllegalReads permits the Framer's ReadFrame method
287+
// to return non-compliant frames or frame orders.
288+
// This is for testing and permits using the Framer to test
289+
// other HTTP/2 implementations' conformance to the spec.
290+
AllowIllegalReads bool
291+
281292
// TODO: track which type of frame & with which flags was sent
282293
// last. Then return an error (unless AllowIllegalWrites) if
283294
// we're in the middle of a header block and a
@@ -394,12 +405,65 @@ func (fr *Framer) ReadFrame() (Frame, error) {
394405
}
395406
f, err := typeFrameParser(fh.Type)(fh, payload)
396407
if err != nil {
408+
if ce, ok := err.(connError); ok {
409+
return nil, fr.connError(ce.Code, ce.Reason)
410+
}
411+
return nil, err
412+
}
413+
if err := fr.checkFrameOrder(f); err != nil {
397414
return nil, err
398415
}
399-
fr.lastFrame = f
400416
return f, nil
401417
}
402418

419+
// connError returns ConnectionError(code) but first
420+
// stashes away a public reason to the caller can optionally relay it
421+
// to the peer before hanging up on them. This might help others debug
422+
// their implementations.
423+
func (fr *Framer) connError(code ErrCode, reason string) error {
424+
fr.errReason = reason
425+
return ConnectionError(code)
426+
}
427+
428+
// checkFrameOrder reports an error if f is an invalid frame to return
429+
// next from ReadFrame. Mostly it checks whether HEADERS and
430+
// CONTINUATION frames are contiguous.
431+
func (fr *Framer) checkFrameOrder(f Frame) error {
432+
last := fr.lastFrame
433+
fr.lastFrame = f
434+
if fr.AllowIllegalReads {
435+
return nil
436+
}
437+
438+
fh := f.Header()
439+
if fr.lastHeaderStream != 0 {
440+
if fh.Type != FrameContinuation {
441+
return fr.connError(ErrCodeProtocol,
442+
fmt.Sprintf("got %s for stream %d; expected CONTINUATION following %s for stream %d",
443+
fh.Type, fh.StreamID,
444+
last.Header().Type, fr.lastHeaderStream))
445+
}
446+
if fh.StreamID != fr.lastHeaderStream {
447+
return fr.connError(ErrCodeProtocol,
448+
fmt.Sprintf("got CONTINUATION for stream %d; expected stream %d",
449+
fh.StreamID, fr.lastHeaderStream))
450+
}
451+
} else if fh.Type == FrameContinuation {
452+
return fr.connError(ErrCodeProtocol, fmt.Sprintf("unexpected CONTINUATION for stream %d", fh.StreamID))
453+
}
454+
455+
switch fh.Type {
456+
case FrameHeaders, FrameContinuation:
457+
if fh.Flags.Has(FlagHeadersEndHeaders) {
458+
fr.lastHeaderStream = 0
459+
} else {
460+
fr.lastHeaderStream = fh.StreamID
461+
}
462+
}
463+
464+
return nil
465+
}
466+
403467
// A DataFrame conveys arbitrary, variable-length sequences of octets
404468
// associated with a stream.
405469
// See http://http2.github.io/http2-spec/#rfc.section.6.1
@@ -428,7 +492,7 @@ func parseDataFrame(fh FrameHeader, payload []byte) (Frame, error) {
428492
// field is 0x0, the recipient MUST respond with a
429493
// connection error (Section 5.4.1) of type
430494
// PROTOCOL_ERROR.
431-
return nil, ConnectionError(ErrCodeProtocol)
495+
return nil, connError{ErrCodeProtocol, "DATA frame with stream ID 0"}
432496
}
433497
f := &DataFrame{
434498
FrameHeader: fh,
@@ -446,7 +510,7 @@ func parseDataFrame(fh FrameHeader, payload []byte) (Frame, error) {
446510
// length of the frame payload, the recipient MUST
447511
// treat this as a connection error.
448512
// Filed: https://github.com/http2/http2-spec/issues/610
449-
return nil, ConnectionError(ErrCodeProtocol)
513+
return nil, connError{ErrCodeProtocol, "pad size larger than data payload"}
450514
}
451515
f.data = payload[:len(payload)-int(padSize)]
452516
return f, nil
@@ -753,7 +817,7 @@ func parseHeadersFrame(fh FrameHeader, p []byte) (_ Frame, err error) {
753817
// is received whose stream identifier field is 0x0, the recipient MUST
754818
// respond with a connection error (Section 5.4.1) of type
755819
// PROTOCOL_ERROR.
756-
return nil, ConnectionError(ErrCodeProtocol)
820+
return nil, connError{ErrCodeProtocol, "HEADERS frame with stream ID 0"}
757821
}
758822
var padLength uint8
759823
if fh.Flags.Has(FlagHeadersPadded) {
@@ -883,10 +947,10 @@ func (p PriorityParam) IsZero() bool {
883947

884948
func parsePriorityFrame(fh FrameHeader, payload []byte) (Frame, error) {
885949
if fh.StreamID == 0 {
886-
return nil, ConnectionError(ErrCodeProtocol)
950+
return nil, connError{ErrCodeProtocol, "PRIORITY frame with stream ID 0"}
887951
}
888952
if len(payload) != 5 {
889-
return nil, ConnectionError(ErrCodeFrameSize)
953+
return nil, connError{ErrCodeFrameSize, fmt.Sprintf("PRIORITY frame payload size was %d; want 5", len(payload))}
890954
}
891955
v := binary.BigEndian.Uint32(payload[:4])
892956
streamID := v & 0x7fffffff // mask off high bit
@@ -956,6 +1020,9 @@ type ContinuationFrame struct {
9561020
}
9571021

9581022
func parseContinuationFrame(fh FrameHeader, p []byte) (Frame, error) {
1023+
if fh.StreamID == 0 {
1024+
return nil, connError{ErrCodeProtocol, "CONTINUATION frame with stream ID 0"}
1025+
}
9591026
return &ContinuationFrame{fh, p}, nil
9601027
}
9611028

‎http2/frame_test.go‎

Lines changed: 138 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,8 @@ package http2
66

77
import (
88
"bytes"
9+
"fmt"
10+
"io"
911
"reflect"
1012
"strings"
1113
"testing"
@@ -264,6 +266,7 @@ func TestWriteContinuation(t *testing.T) {
264266
t.Errorf("test %q: %v", tt.name, err)
265267
continue
266268
}
269+
fr.AllowIllegalReads = true
267270
f, err := fr.ReadFrame()
268271
if err != nil {
269272
t.Errorf("test %q: failed to read the frame back: %v", tt.name, err)
@@ -595,3 +598,138 @@ func TestWritePushPromise(t *testing.T) {
595598
t.Fatalf("parsed back:\n%#v\nwant:\n%#v", f, want)
596599
}
597600
}
601+
602+
// test checkFrameOrder and that HEADERS and CONTINUATION frames can't be intermingled.
603+
func TestReadFrameOrder(t *testing.T) {
604+
head := func(f *Framer, id uint32, end bool) {
605+
f.WriteHeaders(HeadersFrameParam{
606+
StreamID: id,
607+
BlockFragment: []byte("foo"), // unused, but non-empty
608+
EndHeaders: end,
609+
})
610+
}
611+
cont := func(f *Framer, id uint32, end bool) {
612+
f.WriteContinuation(id, end, []byte("foo"))
613+
}
614+
615+
tests := [...]struct {
616+
name string
617+
w func(*Framer)
618+
atLeast int
619+
wantErr string
620+
}{
621+
0: {
622+
w: func(f *Framer) {
623+
head(f, 1, true)
624+
},
625+
},
626+
1: {
627+
w: func(f *Framer) {
628+
head(f, 1, true)
629+
head(f, 2, true)
630+
},
631+
},
632+
2: {
633+
wantErr: "got HEADERS for stream 2; expected CONTINUATION following HEADERS for stream 1",
634+
w: func(f *Framer) {
635+
head(f, 1, false)
636+
head(f, 2, true)
637+
},
638+
},
639+
3: {
640+
wantErr: "got DATA for stream 1; expected CONTINUATION following HEADERS for stream 1",
641+
w: func(f *Framer) {
642+
head(f, 1, false)
643+
},
644+
},
645+
4: {
646+
w: func(f *Framer) {
647+
head(f, 1, false)
648+
cont(f, 1, true)
649+
head(f, 2, true)
650+
},
651+
},
652+
5: {
653+
wantErr: "got CONTINUATION for stream 2; expected stream 1",
654+
w: func(f *Framer) {
655+
head(f, 1, false)
656+
cont(f, 2, true)
657+
head(f, 2, true)
658+
},
659+
},
660+
6: {
661+
wantErr: "unexpected CONTINUATION for stream 1",
662+
w: func(f *Framer) {
663+
cont(f, 1, true)
664+
},
665+
},
666+
7: {
667+
wantErr: "unexpected CONTINUATION for stream 1",
668+
w: func(f *Framer) {
669+
cont(f, 1, false)
670+
},
671+
},
672+
8: {
673+
wantErr: "HEADERS frame with stream ID 0",
674+
w: func(f *Framer) {
675+
head(f, 0, true)
676+
},
677+
},
678+
9: {
679+
wantErr: "CONTINUATION frame with stream ID 0",
680+
w: func(f *Framer) {
681+
cont(f, 0, true)
682+
},
683+
},
684+
10: {
685+
wantErr: "unexpected CONTINUATION for stream 1",
686+
atLeast: 5,
687+
w: func(f *Framer) {
688+
head(f, 1, false)
689+
cont(f, 1, false)
690+
cont(f, 1, false)
691+
cont(f, 1, false)
692+
cont(f, 1, true)
693+
cont(f, 1, false)
694+
},
695+
},
696+
}
697+
for i, tt := range tests {
698+
buf := new(bytes.Buffer)
699+
f := NewFramer(buf, buf)
700+
f.AllowIllegalWrites = true
701+
tt.w(f)
702+
f.WriteData(1, true, nil) // to test transition away from last step
703+
704+
var err error
705+
n := 0
706+
var log bytes.Buffer
707+
for {
708+
var got Frame
709+
got, err = f.ReadFrame()
710+
fmt.Fprintf(&log, " read %v, %v\n", got, err)
711+
if err != nil {
712+
break
713+
}
714+
n++
715+
}
716+
if err == io.EOF {
717+
err = nil
718+
}
719+
ok := tt.wantErr == ""
720+
if ok && err != nil {
721+
t.Errorf("%d. after %d good frames, ReadFrame = %v; want success\n%s", i, n, err, log.Bytes())
722+
continue
723+
}
724+
if !ok && err != ConnectionError(ErrCodeProtocol) {
725+
t.Errorf("%d. after %d good frames, ReadFrame = %v; want ConnectionError(ErrCodeProtocol)\n%s", i, n, err, log.Bytes())
726+
continue
727+
}
728+
if f.errReason != tt.wantErr {
729+
t.Errorf("%d. framer eror = %q; want %q\n%s", i, f.errReason, tt.wantErr, log.Bytes())
730+
}
731+
if n < tt.atLeast {
732+
t.Errorf("%d. framer only read %d frames; want at least %d\n%s", i, n, tt.atLeast, log.Bytes())
733+
}
734+
}
735+
}

‎http2/server.go‎

Lines changed: 0 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -1015,18 +1015,6 @@ func (sc *serverConn) resetStream(se StreamError) {
10151015
}
10161016
}
10171017

1018-
// curHeaderStreamID returns the stream ID of the header block we're
1019-
// currently in the middle of reading. If this returns non-zero, the
1020-
// next frame must be a CONTINUATION with this stream id.
1021-
func (sc *serverConn) curHeaderStreamID() uint32 {
1022-
sc.serveG.check()
1023-
st := sc.req.stream
1024-
if st == nil {
1025-
return 0
1026-
}
1027-
return st.id
1028-
}
1029-
10301018
// processFrameFromReader processes the serve loop's read from readFrameCh from the
10311019
// frame-reading goroutine.
10321020
// processFrameFromReader returns whether the connection should be kept open.
@@ -1091,14 +1079,6 @@ func (sc *serverConn) processFrame(f Frame) error {
10911079
sc.sawFirstSettings = true
10921080
}
10931081

1094-
if s := sc.curHeaderStreamID(); s != 0 {
1095-
if cf, ok := f.(*ContinuationFrame); !ok {
1096-
return ConnectionError(ErrCodeProtocol)
1097-
} else if cf.Header().StreamID != s {
1098-
return ConnectionError(ErrCodeProtocol)
1099-
}
1100-
}
1101-
11021082
switch f := f.(type) {
11031083
case *SettingsFrame:
11041084
return sc.processSettings(f)
@@ -1437,9 +1417,6 @@ func (st *stream) processTrailerHeaders(f *HeadersFrame) error {
14371417
func (sc *serverConn) processContinuation(f *ContinuationFrame) error {
14381418
sc.serveG.check()
14391419
st := sc.streams[f.Header().StreamID]
1440-
if st == nil || sc.curHeaderStreamID() != st.id {
1441-
return ConnectionError(ErrCodeProtocol)
1442-
}
14431420
if st.gotTrailerHeader {
14441421
return st.processTrailerHeaderBlockFragment(f.HeaderBlockFragment(), f.HeadersEnded())
14451422
}

0 commit comments

Comments
 (0)