From a0ffa757499913f7be69aa78f573a6aee3430ae4 Mon Sep 17 00:00:00 2001 From: Marten Seemann Date: Wed, 13 Dec 2023 09:47:09 +0530 Subject: [PATCH 1/2] limit the number of queued PATH_RESPONSE frames to 256 (#4199) --- framer.go | 37 +++++++++++++++++++++++++++++------ framer_test.go | 52 +++++++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 82 insertions(+), 7 deletions(-) diff --git a/framer.go b/framer.go index 9409af4c2ee..d5c61bcf73e 100644 --- a/framer.go +++ b/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 } diff --git a/framer_test.go b/framer_test.go index add9da713c5..97cbd144bed 100644 --- a/framer_test.go +++ b/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()) From 6cc3d58935426191296171a6c0d1ee965e10534e Mon Sep 17 00:00:00 2001 From: Marten Seemann Date: Wed, 13 Dec 2023 09:52:13 +0530 Subject: [PATCH 2/2] don't retransmit PATH_CHALLENGE and PATH_RESPONSE frames (#4200) --- packet_packer.go | 8 +++++++- packet_packer_test.go | 31 +++++++++++++++++++++++++++++++ 2 files changed, 38 insertions(+), 1 deletion(-) diff --git a/packet_packer.go b/packet_packer.go index 64081c6842b..a330632bee6 100644 --- a/packet_packer.go +++ b/packet_packer.go @@ -640,7 +640,13 @@ func (p *packetPacker) composeNextPacket(maxFrameSize protocol.ByteCount, onlyAc pl.length += lengthAdded // add handlers for the control frames that were added for i := startLen; i < len(pl.frames); i++ { - pl.frames[i].Handler = p.retransmissionQueue.AppDataAckHandler() + switch pl.frames[i].Frame.(type) { + case *wire.PathChallengeFrame, *wire.PathResponseFrame: + // Path probing is currently not supported, therefore we don't need to set the OnAcked callback yet. + // PATH_CHALLENGE and PATH_RESPONSE are never retransmitted. + default: + pl.frames[i].Handler = p.retransmissionQueue.AppDataAckHandler() + } } pl.streamFrames, lengthAdded = p.framer.AppendStreamFrames(pl.streamFrames, maxFrameSize-pl.length, v) diff --git a/packet_packer_test.go b/packet_packer_test.go index e68906f39f1..fe746e631c9 100644 --- a/packet_packer_test.go +++ b/packet_packer_test.go @@ -562,6 +562,37 @@ var _ = Describe("Packet packer", func() { Expect(buffer.Len()).ToNot(BeZero()) }) + It("packs PATH_CHALLENGE and PATH_RESPONSE frames", func() { + pnManager.EXPECT().PeekPacketNumber(protocol.Encryption1RTT).Return(protocol.PacketNumber(0x42), protocol.PacketNumberLen2) + pnManager.EXPECT().PopPacketNumber(protocol.Encryption1RTT).Return(protocol.PacketNumber(0x42)) + sealingManager.EXPECT().Get1RTTSealer().Return(getSealer(), nil) + framer.EXPECT().HasData().Return(true) + ackFramer.EXPECT().GetAckFrame(protocol.Encryption1RTT, false) + frames := []ackhandler.Frame{ + {Frame: &wire.PathChallengeFrame{}}, + {Frame: &wire.PathResponseFrame{}}, + {Frame: &wire.DataBlockedFrame{}}, + } + expectAppendControlFrames(frames...) + expectAppendStreamFrames() + buffer := getPacketBuffer() + p, err := packer.AppendPacket(buffer, maxPacketSize, protocol.Version1) + Expect(p).ToNot(BeNil()) + Expect(err).ToNot(HaveOccurred()) + Expect(p.Frames).To(HaveLen(3)) + for i, f := range p.Frames { + Expect(f).To(BeAssignableToTypeOf(frames[i])) + switch f.Frame.(type) { + case *wire.PathChallengeFrame, *wire.PathResponseFrame: + // This means that the frame won't be retransmitted. + Expect(f.Handler).To(BeNil()) + default: + Expect(f.Handler).ToNot(BeNil()) + } + } + Expect(buffer.Len()).ToNot(BeZero()) + }) + It("packs DATAGRAM frames", func() { ackFramer.EXPECT().GetAckFrame(protocol.Encryption1RTT, true) pnManager.EXPECT().PeekPacketNumber(protocol.Encryption1RTT).Return(protocol.PacketNumber(0x42), protocol.PacketNumberLen2)