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 9fcead66e..35d97a000 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 367d53553..75922d4d4 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 985c6ce62..28fed5f81 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 7b2fe69b1..6bf398c9d 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 1cf986e73..d55d9bf55 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 1b5337ae1..5cd973e33 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 45026e3d1..064c62b96 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 07b4a1187..ca7825133 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 040bc12a6..8a02d902e 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 86a5afbbc..bb07039a5 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 })