From 76c742a43d776607c9d463fed8b89955cd2a4cce Mon Sep 17 00:00:00 2001 From: Marten Seemann Date: Fri, 8 Nov 2019 12:54:05 +0700 Subject: [PATCH 1/3] include the frame type in the QuicError error message --- internal/qerr/error_codes.go | 11 ++++++++++- internal/qerr/quic_error.go | 24 +++++++++++++++++++++--- internal/qerr/quic_error_test.go | 10 ++++++++++ 3 files changed, 41 insertions(+), 4 deletions(-) diff --git a/internal/qerr/error_codes.go b/internal/qerr/error_codes.go index f5a9de5e3..426133961 100644 --- a/internal/qerr/error_codes.go +++ b/internal/qerr/error_codes.go @@ -31,11 +31,20 @@ func (e ErrorCode) isCryptoError() bool { func (e ErrorCode) Error() string { if e.isCryptoError() { - return fmt.Sprintf("%s: %s", e.String(), qtls.Alert(e-0x100).Error()) + return fmt.Sprintf("%s: %s", e.String(), e.Message()) } return e.String() } +// Message is a description of the error. +// It only returns a non-empty string for crypto errors. +func (e ErrorCode) Message() string { + if !e.isCryptoError() { + return "" + } + return qtls.Alert(e - 0x100).Error() +} + func (e ErrorCode) String() string { switch e { case NoError: diff --git a/internal/qerr/quic_error.go b/internal/qerr/quic_error.go index 66e8960c5..07148c367 100644 --- a/internal/qerr/quic_error.go +++ b/internal/qerr/quic_error.go @@ -8,6 +8,7 @@ import ( // A QuicError consists of an error code plus a error reason type QuicError struct { ErrorCode ErrorCode + FrameType uint64 // only valid if this not an application error ErrorMessage string isTimeout bool isApplicationError bool @@ -23,6 +24,15 @@ func Error(errorCode ErrorCode, errorMessage string) *QuicError { } } +// ErrorWithFrameType creates a new QuicError instance for a specific frame type +func ErrorWithFrameType(errorCode ErrorCode, frameType uint64, errorMessage string) *QuicError { + return &QuicError{ + ErrorCode: errorCode, + FrameType: frameType, + ErrorMessage: errorMessage, + } +} + // TimeoutError creates a new QuicError instance for a timeout error func TimeoutError(errorMessage string) *QuicError { return &QuicError{ @@ -55,10 +65,18 @@ func (e *QuicError) Error() string { } return fmt.Sprintf("Application error %#x: %s", uint64(e.ErrorCode), e.ErrorMessage) } - if len(e.ErrorMessage) == 0 { - return e.ErrorCode.Error() + str := e.ErrorCode.String() + if e.FrameType != 0 { + str += fmt.Sprintf(" (frame type: %#x)", e.FrameType) } - return fmt.Sprintf("%s: %s", e.ErrorCode.String(), e.ErrorMessage) + msg := e.ErrorMessage + if len(msg) == 0 { + msg = e.ErrorCode.Message() + } + if len(msg) == 0 { + return str + } + return str + ": " + msg } // IsCryptoError says if this error is a crypto error diff --git a/internal/qerr/quic_error_test.go b/internal/qerr/quic_error_test.go index 9c48487e3..162ff2adc 100644 --- a/internal/qerr/quic_error_test.go +++ b/internal/qerr/quic_error_test.go @@ -20,6 +20,16 @@ var _ = Describe("QUIC Transport Errors", func() { Expect(err.Error()).To(Equal("FLOW_CONTROL_ERROR")) }) + It("includes the frame type, for errors without a message", func() { + err := ErrorWithFrameType(FlowControlError, 0x1337, "") + Expect(err.Error()).To(Equal("FLOW_CONTROL_ERROR (frame type: 0x1337)")) + }) + + It("includes the frame type, for errors with a message", func() { + err := ErrorWithFrameType(FlowControlError, 0x1337, "foobar") + Expect(err.Error()).To(Equal("FLOW_CONTROL_ERROR (frame type: 0x1337): foobar")) + }) + It("has a string representation for timeout errors", func() { err := TimeoutError("foobar") Expect(err.Timeout()).To(BeTrue()) From dbdccfa70a469c73cf18aabab3d7e17236045c8b Mon Sep 17 00:00:00 2001 From: Marten Seemann Date: Fri, 8 Nov 2019 14:01:27 +0700 Subject: [PATCH 2/3] include the frame type in the error returned by the frame parser --- internal/wire/frame_parser.go | 5 +++-- internal/wire/frame_parser_test.go | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/internal/wire/frame_parser.go b/internal/wire/frame_parser.go index 60b5dc1bf..fbd66d44e 100644 --- a/internal/wire/frame_parser.go +++ b/internal/wire/frame_parser.go @@ -2,6 +2,7 @@ package wire import ( "bytes" + "errors" "fmt" "reflect" @@ -32,7 +33,7 @@ func (p *frameParser) ParseNext(r *bytes.Reader, encLevel protocol.EncryptionLev f, err := p.parseFrame(r, typeByte, encLevel) if err != nil { - return nil, qerr.Error(qerr.FrameEncodingError, err.Error()) + return nil, qerr.ErrorWithFrameType(qerr.FrameEncodingError, uint64(typeByte), err.Error()) } return f, nil } @@ -85,7 +86,7 @@ func (p *frameParser) parseFrame(r *bytes.Reader, typeByte byte, encLevel protoc case 0x1c, 0x1d: frame, err = parseConnectionCloseFrame(r, p.version) default: - err = fmt.Errorf("unknown type byte 0x%x", typeByte) + err = errors.New("unknown frame type") } } if err != nil { diff --git a/internal/wire/frame_parser_test.go b/internal/wire/frame_parser_test.go index b99c60065..1d204bdf7 100644 --- a/internal/wire/frame_parser_test.go +++ b/internal/wire/frame_parser_test.go @@ -273,7 +273,7 @@ var _ = Describe("Frame parsing", func() { It("errors on invalid type", func() { _, err := parser.ParseNext(bytes.NewReader([]byte{0x42}), protocol.Encryption1RTT) - Expect(err).To(MatchError("FRAME_ENCODING_ERROR: unknown type byte 0x42")) + Expect(err).To(MatchError("FRAME_ENCODING_ERROR (frame type: 0x42): unknown frame type")) }) It("errors on invalid frames", func() { From 312fb638bec4f7cd1b63c6578177d3c01fee925f Mon Sep 17 00:00:00 2001 From: Marten Seemann Date: Fri, 8 Nov 2019 14:19:49 +0700 Subject: [PATCH 3/3] send the frame type in CONNECTION_CLOSE frames --- session.go | 1 + session_test.go | 22 +++++++++++++++++++--- 2 files changed, 20 insertions(+), 3 deletions(-) diff --git a/session.go b/session.go index a37b6b87a..8fbc4061b 100644 --- a/session.go +++ b/session.go @@ -1252,6 +1252,7 @@ func (s *session) sendConnectionClose(quicErr *qerr.QuicError) ([]byte, error) { packet, err := s.packer.PackConnectionClose(&wire.ConnectionCloseFrame{ IsApplicationError: quicErr.IsApplicationError(), ErrorCode: quicErr.ErrorCode, + FrameType: quicErr.FrameType, ReasonPhrase: reason, }) if err != nil { diff --git a/session_test.go b/session_test.go index 2ba573e89..06105c800 100644 --- a/session_test.go +++ b/session_test.go @@ -456,8 +456,7 @@ var _ = Describe("Session", func() { }) It("closes with an error", func() { - testErr := errors.New("test error") - streamManager.EXPECT().CloseWithError(qerr.ApplicationError(0x1337, testErr.Error())) + streamManager.EXPECT().CloseWithError(qerr.ApplicationError(0x1337, "test error")) expectReplaceWithClosed() cryptoSetup.EXPECT().Close() packer.EXPECT().PackConnectionClose(gomock.Any()).DoAndReturn(func(f *wire.ConnectionCloseFrame) (*packedPacket, error) { @@ -466,7 +465,24 @@ var _ = Describe("Session", func() { Expect(f.ReasonPhrase).To(Equal("test error")) return &packedPacket{}, nil }) - sess.CloseWithError(0x1337, testErr.Error()) + sess.CloseWithError(0x1337, "test error") + Eventually(areSessionsRunning).Should(BeFalse()) + Expect(sess.Context().Done()).To(BeClosed()) + }) + + It("includes the frame type in transport-level close frames", func() { + testErr := qerr.ErrorWithFrameType(0x1337, 0x42, "test error") + streamManager.EXPECT().CloseWithError(testErr) + expectReplaceWithClosed() + cryptoSetup.EXPECT().Close() + packer.EXPECT().PackConnectionClose(gomock.Any()).DoAndReturn(func(f *wire.ConnectionCloseFrame) (*packedPacket, error) { + Expect(f.IsApplicationError).To(BeFalse()) + Expect(f.FrameType).To(BeEquivalentTo(0x42)) + Expect(f.ErrorCode).To(BeEquivalentTo(0x1337)) + Expect(f.ReasonPhrase).To(Equal("test error")) + return &packedPacket{}, nil + }) + sess.closeLocal(testErr) Eventually(areSessionsRunning).Should(BeFalse()) Expect(sess.Context().Done()).To(BeClosed()) })