Merge pull request #2211 from lucas-clemente/frame-type-errors

use the frame type in CONNECTION_CLOSE frames for frame parsing errors

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:

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

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())

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 {

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() {

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 {

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())
 		})