From d2c89cbf226d150c416ebe9ef877551f7cca9b0d Mon Sep 17 00:00:00 2001 From: Marten Seemann Date: Sun, 3 Jul 2016 22:46:21 +0800 Subject: [PATCH] detect packets smaller than LeastUnacked in ReceivedPacketHandler ref #196 --- ackhandlerlegacy/received_packet_handler.go | 25 ++++++++-- .../received_packet_handler_test.go | 46 +++++++++++++++++++ ackhandlernew/received_packet_handler.go | 25 ++++++++-- ackhandlernew/received_packet_handler_test.go | 46 +++++++++++++++++++ 4 files changed, 136 insertions(+), 6 deletions(-) diff --git a/ackhandlerlegacy/received_packet_handler.go b/ackhandlerlegacy/received_packet_handler.go index 3051d39e..de7448d2 100644 --- a/ackhandlerlegacy/received_packet_handler.go +++ b/ackhandlerlegacy/received_packet_handler.go @@ -9,8 +9,12 @@ import ( "github.com/lucas-clemente/quic-go/qerr" ) -// ErrDuplicatePacket occurres when a duplicate packet is received -var ErrDuplicatePacket = errors.New("ReceivedPacketHandler: Duplicate Packet") +var ( + // ErrDuplicatePacket occurres when a duplicate packet is received + ErrDuplicatePacket = errors.New("ReceivedPacketHandler: Duplicate Packet") + // ErrPacketSmallerThanLastStopWaiting occurs when a packet arrives with a packet number smaller than the largest LeastUnacked of a StopWaitingFrame. If this error occurs, the packet should be ignored + ErrPacketSmallerThanLastStopWaiting = errors.New("ReceivedPacketHandler: Packet number smaller than highest StopWaiting") +) var ( errInvalidPacketNumber = errors.New("ReceivedPacketHandler: Invalid packet number") @@ -26,6 +30,7 @@ type receivedPacketHandler struct { highestInOrderObserved protocol.PacketNumber highestInOrderObservedEntropy EntropyAccumulator largestObserved protocol.PacketNumber + ignorePacketsBelow protocol.PacketNumber currentAckFrame *frames.AckFrameLegacy stateChanged bool // has an ACK for this state already been sent? Will be set to false every time a new packet arrives, and to false every time an ACK is sent @@ -44,6 +49,13 @@ func (h *receivedPacketHandler) ReceivedPacket(packetNumber protocol.PacketNumbe if packetNumber == 0 { return errInvalidPacketNumber } + + // if the packet number is smaller than the largest LeastUnacked value of a StopWaiting we received, we cannot detect if this packet has a duplicate number + // the packet has to be ignored anyway + if packetNumber <= h.ignorePacketsBelow { + return ErrPacketSmallerThanLastStopWaiting + } + _, ok := h.packetHistory[packetNumber] if packetNumber <= h.highestInOrderObserved || ok { return ErrDuplicatePacket @@ -76,7 +88,14 @@ func (h *receivedPacketHandler) ReceivedPacket(packetNumber protocol.PacketNumbe } func (h *receivedPacketHandler) ReceivedStopWaiting(f *frames.StopWaitingFrame) error { - // Ignore if STOP_WAITING is unneeded + // ignore if StopWaiting is unneeded, because we already received a StopWaiting with a higher LeastUnacked + if h.ignorePacketsBelow >= f.LeastUnacked { + return nil + } + + h.ignorePacketsBelow = f.LeastUnacked - 1 + + // ignore if StopWaiting is unneeded, since all packets below have already been received if h.highestInOrderObserved >= f.LeastUnacked { return nil } diff --git a/ackhandlerlegacy/received_packet_handler_test.go b/ackhandlerlegacy/received_packet_handler_test.go index 1e1cc54d..6741c61a 100644 --- a/ackhandlerlegacy/received_packet_handler_test.go +++ b/ackhandlerlegacy/received_packet_handler_test.go @@ -57,6 +57,26 @@ var _ = Describe("receivedPacketHandler", func() { Expect(err).To(MatchError(ErrDuplicatePacket)) }) + It("ignores a packet with PacketNumber less than the LeastUnacked of a previously received StopWaiting", func() { + err := handler.ReceivedPacket(5, false) + Expect(err).ToNot(HaveOccurred()) + err = handler.ReceivedStopWaiting(&frames.StopWaitingFrame{LeastUnacked: 10}) + Expect(err).ToNot(HaveOccurred()) + err = handler.ReceivedPacket(9, false) + Expect(err).To(MatchError(ErrPacketSmallerThanLastStopWaiting)) + Expect(handler.highestInOrderObserved).To(Equal(protocol.PacketNumber(9))) + }) + + It("does not ignore a packet with PacketNumber equal to LeastUnacked of a previously received StopWaiting", func() { + err := handler.ReceivedPacket(5, false) + Expect(err).ToNot(HaveOccurred()) + err = handler.ReceivedStopWaiting(&frames.StopWaitingFrame{LeastUnacked: 10}) + Expect(err).ToNot(HaveOccurred()) + err = handler.ReceivedPacket(10, false) + Expect(err).ToNot(HaveOccurred()) + Expect(handler.highestInOrderObserved).To(Equal(protocol.PacketNumber(10))) + }) + It("saves the time when each packet arrived", func() { err := handler.ReceivedPacket(protocol.PacketNumber(3), false) Expect(err).ToNot(HaveOccurred()) @@ -245,6 +265,32 @@ var _ = Describe("receivedPacketHandler", func() { ranges, _ = handler.getNackRanges() Expect(ranges).To(BeEmpty()) }) + + It("increases the ignorePacketsBelow number", func() { + err := handler.ReceivedStopWaiting(&frames.StopWaitingFrame{LeastUnacked: protocol.PacketNumber(12)}) + Expect(err).ToNot(HaveOccurred()) + Expect(handler.ignorePacketsBelow).To(Equal(protocol.PacketNumber(11))) + }) + + It("increase the ignorePacketsBelow number, even if all packets below the LeastUnacked were already acked", func() { + for i := 1; i < 20; i++ { + err := handler.ReceivedPacket(protocol.PacketNumber(i), false) + Expect(err).ToNot(HaveOccurred()) + } + Expect(handler.highestInOrderObserved).To(Equal(protocol.PacketNumber(19))) + err := handler.ReceivedStopWaiting(&frames.StopWaitingFrame{LeastUnacked: protocol.PacketNumber(12)}) + Expect(err).ToNot(HaveOccurred()) + Expect(handler.ignorePacketsBelow).To(Equal(protocol.PacketNumber(11))) + }) + + It("does not decrease the ignorePacketsBelow number when an out-of-order StopWaiting arrives", func() { + err := handler.ReceivedStopWaiting(&frames.StopWaitingFrame{LeastUnacked: protocol.PacketNumber(12)}) + Expect(err).ToNot(HaveOccurred()) + Expect(handler.ignorePacketsBelow).To(Equal(protocol.PacketNumber(11))) + err = handler.ReceivedStopWaiting(&frames.StopWaitingFrame{LeastUnacked: protocol.PacketNumber(6)}) + Expect(err).ToNot(HaveOccurred()) + Expect(handler.ignorePacketsBelow).To(Equal(protocol.PacketNumber(11))) + }) }) Context("ACK package generation", func() { diff --git a/ackhandlernew/received_packet_handler.go b/ackhandlernew/received_packet_handler.go index 3f24c482..c9cd3a64 100644 --- a/ackhandlernew/received_packet_handler.go +++ b/ackhandlernew/received_packet_handler.go @@ -9,8 +9,12 @@ import ( "github.com/lucas-clemente/quic-go/qerr" ) -// ErrDuplicatePacket occurres when a duplicate packet is received -var ErrDuplicatePacket = errors.New("ReceivedPacketHandler: Duplicate Packet") +var ( + // ErrDuplicatePacket occurres when a duplicate packet is received + ErrDuplicatePacket = errors.New("ReceivedPacketHandler: Duplicate Packet") + // ErrPacketSmallerThanLastStopWaiting occurs when a packet arrives with a packet number smaller than the largest LeastUnacked of a StopWaitingFrame. If this error occurs, the packet should be ignored + ErrPacketSmallerThanLastStopWaiting = errors.New("ReceivedPacketHandler: Packet number smaller than highest StopWaiting") +) var ( errInvalidPacketNumber = errors.New("ReceivedPacketHandler: Invalid packet number") @@ -20,6 +24,7 @@ var ( type receivedPacketHandler struct { largestInOrderObserved protocol.PacketNumber largestObserved protocol.PacketNumber + ignorePacketsBelow protocol.PacketNumber currentAckFrame *frames.AckFrameNew stateChanged bool // has an ACK for this state already been sent? Will be set to false every time a new packet arrives, and to false every time an ACK is sent @@ -40,6 +45,13 @@ func (h *receivedPacketHandler) ReceivedPacket(packetNumber protocol.PacketNumbe if packetNumber == 0 { return errInvalidPacketNumber } + + // if the packet number is smaller than the largest LeastUnacked value of a StopWaiting we received, we cannot detect if this packet has a duplicate number + // the packet has to be ignored anyway + if packetNumber <= h.ignorePacketsBelow { + return ErrPacketSmallerThanLastStopWaiting + } + _, ok := h.receivedTimes[packetNumber] if packetNumber <= h.largestInOrderObserved || ok { return ErrDuplicatePacket @@ -70,7 +82,14 @@ func (h *receivedPacketHandler) ReceivedPacket(packetNumber protocol.PacketNumbe } func (h *receivedPacketHandler) ReceivedStopWaiting(f *frames.StopWaitingFrame) error { - // Ignore if STOP_WAITING is unneeded + // ignore if StopWaiting is unneeded, because we already received a StopWaiting with a higher LeastUnacked + if h.ignorePacketsBelow >= f.LeastUnacked { + return nil + } + + h.ignorePacketsBelow = f.LeastUnacked - 1 + + // ignore if StopWaiting is unneeded, since all packets below have already been received if h.largestInOrderObserved >= f.LeastUnacked { return nil } diff --git a/ackhandlernew/received_packet_handler_test.go b/ackhandlernew/received_packet_handler_test.go index 0bf7a8b8..8c31e3da 100644 --- a/ackhandlernew/received_packet_handler_test.go +++ b/ackhandlernew/received_packet_handler_test.go @@ -55,6 +55,26 @@ var _ = Describe("receivedPacketHandler", func() { Expect(err).To(MatchError(ErrDuplicatePacket)) }) + It("ignores a packet with PacketNumber less than the LeastUnacked of a previously received StopWaiting", func() { + err := handler.ReceivedPacket(5) + Expect(err).ToNot(HaveOccurred()) + err = handler.ReceivedStopWaiting(&frames.StopWaitingFrame{LeastUnacked: 10}) + Expect(err).ToNot(HaveOccurred()) + err = handler.ReceivedPacket(9) + Expect(err).To(MatchError(ErrPacketSmallerThanLastStopWaiting)) + Expect(handler.largestInOrderObserved).To(Equal(protocol.PacketNumber(9))) + }) + + It("does not ignore a packet with PacketNumber equal to LeastUnacked of a previously received StopWaiting", func() { + err := handler.ReceivedPacket(5) + Expect(err).ToNot(HaveOccurred()) + err = handler.ReceivedStopWaiting(&frames.StopWaitingFrame{LeastUnacked: 10}) + Expect(err).ToNot(HaveOccurred()) + err = handler.ReceivedPacket(10) + Expect(err).ToNot(HaveOccurred()) + Expect(handler.largestInOrderObserved).To(Equal(protocol.PacketNumber(10))) + }) + It("saves the time when each packet arrived", func() { err := handler.ReceivedPacket(protocol.PacketNumber(3)) Expect(err).ToNot(HaveOccurred()) @@ -87,6 +107,32 @@ var _ = Describe("receivedPacketHandler", func() { Expect(err).ToNot(HaveOccurred()) Expect(handler.largestInOrderObserved).To(Equal(protocol.PacketNumber(11))) }) + + It("increases the ignorePacketsBelow number", func() { + err := handler.ReceivedStopWaiting(&frames.StopWaitingFrame{LeastUnacked: protocol.PacketNumber(12)}) + Expect(err).ToNot(HaveOccurred()) + Expect(handler.ignorePacketsBelow).To(Equal(protocol.PacketNumber(11))) + }) + + It("increase the ignorePacketsBelow number, even if all packets below the LeastUnacked were already acked", func() { + for i := 1; i < 20; i++ { + err := handler.ReceivedPacket(protocol.PacketNumber(i)) + Expect(err).ToNot(HaveOccurred()) + } + Expect(handler.largestInOrderObserved).To(Equal(protocol.PacketNumber(19))) + err := handler.ReceivedStopWaiting(&frames.StopWaitingFrame{LeastUnacked: protocol.PacketNumber(12)}) + Expect(err).ToNot(HaveOccurred()) + Expect(handler.ignorePacketsBelow).To(Equal(protocol.PacketNumber(11))) + }) + + It("does not decrease the ignorePacketsBelow number when an out-of-order StopWaiting arrives", func() { + err := handler.ReceivedStopWaiting(&frames.StopWaitingFrame{LeastUnacked: protocol.PacketNumber(12)}) + Expect(err).ToNot(HaveOccurred()) + Expect(handler.ignorePacketsBelow).To(Equal(protocol.PacketNumber(11))) + err = handler.ReceivedStopWaiting(&frames.StopWaitingFrame{LeastUnacked: protocol.PacketNumber(6)}) + Expect(err).ToNot(HaveOccurred()) + Expect(handler.ignorePacketsBelow).To(Equal(protocol.PacketNumber(11))) + }) }) Context("ACK package generation", func() {