drop duplicate packets

Duplicate detection uses the same data structure that is used to track
received packets to generate ACK frames. That means that after an old
ACK range has been pruned, a severly delayed packet might be incorrectly
detected as a duplicate.
As we wouldn't have acknowledged receipt of this packet, this situation
would have resulted in a retransmission by the peer anyway, so dropping
the packet won't cause a big regression.

internal/ackhandler/interfaces.go

@@ -60,6 +60,7 @@ type sentPacketTracker interface {
 
 // ReceivedPacketHandler handles ACKs needed to send for incoming packets
 type ReceivedPacketHandler interface {
+	IsPotentiallyDuplicate(protocol.PacketNumber, protocol.EncryptionLevel) bool
 	ReceivedPacket(pn protocol.PacketNumber, encLevel protocol.EncryptionLevel, rcvTime time.Time, shouldInstigateAck bool) error
 	DropPackets(protocol.EncryptionLevel)
 

internal/ackhandler/received_packet_handler.go

@@ -136,3 +136,21 @@ func (h *receivedPacketHandler) GetAckFrame(encLevel protocol.EncryptionLevel) *
 	}
 	return ack
 }
+
+func (h *receivedPacketHandler) IsPotentiallyDuplicate(pn protocol.PacketNumber, encLevel protocol.EncryptionLevel) bool {
+	switch encLevel {
+	case protocol.EncryptionInitial:
+		if h.initialPackets != nil {
+			return h.initialPackets.IsPotentiallyDuplicate(pn)
+		}
+	case protocol.EncryptionHandshake:
+		if h.handshakePackets != nil {
+			return h.handshakePackets.IsPotentiallyDuplicate(pn)
+		}
+	case protocol.Encryption0RTT, protocol.Encryption1RTT:
+		if h.appDataPackets != nil {
+			return h.appDataPackets.IsPotentiallyDuplicate(pn)
+		}
+	}
+	panic("unexpected encryption level")
+}

internal/ackhandler/received_packet_handler_test.go

@@ -122,4 +122,26 @@ var _ = Describe("Received Packet Handler", func() {
 		Expect(ack.LowestAcked()).To(Equal(protocol.PacketNumber(2)))
 		Expect(ack.LargestAcked()).To(Equal(protocol.PacketNumber(4)))
 	})
+
+	It("says if packets are duplicates", func() {
+		sendTime := time.Now()
+		sentPackets.EXPECT().GetLowestPacketNotConfirmedAcked().AnyTimes()
+		// Initial
+		Expect(handler.IsPotentiallyDuplicate(3, protocol.EncryptionInitial)).To(BeFalse())
+		Expect(handler.ReceivedPacket(3, protocol.EncryptionInitial, sendTime, true)).To(Succeed())
+		Expect(handler.IsPotentiallyDuplicate(3, protocol.EncryptionInitial)).To(BeTrue())
+		// Handshake
+		Expect(handler.IsPotentiallyDuplicate(3, protocol.EncryptionHandshake)).To(BeFalse())
+		Expect(handler.ReceivedPacket(3, protocol.EncryptionHandshake, sendTime, true)).To(Succeed())
+		Expect(handler.IsPotentiallyDuplicate(3, protocol.EncryptionHandshake)).To(BeTrue())
+		// 0-RTT
+		Expect(handler.IsPotentiallyDuplicate(3, protocol.Encryption0RTT)).To(BeFalse())
+		Expect(handler.ReceivedPacket(3, protocol.Encryption0RTT, sendTime, true)).To(Succeed())
+		Expect(handler.IsPotentiallyDuplicate(3, protocol.Encryption0RTT)).To(BeTrue())
+		// 1-RTT
+		Expect(handler.IsPotentiallyDuplicate(3, protocol.Encryption1RTT)).To(BeTrue())
+		Expect(handler.IsPotentiallyDuplicate(4, protocol.Encryption1RTT)).To(BeFalse())
+		Expect(handler.ReceivedPacket(4, protocol.Encryption1RTT, sendTime, true)).To(Succeed())
+		Expect(handler.IsPotentiallyDuplicate(4, protocol.Encryption1RTT)).To(BeTrue())
+	})
 })

internal/ackhandler/received_packet_history.go

@@ -128,3 +128,18 @@ func (h *receivedPacketHistory) GetHighestAckRange() wire.AckRange {
 	}
 	return ackRange
 }
+
+func (h *receivedPacketHistory) IsPotentiallyDuplicate(p protocol.PacketNumber) bool {
+	if p < h.deletedBelow {
+		return true
+	}
+	for el := h.ranges.Back(); el != nil; el = el.Prev() {
+		if p > el.Value.End {
+			return false
+		}
+		if p <= el.Value.End && p >= el.Value.Start {
+			return true
+		}
+	}
+	return false
+}

internal/ackhandler/received_packet_history_test.go

@@ -246,4 +246,55 @@ var _ = Describe("receivedPacketHistory", func() {
 			Expect(hist.GetHighestAckRange()).To(Equal(wire.AckRange{Smallest: 6, Largest: 7}))
 		})
 	})
+
+	Context("duplicate detection", func() {
+		It("doesn't declare the first packet a duplicate", func() {
+			Expect(hist.IsPotentiallyDuplicate(5)).To(BeFalse())
+		})
+
+		It("detects a duplicate in a range", func() {
+			hist.ReceivedPacket(4)
+			hist.ReceivedPacket(5)
+			hist.ReceivedPacket(6)
+			Expect(hist.IsPotentiallyDuplicate(3)).To(BeFalse())
+			Expect(hist.IsPotentiallyDuplicate(4)).To(BeTrue())
+			Expect(hist.IsPotentiallyDuplicate(5)).To(BeTrue())
+			Expect(hist.IsPotentiallyDuplicate(6)).To(BeTrue())
+			Expect(hist.IsPotentiallyDuplicate(7)).To(BeFalse())
+		})
+
+		It("detects a duplicate in multiple ranges", func() {
+			hist.ReceivedPacket(4)
+			hist.ReceivedPacket(5)
+			hist.ReceivedPacket(8)
+			hist.ReceivedPacket(9)
+			Expect(hist.IsPotentiallyDuplicate(3)).To(BeFalse())
+			Expect(hist.IsPotentiallyDuplicate(4)).To(BeTrue())
+			Expect(hist.IsPotentiallyDuplicate(5)).To(BeTrue())
+			Expect(hist.IsPotentiallyDuplicate(6)).To(BeFalse())
+			Expect(hist.IsPotentiallyDuplicate(7)).To(BeFalse())
+			Expect(hist.IsPotentiallyDuplicate(8)).To(BeTrue())
+			Expect(hist.IsPotentiallyDuplicate(9)).To(BeTrue())
+			Expect(hist.IsPotentiallyDuplicate(10)).To(BeFalse())
+		})
+
+		It("says a packet is a potentially duplicate if the ranges were already deleted", func() {
+			hist.ReceivedPacket(4)
+			hist.ReceivedPacket(5)
+			hist.ReceivedPacket(8)
+			hist.ReceivedPacket(9)
+			hist.ReceivedPacket(11)
+			hist.DeleteBelow(8)
+			Expect(hist.IsPotentiallyDuplicate(3)).To(BeTrue())
+			Expect(hist.IsPotentiallyDuplicate(4)).To(BeTrue())
+			Expect(hist.IsPotentiallyDuplicate(5)).To(BeTrue())
+			Expect(hist.IsPotentiallyDuplicate(6)).To(BeTrue())
+			Expect(hist.IsPotentiallyDuplicate(7)).To(BeTrue())
+			Expect(hist.IsPotentiallyDuplicate(8)).To(BeTrue())
+			Expect(hist.IsPotentiallyDuplicate(9)).To(BeTrue())
+			Expect(hist.IsPotentiallyDuplicate(10)).To(BeFalse())
+			Expect(hist.IsPotentiallyDuplicate(11)).To(BeTrue())
+			Expect(hist.IsPotentiallyDuplicate(12)).To(BeFalse())
+		})
+	})
 })

internal/ackhandler/received_packet_tracker.go

@@ -188,3 +188,7 @@ func (h *receivedPacketTracker) GetAckFrame() *wire.AckFrame {
 }
 
 func (h *receivedPacketTracker) GetAlarmTimeout() time.Time { return h.ackAlarm }
+
+func (h *receivedPacketTracker) IsPotentiallyDuplicate(pn protocol.PacketNumber) bool {
+	return h.packetHistory.IsPotentiallyDuplicate(pn)
+}

internal/mocks/ackhandler/received_packet_handler.go

@@ -76,6 +76,20 @@ func (mr *MockReceivedPacketHandlerMockRecorder) GetAlarmTimeout() *gomock.Call
 	return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetAlarmTimeout", reflect.TypeOf((*MockReceivedPacketHandler)(nil).GetAlarmTimeout))
 }
 
+// IsPotentiallyDuplicate mocks base method
+func (m *MockReceivedPacketHandler) IsPotentiallyDuplicate(arg0 protocol.PacketNumber, arg1 protocol.EncryptionLevel) bool {
+	m.ctrl.T.Helper()
+	ret := m.ctrl.Call(m, "IsPotentiallyDuplicate", arg0, arg1)
+	ret0, _ := ret[0].(bool)
+	return ret0
+}
+
+// IsPotentiallyDuplicate indicates an expected call of IsPotentiallyDuplicate
+func (mr *MockReceivedPacketHandlerMockRecorder) IsPotentiallyDuplicate(arg0, arg1 interface{}) *gomock.Call {
+	mr.mock.ctrl.T.Helper()
+	return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "IsPotentiallyDuplicate", reflect.TypeOf((*MockReceivedPacketHandler)(nil).IsPotentiallyDuplicate), arg0, arg1)
+}
+
 // ReceivedPacket mocks base method
 func (m *MockReceivedPacketHandler) ReceivedPacket(arg0 protocol.PacketNumber, arg1 protocol.EncryptionLevel, arg2 time.Time, arg3 bool) error {
 	m.ctrl.T.Helper()

qlog/types.go

@@ -262,6 +262,8 @@ const (
 	PacketDropUnexpectedSourceConnectionID
 	// PacketDropUnexpectedVersion is used when a packet with an unexpected version is received
 	PacketDropUnexpectedVersion
+	// PacketDropDuplicate is used when a duplicate packet is received
+	PacketDropDuplicate
 )
 
 func (r PacketDropReason) String() string {
@@ -286,6 +288,8 @@ func (r PacketDropReason) String() string {
 		return "unexpected_source_connection_id"
 	case PacketDropUnexpectedVersion:
 		return "unexpected_version"
+	case PacketDropDuplicate:
+		return "duplicate"
 	default:
 		panic("unknown packet drop reason")
 	}

session.go

@@ -815,6 +815,14 @@ func (s *session) handleSinglePacket(p *receivedPacket, hdr *wire.Header) bool /
 		packet.hdr.Log(s.logger)
 	}
 
+	if s.receivedPacketHandler.IsPotentiallyDuplicate(packet.packetNumber, packet.encryptionLevel) {
+		s.logger.Debugf("Dropping (potentially) duplicate packet.")
+		if s.qlogger != nil {
+			s.qlogger.DroppedPacket(qlog.PacketTypeFromHeader(hdr), protocol.ByteCount(len(p.data)), qlog.PacketDropDuplicate)
+		}
+		return false
+	}
+
 	if err := s.handleUnpackedPacket(packet, p.rcvTime, protocol.ByteCount(len(p.data))); err != nil {
 		s.closeLocal(err)
 		return false

session_test.go

@@ -666,7 +666,10 @@ var _ = Describe("Session", func() {
 				data:            []byte{0}, // one PADDING frame
 			}, nil)
 			rph := mockackhandler.NewMockReceivedPacketHandler(mockCtrl)
-			rph.EXPECT().ReceivedPacket(protocol.PacketNumber(0x1337), protocol.EncryptionInitial, rcvTime, false)
+			gomock.InOrder(
+				rph.EXPECT().IsPotentiallyDuplicate(protocol.PacketNumber(0x1337), protocol.EncryptionInitial),
+				rph.EXPECT().ReceivedPacket(protocol.PacketNumber(0x1337), protocol.EncryptionInitial, rcvTime, false),
+			)
 			sess.receivedPacketHandler = rph
 			packet.rcvTime = rcvTime
 			qlogger.EXPECT().StartedConnection(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any())
@@ -691,7 +694,10 @@ var _ = Describe("Session", func() {
 				data:            buf.Bytes(),
 			}, nil)
 			rph := mockackhandler.NewMockReceivedPacketHandler(mockCtrl)
-			rph.EXPECT().ReceivedPacket(protocol.PacketNumber(0x1337), protocol.Encryption1RTT, rcvTime, true)
+			gomock.InOrder(
+				rph.EXPECT().IsPotentiallyDuplicate(protocol.PacketNumber(0x1337), protocol.Encryption1RTT),
+				rph.EXPECT().ReceivedPacket(protocol.PacketNumber(0x1337), protocol.Encryption1RTT, rcvTime, true),
+			)
 			sess.receivedPacketHandler = rph
 			packet.rcvTime = rcvTime
 			qlogger.EXPECT().StartedConnection(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any())
@@ -699,6 +705,26 @@ var _ = Describe("Session", func() {
 			Expect(sess.handlePacketImpl(packet)).To(BeTrue())
 		})
 
+		It("drops duplicate packets", func() {
+			hdr := &wire.ExtendedHeader{
+				Header:          wire.Header{DestConnectionID: srcConnID},
+				PacketNumber:    0x37,
+				PacketNumberLen: protocol.PacketNumberLen1,
+			}
+			packet := getPacket(hdr, nil)
+			unpacker.EXPECT().Unpack(gomock.Any(), gomock.Any(), gomock.Any()).Return(&unpackedPacket{
+				packetNumber:    0x1337,
+				encryptionLevel: protocol.Encryption1RTT,
+				hdr:             hdr,
+				data:            []byte("foobar"),
+			}, nil)
+			rph := mockackhandler.NewMockReceivedPacketHandler(mockCtrl)
+			rph.EXPECT().IsPotentiallyDuplicate(protocol.PacketNumber(0x1337), protocol.Encryption1RTT).Return(true)
+			sess.receivedPacketHandler = rph
+			qlogger.EXPECT().DroppedPacket(qlog.PacketType1RTT, protocol.ByteCount(len(packet.data)), qlog.PacketDropDuplicate)
+			Expect(sess.handlePacketImpl(packet)).To(BeFalse())
+		})
+
 		It("drops a packet when unpacking fails", func() {
 			unpacker.EXPECT().Unpack(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, handshake.ErrDecryptionFailed)
 			streamManager.EXPECT().CloseWithError(gomock.Any())
@@ -784,8 +810,9 @@ var _ = Describe("Session", func() {
 
 		It("rejects packets with empty payload", func() {
 			unpacker.EXPECT().Unpack(gomock.Any(), gomock.Any(), gomock.Any()).Return(&unpackedPacket{
-				hdr:  &wire.ExtendedHeader{},
-				data: []byte{}, // no payload
+				hdr:             &wire.ExtendedHeader{},
+				data:            []byte{}, // no payload
+				encryptionLevel: protocol.Encryption1RTT,
 			}, nil)
 			streamManager.EXPECT().CloseWithError(gomock.Any())
 			cryptoSetup.EXPECT().Close()
@@ -933,6 +960,7 @@ var _ = Describe("Session", func() {
 					return &unpackedPacket{
 						encryptionLevel: protocol.EncryptionHandshake,
 						data:            []byte{0},
+						packetNumber:    1,
 						hdr:             &wire.ExtendedHeader{Header: wire.Header{SrcConnectionID: destConnID}},
 					}, nil
 				})
@@ -942,6 +970,7 @@ var _ = Describe("Session", func() {
 					return &unpackedPacket{
 						encryptionLevel: protocol.EncryptionHandshake,
 						data:            []byte{0},
+						packetNumber:    2,
 						hdr:             &wire.ExtendedHeader{Header: wire.Header{SrcConnectionID: destConnID}},
 					}, nil
 				})