check for consistent ACK ranges when writing QUIC 34 ACK frames

ref #182

frames/ack_frame_new.go

@@ -9,7 +9,13 @@ import (
 	"github.com/lucas-clemente/quic-go/utils"
 )
 
-var errInvalidAckRanges = errors.New("AckFrame: ACK frame contains invalid ACK ranges")
+// ErrInvalidAckRanges occurs when a client sends inconsistent ACK ranges
+var ErrInvalidAckRanges = errors.New("AckFrame: ACK frame contains invalid ACK ranges")
+
+var (
+	errInconsistentAckLargestObserved = errors.New("internal inconsistency: LargestObserved does not match ACK ranges")
+	errInconsistentAckLowestAcked     = errors.New("internal inconsistency: LowestAcked does not match ACK ranges")
+)
 
 // An AckFrameNew is a ACK frame in QUIC c34
 type AckFrameNew struct {
@@ -73,7 +79,7 @@ func ParseAckFrameNew(r *bytes.Reader, version protocol.VersionNumber) (*AckFram
 	}
 
 	if ackBlockLength > largestObserved {
-		return nil, errInvalidAckRanges
+		return nil, ErrInvalidAckRanges
 	}
 
 	if hasMissingRanges {
@@ -117,7 +123,7 @@ func ParseAckFrameNew(r *bytes.Reader, version protocol.VersionNumber) (*AckFram
 	}
 
 	if !frame.validateAckRanges() {
-		return nil, errInvalidAckRanges
+		return nil, ErrInvalidAckRanges
 	}
 
 	var numTimestampByte byte
@@ -207,10 +213,10 @@ func (f *AckFrameNew) Write(b *bytes.Buffer, version protocol.VersionNumber) err
 		utils.WriteUint48(b, uint64(f.LargestObserved-f.LowestAcked+1))
 	} else {
 		if f.LargestObserved != f.AckRanges[0].LastPacketNumber {
-			return errors.New("internal inconsistency: LastPacketNumber does not match ACK ranges")
+			return errInconsistentAckLargestObserved
 		}
 		if f.LowestAcked != f.AckRanges[len(f.AckRanges)-1].FirstPacketNumber {
-			return errors.New("internal inconsistency: FirstPacketNumber does not match ACK ranges")
+			return errInconsistentAckLowestAcked
 		}
 		length := f.LargestObserved - f.AckRanges[0].FirstPacketNumber + 1
 		utils.WriteUint48(b, uint64(length))

frames/ack_frame_new_test.go

@@ -59,7 +59,7 @@ var _ = Describe("AckFrame", func() {
 			// Length: 0x1d => LowestAcked would be -1
 			b := bytes.NewReader([]byte{0x40, 0x1c, 0x8e, 0x0, 0x1d, 0x1, 0x1, 0x6b, 0x26, 0x3, 0x0})
 			_, err := ParseAckFrameNew(b, 0)
-			Expect(err).To(MatchError(errInvalidAckRanges))
+			Expect(err).To(MatchError(ErrInvalidAckRanges))
 		})
 
 		Context("ACK blocks", func() {
@@ -80,7 +80,7 @@ var _ = Describe("AckFrame", func() {
 				// like the test before, but increased the last ACK range, such that the FirstPacketNumber would be negative
 				b := bytes.NewReader([]byte{0x60, 0x18, 0x94, 0x1, 0x1, 0x3, 0x2, 0x15, 0x2, 0x1, 0x5c, 0xd5, 0x0, 0x0, 0x0, 0x95, 0x0})
 				_, err := ParseAckFrameNew(b, 0)
-				Expect(err).To(MatchError(errInvalidAckRanges))
+				Expect(err).To(MatchError(ErrInvalidAckRanges))
 			})
 
 			It("parses a frame with multiple single packets missing", func() {
@@ -284,6 +284,32 @@ var _ = Describe("AckFrame", func() {
 				Expect(r.Len()).To(BeZero())
 			})
 
+			It("rejects a frame with incorrect LargestObserved value", func() {
+				frame := &AckFrameNew{
+					LargestObserved: 26,
+					LowestAcked:     1,
+					AckRanges: []AckRange{
+						AckRange{FirstPacketNumber: 12, LastPacketNumber: 25},
+						AckRange{FirstPacketNumber: 1, LastPacketNumber: 10},
+					},
+				}
+				err := frame.Write(b, 0)
+				Expect(err).To(MatchError(errInconsistentAckLargestObserved))
+			})
+
+			It("rejects a frame with incorrect LargestObserved value", func() {
+				frame := &AckFrameNew{
+					LargestObserved: 25,
+					LowestAcked:     2,
+					AckRanges: []AckRange{
+						AckRange{FirstPacketNumber: 12, LastPacketNumber: 25},
+						AckRange{FirstPacketNumber: 1, LastPacketNumber: 10},
+					},
+				}
+				err := frame.Write(b, 0)
+				Expect(err).To(MatchError(errInconsistentAckLowestAcked))
+			})
+
 			Context("longer ACK blocks", func() {
 				It("only writes one block for 254 lost packets", func() {
 					frameOrig := &AckFrameNew{