From 440ff107a3aa7e5f03a3b171ff41aa50d3c75962 Mon Sep 17 00:00:00 2001 From: Marten Seemann Date: Wed, 27 May 2020 09:20:51 +0700 Subject: [PATCH] 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 | 1 + .../ackhandler/received_packet_handler.go | 18 +++++++ .../received_packet_handler_test.go | 22 ++++++++ .../ackhandler/received_packet_history.go | 15 ++++++ .../received_packet_history_test.go | 51 +++++++++++++++++++ .../ackhandler/received_packet_tracker.go | 4 ++ .../ackhandler/received_packet_handler.go | 14 +++++ qlog/types.go | 4 ++ session.go | 8 +++ session_test.go | 37 ++++++++++++-- 10 files changed, 170 insertions(+), 4 deletions(-) diff --git a/internal/ackhandler/interfaces.go b/internal/ackhandler/interfaces.go index 9fcead66..35d97a00 100644 --- a/internal/ackhandler/interfaces.go +++ b/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) diff --git a/internal/ackhandler/received_packet_handler.go b/internal/ackhandler/received_packet_handler.go index 367d5355..75922d4d 100644 --- a/internal/ackhandler/received_packet_handler.go +++ b/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") +} diff --git a/internal/ackhandler/received_packet_handler_test.go b/internal/ackhandler/received_packet_handler_test.go index 985c6ce6..28fed5f8 100644 --- a/internal/ackhandler/received_packet_handler_test.go +++ b/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()) + }) }) diff --git a/internal/ackhandler/received_packet_history.go b/internal/ackhandler/received_packet_history.go index 7b2fe69b..6bf398c9 100644 --- a/internal/ackhandler/received_packet_history.go +++ b/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 +} diff --git a/internal/ackhandler/received_packet_history_test.go b/internal/ackhandler/received_packet_history_test.go index 1cf986e7..d55d9bf5 100644 --- a/internal/ackhandler/received_packet_history_test.go +++ b/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()) + }) + }) }) diff --git a/internal/ackhandler/received_packet_tracker.go b/internal/ackhandler/received_packet_tracker.go index 1b5337ae..5cd973e3 100644 --- a/internal/ackhandler/received_packet_tracker.go +++ b/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) +} diff --git a/internal/mocks/ackhandler/received_packet_handler.go b/internal/mocks/ackhandler/received_packet_handler.go index 45026e3d..064c62b9 100644 --- a/internal/mocks/ackhandler/received_packet_handler.go +++ b/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() diff --git a/qlog/types.go b/qlog/types.go index 07b4a118..ca782513 100644 --- a/qlog/types.go +++ b/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") } diff --git a/session.go b/session.go index 040bc12a..8a02d902 100644 --- a/session.go +++ b/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 diff --git a/session_test.go b/session_test.go index 86a5afbb..bb07039a 100644 --- a/session_test.go +++ b/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 })