From 5163ae1f6130162ac49eda84503d6fbafb00c0e4 Mon Sep 17 00:00:00 2001 From: Marten Seemann Date: Sat, 29 Oct 2016 01:29:01 +0700 Subject: [PATCH] move duplicate detection to receivedPacketHistory --- ackhandler/received_packet_handler.go | 3 +- ackhandler/received_packet_handler_test.go | 11 +-- ackhandler/received_packet_history.go | 43 +++++++++-- ackhandler/received_packet_history_test.go | 90 ++++++++++++++++++++++ 4 files changed, 130 insertions(+), 17 deletions(-) diff --git a/ackhandler/received_packet_handler.go b/ackhandler/received_packet_handler.go index 0154197f..55dc9425 100644 --- a/ackhandler/received_packet_handler.go +++ b/ackhandler/received_packet_handler.go @@ -56,8 +56,7 @@ func (h *receivedPacketHandler) ReceivedPacket(packetNumber protocol.PacketNumbe return ErrPacketSmallerThanLastStopWaiting } - _, ok := h.receivedTimes[packetNumber] - if packetNumber <= h.largestInOrderObserved || ok { + if h.packetHistory.IsDuplicate(packetNumber) { return ErrDuplicatePacket } diff --git a/ackhandler/received_packet_handler_test.go b/ackhandler/received_packet_handler_test.go index 5a4bb54f..2ac2c4c4 100644 --- a/ackhandler/received_packet_handler_test.go +++ b/ackhandler/received_packet_handler_test.go @@ -37,7 +37,7 @@ var _ = Describe("receivedPacketHandler", func() { Expect(err).To(MatchError(errInvalidPacketNumber)) }) - It("rejects a duplicate package with PacketNumber equal to LargestObserved", func() { + It("rejects a duplicate package", func() { for i := 1; i < 5; i++ { err := handler.ReceivedPacket(protocol.PacketNumber(i)) Expect(err).ToNot(HaveOccurred()) @@ -46,15 +46,6 @@ var _ = Describe("receivedPacketHandler", func() { Expect(err).To(MatchError(ErrDuplicatePacket)) }) - It("rejects a duplicate package with PacketNumber less than the LargestObserved", func() { - for i := 1; i < 5; i++ { - err := handler.ReceivedPacket(protocol.PacketNumber(i)) - Expect(err).ToNot(HaveOccurred()) - } - err := handler.ReceivedPacket(2) - 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()) diff --git a/ackhandler/received_packet_history.go b/ackhandler/received_packet_history.go index ad1d01d3..9b2ee687 100644 --- a/ackhandler/received_packet_history.go +++ b/ackhandler/received_packet_history.go @@ -10,9 +10,13 @@ import ( ) type receivedPacketHistory struct { + mutex sync.RWMutex + ranges *utils.PacketIntervalList - mutex sync.RWMutex + // the map is used as a replacement for a set here. The bool is always supposed to be set to true + receivedPacketNumbers map[protocol.PacketNumber]bool + lowestInReceivedPacketNumbers protocol.PacketNumber } var errTooManyOutstandingReceivedAckRanges = qerr.Error(qerr.TooManyOutstandingReceivedPackets, "Too many outstanding received ACK ranges") @@ -20,7 +24,8 @@ var errTooManyOutstandingReceivedAckRanges = qerr.Error(qerr.TooManyOutstandingR // newReceivedPacketHistory creates a new received packet history func newReceivedPacketHistory() *receivedPacketHistory { return &receivedPacketHistory{ - ranges: utils.NewPacketIntervalList(), + ranges: utils.NewPacketIntervalList(), + receivedPacketNumbers: make(map[protocol.PacketNumber]bool), } } @@ -33,6 +38,12 @@ func (h *receivedPacketHistory) ReceivedPacket(p protocol.PacketNumber) error { return errTooManyOutstandingReceivedAckRanges } + if len(h.receivedPacketNumbers) >= protocol.MaxTrackedReceivedPackets { + return errTooManyOutstandingReceivedPackets + } + + h.receivedPacketNumbers[p] = true + if h.ranges.Len() == 0 { h.ranges.PushBack(utils.PacketInterval{Start: p, End: p}) return nil @@ -77,25 +88,47 @@ func (h *receivedPacketHistory) ReceivedPacket(p protocol.PacketNumber) error { return nil } +// DeleteBelow deletes all entries below the leastUnacked packet number func (h *receivedPacketHistory) DeleteBelow(leastUnacked protocol.PacketNumber) { h.mutex.Lock() defer h.mutex.Unlock() + h.lowestInReceivedPacketNumbers = utils.MaxPacketNumber(h.lowestInReceivedPacketNumbers, leastUnacked) + nextEl := h.ranges.Front() for el := h.ranges.Front(); nextEl != nil; el = nextEl { nextEl = el.Next() if leastUnacked > el.Value.Start && leastUnacked <= el.Value.End { + for i := el.Value.Start; i < leastUnacked; i++ { // adjust start value of a range + delete(h.receivedPacketNumbers, i) + } el.Value.Start = leastUnacked - } - if el.Value.End < leastUnacked { // delete a whole range + } else if el.Value.End < leastUnacked { // delete a whole range + for i := el.Value.Start; i <= el.Value.End; i++ { + delete(h.receivedPacketNumbers, i) + } h.ranges.Remove(el) - } else { + } else { // no ranges affected. Nothing to do return } } } +// IsDuplicate determines if a packet should be regarded as a duplicate packet +// note that after receiving a StopWaitingFrame, all packets below the LeastUnacked should be regarded as duplicates, even if the packet was just delayed +func (h *receivedPacketHistory) IsDuplicate(p protocol.PacketNumber) bool { + h.mutex.RLock() + defer h.mutex.RUnlock() + + if p < h.lowestInReceivedPacketNumbers { + return true + } + + _, ok := h.receivedPacketNumbers[p] + return ok +} + // GetAckRanges gets a slice of all AckRanges that can be used in an AckFrame func (h *receivedPacketHistory) GetAckRanges() []frames.AckRange { h.mutex.RLock() diff --git a/ackhandler/received_packet_history_test.go b/ackhandler/received_packet_history_test.go index c7b63f3d..ed421e25 100644 --- a/ackhandler/received_packet_history_test.go +++ b/ackhandler/received_packet_history_test.go @@ -17,17 +17,51 @@ var _ = Describe("receivedPacketHistory", func() { hist = newReceivedPacketHistory() }) + // check if the ranges PacketIntervalList contains exactly the same packet number as the receivedPacketNumbers + historiesConsistent := func() bool { + // check if a packet number is contained in any of the ranges + containedInRanges := func(p protocol.PacketNumber) bool { + for el := hist.ranges.Front(); el != nil; el = el.Next() { + if p >= el.Value.Start && p <= el.Value.End { + return true + } + } + return false + } + + // first check if all packets contained in the ranges are present in the map + for el := hist.ranges.Front(); el != nil; el = el.Next() { + for i := el.Value.Start; i <= el.Value.Start; i++ { + _, ok := hist.receivedPacketNumbers[i] + if !ok { + return false + } + } + } + + // then check if all packets in the map are contained in any of the ranges + for i := range hist.receivedPacketNumbers { + if !containedInRanges(i) { + return false + } + } + + return true + } + Context("ranges", func() { It("adds the first packet", func() { hist.ReceivedPacket(4) Expect(hist.ranges.Len()).To(Equal(1)) Expect(hist.ranges.Front().Value).To(Equal(utils.PacketInterval{Start: 4, End: 4})) + Expect(historiesConsistent()).To(BeTrue()) }) It("doesn't care about duplicate packets", func() { hist.ReceivedPacket(4) Expect(hist.ranges.Len()).To(Equal(1)) Expect(hist.ranges.Front().Value).To(Equal(utils.PacketInterval{Start: 4, End: 4})) + Expect(historiesConsistent()).To(BeTrue()) }) It("adds a few consecutive packets", func() { @@ -36,6 +70,7 @@ var _ = Describe("receivedPacketHistory", func() { hist.ReceivedPacket(6) Expect(hist.ranges.Len()).To(Equal(1)) Expect(hist.ranges.Front().Value).To(Equal(utils.PacketInterval{Start: 4, End: 6})) + Expect(historiesConsistent()).To(BeTrue()) }) It("doesn't care about a duplicate packet contained in an existing range", func() { @@ -45,6 +80,7 @@ var _ = Describe("receivedPacketHistory", func() { hist.ReceivedPacket(5) Expect(hist.ranges.Len()).To(Equal(1)) Expect(hist.ranges.Front().Value).To(Equal(utils.PacketInterval{Start: 4, End: 6})) + Expect(historiesConsistent()).To(BeTrue()) }) It("extends a range at the front", func() { @@ -52,6 +88,7 @@ var _ = Describe("receivedPacketHistory", func() { hist.ReceivedPacket(3) Expect(hist.ranges.Len()).To(Equal(1)) Expect(hist.ranges.Front().Value).To(Equal(utils.PacketInterval{Start: 3, End: 4})) + Expect(historiesConsistent()).To(BeTrue()) }) It("creates a new range when a packet is lost", func() { @@ -60,6 +97,7 @@ var _ = Describe("receivedPacketHistory", func() { Expect(hist.ranges.Len()).To(Equal(2)) Expect(hist.ranges.Front().Value).To(Equal(utils.PacketInterval{Start: 4, End: 4})) Expect(hist.ranges.Back().Value).To(Equal(utils.PacketInterval{Start: 6, End: 6})) + Expect(historiesConsistent()).To(BeTrue()) }) It("creates a new range in between two ranges", func() { @@ -71,6 +109,7 @@ var _ = Describe("receivedPacketHistory", func() { Expect(hist.ranges.Front().Value).To(Equal(utils.PacketInterval{Start: 4, End: 4})) Expect(hist.ranges.Front().Next().Value).To(Equal(utils.PacketInterval{Start: 7, End: 7})) Expect(hist.ranges.Back().Value).To(Equal(utils.PacketInterval{Start: 10, End: 10})) + Expect(historiesConsistent()).To(BeTrue()) }) It("creates a new range before an existing range for a belated packet", func() { @@ -79,6 +118,7 @@ var _ = Describe("receivedPacketHistory", func() { Expect(hist.ranges.Len()).To(Equal(2)) Expect(hist.ranges.Front().Value).To(Equal(utils.PacketInterval{Start: 4, End: 4})) Expect(hist.ranges.Back().Value).To(Equal(utils.PacketInterval{Start: 6, End: 6})) + Expect(historiesConsistent()).To(BeTrue()) }) It("extends a previous range at the end", func() { @@ -88,6 +128,7 @@ var _ = Describe("receivedPacketHistory", func() { Expect(hist.ranges.Len()).To(Equal(2)) Expect(hist.ranges.Front().Value).To(Equal(utils.PacketInterval{Start: 4, End: 5})) Expect(hist.ranges.Back().Value).To(Equal(utils.PacketInterval{Start: 7, End: 7})) + Expect(historiesConsistent()).To(BeTrue()) }) It("extends a range at the front", func() { @@ -97,6 +138,7 @@ var _ = Describe("receivedPacketHistory", func() { Expect(hist.ranges.Len()).To(Equal(2)) Expect(hist.ranges.Front().Value).To(Equal(utils.PacketInterval{Start: 4, End: 4})) Expect(hist.ranges.Back().Value).To(Equal(utils.PacketInterval{Start: 6, End: 7})) + Expect(historiesConsistent()).To(BeTrue()) }) It("closes a range", func() { @@ -106,6 +148,7 @@ var _ = Describe("receivedPacketHistory", func() { hist.ReceivedPacket(5) Expect(hist.ranges.Len()).To(Equal(1)) Expect(hist.ranges.Front().Value).To(Equal(utils.PacketInterval{Start: 4, End: 6})) + Expect(historiesConsistent()).To(BeTrue()) }) It("closes a range in the middle", func() { @@ -119,6 +162,7 @@ var _ = Describe("receivedPacketHistory", func() { Expect(hist.ranges.Front().Value).To(Equal(utils.PacketInterval{Start: 1, End: 1})) Expect(hist.ranges.Front().Next().Value).To(Equal(utils.PacketInterval{Start: 4, End: 6})) Expect(hist.ranges.Back().Value).To(Equal(utils.PacketInterval{Start: 10, End: 10})) + Expect(historiesConsistent()).To(BeTrue()) }) }) @@ -126,6 +170,7 @@ var _ = Describe("receivedPacketHistory", func() { It("does nothing when the history is empty", func() { hist.DeleteBelow(5) Expect(hist.ranges.Len()).To(BeZero()) + Expect(historiesConsistent()).To(BeTrue()) }) It("deletes a range", func() { @@ -135,6 +180,7 @@ var _ = Describe("receivedPacketHistory", func() { hist.DeleteBelow(6) Expect(hist.ranges.Len()).To(Equal(1)) Expect(hist.ranges.Front().Value).To(Equal(utils.PacketInterval{Start: 10, End: 10})) + Expect(historiesConsistent()).To(BeTrue()) }) It("deletes multiple ranges", func() { @@ -144,6 +190,7 @@ var _ = Describe("receivedPacketHistory", func() { hist.DeleteBelow(8) Expect(hist.ranges.Len()).To(Equal(1)) Expect(hist.ranges.Front().Value).To(Equal(utils.PacketInterval{Start: 10, End: 10})) + Expect(historiesConsistent()).To(BeTrue()) }) It("adjusts a range, if leastUnacked lies inside it", func() { @@ -154,6 +201,7 @@ var _ = Describe("receivedPacketHistory", func() { hist.DeleteBelow(4) Expect(hist.ranges.Len()).To(Equal(1)) Expect(hist.ranges.Front().Value).To(Equal(utils.PacketInterval{Start: 4, End: 6})) + Expect(historiesConsistent()).To(BeTrue()) }) It("adjusts a range, if leastUnacked is the last of the range", func() { @@ -164,6 +212,7 @@ var _ = Describe("receivedPacketHistory", func() { Expect(hist.ranges.Len()).To(Equal(2)) Expect(hist.ranges.Front().Value).To(Equal(utils.PacketInterval{Start: 5, End: 5})) Expect(hist.ranges.Back().Value).To(Equal(utils.PacketInterval{Start: 10, End: 10})) + Expect(historiesConsistent()).To(BeTrue()) }) It("keeps a one-packet range, if leastUnacked is exactly that value", func() { @@ -171,6 +220,7 @@ var _ = Describe("receivedPacketHistory", func() { hist.DeleteBelow(4) Expect(hist.ranges.Len()).To(Equal(1)) Expect(hist.ranges.Front().Value).To(Equal(utils.PacketInterval{Start: 4, End: 4})) + Expect(historiesConsistent()).To(BeTrue()) }) Context("DoS protection", func() { @@ -181,6 +231,18 @@ var _ = Describe("receivedPacketHistory", func() { } err := hist.ReceivedPacket(2*protocol.MaxTrackedReceivedAckRanges + 2) Expect(err).To(MatchError(errTooManyOutstandingReceivedAckRanges)) + Expect(historiesConsistent()).To(BeTrue()) + }) + + It("doesn't store more than MaxTrackedReceivedPackets packets", func() { + err := hist.ReceivedPacket(1) + Expect(err).ToNot(HaveOccurred()) + for i := protocol.PacketNumber(3); i < 3+protocol.MaxTrackedReceivedPackets-1; i++ { + err := hist.ReceivedPacket(protocol.PacketNumber(i)) + Expect(err).ToNot(HaveOccurred()) + } + err = hist.ReceivedPacket(protocol.PacketNumber(protocol.MaxTrackedReceivedPackets) + 10) + Expect(err).To(MatchError(errTooManyOutstandingReceivedPackets)) }) It("doesn't consider already deleted ranges for MaxTrackedReceivedAckRanges", func() { @@ -193,10 +255,38 @@ var _ = Describe("receivedPacketHistory", func() { hist.DeleteBelow(protocol.MaxTrackedReceivedAckRanges) // deletes about half of the ranges err = hist.ReceivedPacket(2*protocol.MaxTrackedReceivedAckRanges + 4) Expect(err).ToNot(HaveOccurred()) + Expect(historiesConsistent()).To(BeTrue()) }) }) }) + Context("duplicate packet detection", func() { + It("detects duplicates for existing ranges", func() { + hist.ReceivedPacket(2) + hist.ReceivedPacket(4) + hist.ReceivedPacket(5) + Expect(hist.IsDuplicate(1)).To(BeFalse()) + Expect(hist.IsDuplicate(2)).To(BeTrue()) + Expect(hist.IsDuplicate(3)).To(BeFalse()) + Expect(hist.IsDuplicate(4)).To(BeTrue()) + Expect(hist.IsDuplicate(5)).To(BeTrue()) + Expect(hist.IsDuplicate(6)).To(BeFalse()) + }) + + It("detects duplicates after a range has been deleted", func() { + hist.ReceivedPacket(2) + hist.ReceivedPacket(3) + hist.ReceivedPacket(6) + hist.DeleteBelow(5) + for i := 1; i < 5; i++ { + Expect(hist.IsDuplicate(protocol.PacketNumber(i))).To(BeTrue()) + } + Expect(hist.IsDuplicate(5)).To(BeFalse()) + Expect(hist.IsDuplicate(6)).To(BeTrue()) + Expect(hist.IsDuplicate(7)).To(BeFalse()) + }) + }) + Context("ACK range export", func() { It("returns nil if there are no ranges", func() { Expect(hist.GetAckRanges()).To(BeNil())