limit the number of queued PATH_RESPONSE frames to 256 (#4199)

framer.go

@@ -23,6 +23,8 @@ type framer interface {
 	Handle0RTTRejection() error
 }
 
+const maxPathResponses = 256
+
 type framerI struct {
 	mutex sync.Mutex
 
@@ -33,6 +35,7 @@ type framerI struct {
 
 	controlFrameMutex sync.Mutex
 	controlFrames     []wire.Frame
+	pathResponses     []*wire.PathResponseFrame
 }
 
 var _ framer = &framerI{}
@@ -52,20 +55,43 @@ func (f *framerI) HasData() bool {
 		return true
 	}
 	f.controlFrameMutex.Lock()
-	hasData = len(f.controlFrames) > 0
-	f.controlFrameMutex.Unlock()
-	return hasData
+	defer f.controlFrameMutex.Unlock()
+	return len(f.controlFrames) > 0 || len(f.pathResponses) > 0
 }
 
 func (f *framerI) QueueControlFrame(frame wire.Frame) {
 	f.controlFrameMutex.Lock()
+	defer f.controlFrameMutex.Unlock()
+
+	if pr, ok := frame.(*wire.PathResponseFrame); ok {
+		// Only queue up to maxPathResponses PATH_RESPONSE frames.
+		// This limit should be high enough to never be hit in practice,
+		// unless the peer is doing something malicious.
+		if len(f.pathResponses) >= maxPathResponses {
+			return
+		}
+		f.pathResponses = append(f.pathResponses, pr)
+		return
+	}
 	f.controlFrames = append(f.controlFrames, frame)
-	f.controlFrameMutex.Unlock()
 }
 
 func (f *framerI) AppendControlFrames(frames []ackhandler.Frame, maxLen protocol.ByteCount, v protocol.VersionNumber) ([]ackhandler.Frame, protocol.ByteCount) {
-	var length protocol.ByteCount
 	f.controlFrameMutex.Lock()
+	defer f.controlFrameMutex.Unlock()
+
+	var length protocol.ByteCount
+	// add a PATH_RESPONSE first, but only pack a single PATH_RESPONSE per packet
+	if len(f.pathResponses) > 0 {
+		frame := f.pathResponses[0]
+		frameLen := frame.Length(v)
+		if frameLen <= maxLen {
+			frames = append(frames, ackhandler.Frame{Frame: frame})
+			length += frameLen
+			f.pathResponses = f.pathResponses[1:]
+		}
+	}
+
 	for len(f.controlFrames) > 0 {
 		frame := f.controlFrames[len(f.controlFrames)-1]
 		frameLen := frame.Length(v)
@@ -76,7 +102,6 @@ func (f *framerI) AppendControlFrames(frames []ackhandler.Frame, maxLen protocol
 		length += frameLen
 		f.controlFrames = f.controlFrames[:len(f.controlFrames)-1]
 	}
-	f.controlFrameMutex.Unlock()
 	return frames, length
 }
 

framer_test.go

@@ -2,7 +2,8 @@ package quic
 
 import (
 	"bytes"
-	"math/rand"
+
+	"golang.org/x/exp/rand"
 
 	"github.com/quic-go/quic-go/internal/ackhandler"
 	"github.com/quic-go/quic-go/internal/protocol"
@@ -110,6 +111,55 @@ var _ = Describe("Framer", func() {
 		})
 	})
 
+	Context("handling PATH_RESPONSE frames", func() {
+		It("packs a single PATH_RESPONSE per packet", func() {
+			f1 := &wire.PathResponseFrame{Data: [8]byte{1, 2, 3, 4, 5, 6, 7, 8}}
+			f2 := &wire.PathResponseFrame{Data: [8]byte{2, 3, 4, 5, 6, 7, 8, 9}}
+			cf1 := &wire.DataBlockedFrame{MaximumData: 1337}
+			cf2 := &wire.HandshakeDoneFrame{}
+			framer.QueueControlFrame(f1)
+			framer.QueueControlFrame(f2)
+			framer.QueueControlFrame(cf1)
+			framer.QueueControlFrame(cf2)
+			// the first packet should contain a single PATH_RESPONSE frame, but all the other control frames
+			Expect(framer.HasData()).To(BeTrue())
+			frames, length := framer.AppendControlFrames(nil, protocol.MaxByteCount, protocol.Version1)
+			Expect(frames).To(HaveLen(3))
+			Expect(frames[0].Frame).To(Equal(f1))
+			Expect([]wire.Frame{frames[1].Frame, frames[2].Frame}).To(ContainElement(cf1))
+			Expect([]wire.Frame{frames[1].Frame, frames[2].Frame}).To(ContainElement(cf2))
+			Expect(length).To(Equal(f1.Length(protocol.Version1) + cf1.Length(protocol.Version1) + cf2.Length(protocol.Version1)))
+			// the second packet should contain the other PATH_RESPONSE frame
+			Expect(framer.HasData()).To(BeTrue())
+			frames, length = framer.AppendControlFrames(nil, protocol.MaxByteCount, protocol.Version1)
+			Expect(frames).To(HaveLen(1))
+			Expect(frames[0].Frame).To(Equal(f2))
+			Expect(length).To(Equal(f2.Length(protocol.Version1)))
+			Expect(framer.HasData()).To(BeFalse())
+		})
+
+		It("limits the number of queued PATH_RESPONSE frames", func() {
+			var pathResponses []*wire.PathResponseFrame
+			for i := 0; i < 2*maxPathResponses; i++ {
+				var f wire.PathResponseFrame
+				rand.Read(f.Data[:])
+				pathResponses = append(pathResponses, &f)
+				framer.QueueControlFrame(&f)
+			}
+			for i := 0; i < maxPathResponses; i++ {
+				Expect(framer.HasData()).To(BeTrue())
+				frames, length := framer.AppendControlFrames(nil, protocol.MaxByteCount, protocol.Version1)
+				Expect(frames).To(HaveLen(1))
+				Expect(frames[0].Frame).To(Equal(pathResponses[i]))
+				Expect(length).To(Equal(pathResponses[i].Length(protocol.Version1)))
+			}
+			Expect(framer.HasData()).To(BeFalse())
+			frames, length := framer.AppendControlFrames(nil, protocol.MaxByteCount, protocol.Version1)
+			Expect(frames).To(BeEmpty())
+			Expect(length).To(BeZero())
+		})
+	})
+
 	Context("popping STREAM frames", func() {
 		It("returns nil when popping an empty framer", func() {
 			Expect(framer.AppendStreamFrames(nil, 1000, protocol.Version1)).To(BeEmpty())