diff --git a/internal/ackhandler/received_packet_history.go b/internal/ackhandler/received_packet_history.go index e75b0b0e..b2678806 100644 --- a/internal/ackhandler/received_packet_history.go +++ b/internal/ackhandler/received_packet_history.go @@ -2,7 +2,6 @@ package ackhandler import ( "github.com/lucas-clemente/quic-go/internal/protocol" - "github.com/lucas-clemente/quic-go/internal/qerr" "github.com/lucas-clemente/quic-go/internal/utils" "github.com/lucas-clemente/quic-go/internal/wire" ) @@ -16,9 +15,6 @@ type receivedPacketHistory struct { deletedBelow protocol.PacketNumber } -var errTooManyOutstandingReceivedAckRanges = qerr.Error(qerr.InternalError, "Too many outstanding received ACK ranges") - -// newReceivedPacketHistory creates a new received packet history func newReceivedPacketHistory() *receivedPacketHistory { return &receivedPacketHistory{ ranges: utils.NewPacketIntervalList(), @@ -31,10 +27,14 @@ func (h *receivedPacketHistory) ReceivedPacket(p protocol.PacketNumber) error { if p < h.deletedBelow { return nil } - if h.ranges.Len() >= protocol.MaxTrackedReceivedAckRanges { - return errTooManyOutstandingReceivedAckRanges + if err := h.addToRanges(p); err != nil { + return err } + h.maybeDeleteOldRanges() + return nil +} +func (h *receivedPacketHistory) addToRanges(p protocol.PacketNumber) error { if h.ranges.Len() == 0 { h.ranges.PushBack(utils.PacketInterval{Start: p, End: p}) return nil @@ -75,10 +75,17 @@ func (h *receivedPacketHistory) ReceivedPacket(p protocol.PacketNumber) error { // create a new range at the beginning h.ranges.InsertBefore(utils.PacketInterval{Start: p, End: p}, h.ranges.Front()) - return nil } +// Delete old ranges, if we're tracking more than 500 of them. +// This is a DoS defense against a peer that sends us too many gaps. +func (h *receivedPacketHistory) maybeDeleteOldRanges() { + for h.ranges.Len() > protocol.MaxNumAckRanges { + h.ranges.Remove(h.ranges.Front()) + } +} + // DeleteBelow deletes all entries below (but not including) p func (h *receivedPacketHistory) DeleteBelow(p protocol.PacketNumber) { if p < h.deletedBelow { diff --git a/internal/ackhandler/received_packet_history_test.go b/internal/ackhandler/received_packet_history_test.go index db3afb08..c02317d4 100644 --- a/internal/ackhandler/received_packet_history_test.go +++ b/internal/ackhandler/received_packet_history_test.go @@ -186,27 +186,18 @@ var _ = Describe("receivedPacketHistory", func() { Expect(hist.ranges.Front().Value).To(Equal(utils.PacketInterval{Start: 5, End: 6})) }) - Context("DoS protection", func() { - It("doesn't create more than MaxTrackedReceivedAckRanges ranges", func() { - for i := protocol.PacketNumber(1); i <= protocol.MaxTrackedReceivedAckRanges; i++ { - err := hist.ReceivedPacket(2 * i) - Expect(err).ToNot(HaveOccurred()) - } - err := hist.ReceivedPacket(2*protocol.MaxTrackedReceivedAckRanges + 2) - Expect(err).To(MatchError(errTooManyOutstandingReceivedAckRanges)) - }) - - It("doesn't consider already deleted ranges for MaxTrackedReceivedAckRanges", func() { - for i := protocol.PacketNumber(1); i <= protocol.MaxTrackedReceivedAckRanges; i++ { - err := hist.ReceivedPacket(2 * i) - Expect(err).ToNot(HaveOccurred()) - } - err := hist.ReceivedPacket(2*protocol.MaxTrackedReceivedAckRanges + 2) - Expect(err).To(MatchError(errTooManyOutstandingReceivedAckRanges)) - hist.DeleteBelow(protocol.MaxTrackedReceivedAckRanges) // deletes about half of the ranges - err = hist.ReceivedPacket(2*protocol.MaxTrackedReceivedAckRanges + 4) + It("doesn't create more than MaxNumAckRanges ranges", func() { + for i := protocol.PacketNumber(0); i < protocol.MaxNumAckRanges; i++ { + err := hist.ReceivedPacket(2 * i) Expect(err).ToNot(HaveOccurred()) - }) + } + Expect(hist.ranges.Len()).To(Equal(protocol.MaxNumAckRanges)) + Expect(hist.ranges.Front().Value).To(Equal(utils.PacketInterval{Start: 0, End: 0})) + err := hist.ReceivedPacket(2*protocol.MaxNumAckRanges + 1000) + Expect(err).ToNot(HaveOccurred()) + // check that the oldest ACK range was deleted + Expect(hist.ranges.Len()).To(Equal(protocol.MaxNumAckRanges)) + Expect(hist.ranges.Front().Value).To(Equal(utils.PacketInterval{Start: 2, End: 2})) }) }) diff --git a/internal/ackhandler/received_packet_tracker_test.go b/internal/ackhandler/received_packet_tracker_test.go index a06a2e2a..ee297c94 100644 --- a/internal/ackhandler/received_packet_tracker_test.go +++ b/internal/ackhandler/received_packet_tracker_test.go @@ -59,19 +59,6 @@ var _ = Describe("Received Packet Tracker", func() { Expect(tracker.largestObserved).To(Equal(protocol.PacketNumber(5))) Expect(tracker.largestObservedReceivedTime).To(Equal(timestamp)) }) - - It("passes on errors from receivedPacketHistory", func() { - var err error - for i := protocol.PacketNumber(0); i < 5*protocol.MaxTrackedReceivedAckRanges; i++ { - err = tracker.ReceivedPacket(2*i+1, time.Time{}, true) - // this will eventually return an error - // details about when exactly the receivedPacketHistory errors are tested there - if err != nil { - break - } - } - Expect(err).To(MatchError(errTooManyOutstandingReceivedAckRanges)) - }) }) Context("ACKs", func() { diff --git a/internal/protocol/params.go b/internal/protocol/params.go index 8a6d3444..dd191de2 100644 --- a/internal/protocol/params.go +++ b/internal/protocol/params.go @@ -73,9 +73,6 @@ const MaxOutstandingSentPackets = 2 * defaultMaxCongestionWindowPackets // This value *must* be larger than MaxOutstandingSentPackets. const MaxTrackedSentPackets = MaxOutstandingSentPackets * 5 / 4 -// MaxTrackedReceivedAckRanges is the maximum number of ACK ranges tracked -const MaxTrackedReceivedAckRanges = defaultMaxCongestionWindowPackets - // MaxNonAckElicitingAcks is the maximum number of packets containing an ACK, // but no ack-eliciting frames, that we send in a row const MaxNonAckElicitingAcks = 19 @@ -117,6 +114,11 @@ const MaxPostHandshakeCryptoFrameSize ByteCount = 1000 // but must ensure that a maximum size ACK frame fits into one packet. const MaxAckFrameSize ByteCount = 1000 +// MaxNumAckRanges is the maximum number of ACK ranges that we send in an ACK frame. +// It also serves as a limit for the packet history. +// If at any point we keep track of more ranges, old ranges are discarded. +const MaxNumAckRanges = 500 + // MinPacingDelay is the minimum duration that is used for packet pacing // If the packet packing frequency is higher, multiple packets might be sent at once. // Example: For a packet pacing delay of 20 microseconds, we would send 5 packets at once, wait for 100 microseconds, and so forth.