From 88e1e50efe40cee2620c767771fcfde647116949 Mon Sep 17 00:00:00 2001 From: Marten Seemann Date: Tue, 16 Aug 2016 16:55:44 +0700 Subject: [PATCH] only increase packet number when sending a packet in PacketPacker fixes #288 --- packet_number_generator.go | 29 +++++++++++++++++++---------- packet_number_generator_test.go | 25 ++++++++++++++++++------- packet_packer.go | 12 ++++++++++-- packet_packer_test.go | 22 +++++++++++++++++++++- session_test.go | 2 +- 5 files changed, 69 insertions(+), 21 deletions(-) diff --git a/packet_number_generator.go b/packet_number_generator.go index 183d7f0b..71ca9a3c 100644 --- a/packet_number_generator.go +++ b/packet_number_generator.go @@ -7,30 +7,39 @@ import ( "github.com/lucas-clemente/quic-go/protocol" ) +// The packetNumberGenerator generates the packet number for the next packet +// it randomly skips a packet number every averagePeriod packets (on average) +// it is guarantued to never skip two consecutive packet numbers type packetNumberGenerator struct { - last protocol.PacketNumber - nextToSkip protocol.PacketNumber averagePeriod protocol.PacketNumber + + next protocol.PacketNumber + nextToSkip protocol.PacketNumber } func newPacketNumberGenerator(averagePeriod protocol.PacketNumber) *packetNumberGenerator { return &packetNumberGenerator{ + next: 1, averagePeriod: averagePeriod, } } -func (p *packetNumberGenerator) GetNextPacketNumber() protocol.PacketNumber { - p.last++ +func (p *packetNumberGenerator) Peek() protocol.PacketNumber { + return p.next +} - if p.last == p.nextToSkip { - return p.GetNextPacketNumber() - } +func (p *packetNumberGenerator) Pop() protocol.PacketNumber { + next := p.next - if p.last > p.nextToSkip { + // generate a new packet number for the next packet + p.next++ + + if p.next == p.nextToSkip { + p.next++ p.generateNewSkip() } - return p.last + return next } func (p *packetNumberGenerator) generateNewSkip() error { @@ -41,7 +50,7 @@ func (p *packetNumberGenerator) generateNewSkip() error { skip := protocol.PacketNumber(num) * (p.averagePeriod - 1) / (math.MaxUint16 / 2) // make sure that there are never two consecutive packet numbers that are skipped - p.nextToSkip = p.last + 2 + skip + p.nextToSkip = p.next + 2 + skip return nil } diff --git a/packet_number_generator_test.go b/packet_number_generator_test.go index ab10d297..f1f87fe5 100644 --- a/packet_number_generator_test.go +++ b/packet_number_generator_test.go @@ -12,27 +12,38 @@ var _ = Describe("Packet Number Generator", func() { var png packetNumberGenerator BeforeEach(func() { - png = packetNumberGenerator{} + png = *newPacketNumberGenerator(100) }) It("gets 1 as the first packet number", func() { - num := png.GetNextPacketNumber() + num := png.Pop() Expect(num).To(Equal(protocol.PacketNumber(1))) }) + It("allows peeking", func() { + png.nextToSkip = 1000 + Expect(png.Peek()).To(Equal(protocol.PacketNumber(1))) + Expect(png.Peek()).To(Equal(protocol.PacketNumber(1))) + num := png.Pop() + Expect(num).To(Equal(protocol.PacketNumber(1))) + Expect(png.Peek()).To(Equal(protocol.PacketNumber(2))) + Expect(png.Peek()).To(Equal(protocol.PacketNumber(2))) + }) + It("skips a packet number", func() { png.nextToSkip = 2 - num := png.GetNextPacketNumber() + num := png.Pop() Expect(num).To(Equal(protocol.PacketNumber(1))) - num = png.GetNextPacketNumber() + Expect(png.Peek()).To(Equal(protocol.PacketNumber(3))) + num = png.Pop() Expect(num).To(Equal(protocol.PacketNumber(3))) }) It("generates a new packet number to skip", func() { - png.last = 100 + png.next = 100 png.averagePeriod = 100 - rep := 1000 + rep := 5000 var sum protocol.PacketNumber for i := 0; i < rep; i++ { @@ -42,7 +53,7 @@ var _ = Describe("Packet Number Generator", func() { } average := sum / protocol.PacketNumber(rep) - Expect(average).To(BeNumerically("==", protocol.PacketNumber(200), 10)) + Expect(average).To(BeNumerically("==", protocol.PacketNumber(200), 4)) }) It("uses random numbers", func() { diff --git a/packet_packer.go b/packet_packer.go index d19394d4..dfb94768 100644 --- a/packet_packer.go +++ b/packet_packer.go @@ -65,7 +65,7 @@ func (p *packetPacker) packPacket(stopWaitingFrame *frames.StopWaitingFrame, con if p.version <= protocol.Version33 { currentPacketNumber = p.lastPacketNumber + 1 } else { - currentPacketNumber = p.packetNumberGenerator.GetNextPacketNumber() + currentPacketNumber = p.packetNumberGenerator.Peek() } // cryptoSetup needs to be locked here, so that the AEADs are not changed between @@ -158,7 +158,15 @@ func (p *packetPacker) packPacket(stopWaitingFrame *frames.StopWaitingFrame, con p.cryptoSetup.Seal(raw[payloadStartIndex:payloadStartIndex], raw[payloadStartIndex:], currentPacketNumber, raw[:payloadStartIndex]) raw = raw[0 : buffer.Len()+12] - p.lastPacketNumber++ + if p.version <= protocol.Version33 { + p.lastPacketNumber++ + } else { + num := p.packetNumberGenerator.Pop() + if num != currentPacketNumber { + return nil, errors.New("PacketPacker BUG: Peeked and Popped packet numbers do not match.") + } + } + return &packedPacket{ number: currentPacketNumber, entropyBit: entropyBit, diff --git a/packet_packer_test.go b/packet_packer_test.go index 9d3b32f4..3c9c3f90 100644 --- a/packet_packer_test.go +++ b/packet_packer_test.go @@ -193,7 +193,9 @@ var _ = Describe("Packet packer", func() { Expect(payloadFrames).To(HaveLen(10)) }) - It("only increases the packet number when there is an actual packet to send", func() { + // TODO: remove once we drop support for QUIC 33 + It("only increases the packet number when there is an actual packet to send, in QUIC 33", func() { + packer.version = protocol.Version33 f := &frames.StreamFrame{ StreamID: 5, Data: []byte{0xDE, 0xCA, 0xFB, 0xAD}, @@ -214,6 +216,24 @@ var _ = Describe("Packet packer", func() { Expect(packer.lastPacketNumber).To(Equal(protocol.PacketNumber(2))) }) + It("only increases the packet number when there is an actual packet to send, in QUIC 34", func() { + packer.packetNumberGenerator.nextToSkip = 1000 + p, err := packer.PackPacket(nil, []frames.Frame{}, 0, true) + Expect(p).To(BeNil()) + Expect(err).ToNot(HaveOccurred()) + Expect(packer.packetNumberGenerator.Peek()).To(Equal(protocol.PacketNumber(1))) + f := &frames.StreamFrame{ + StreamID: 5, + Data: []byte{0xDE, 0xCA, 0xFB, 0xAD}, + } + streamFramer.AddFrameForRetransmission(f) + p, err = packer.PackPacket(nil, []frames.Frame{}, 0, true) + Expect(err).ToNot(HaveOccurred()) + Expect(p).ToNot(BeNil()) + Expect(p.number).To(Equal(protocol.PacketNumber(1))) + Expect(packer.packetNumberGenerator.Peek()).To(Equal(protocol.PacketNumber(2))) + }) + Context("Stream Frame handling", func() { It("does not splits a stream frame with maximum size", func() { f := &frames.StreamFrame{ diff --git a/session_test.go b/session_test.go index 2c32ae41..7919ba74 100644 --- a/session_test.go +++ b/session_test.go @@ -757,7 +757,7 @@ var _ = Describe("Session", func() { if version == protocol.Version33 { session.packer.lastPacketNumber = n } else { - session.packer.packetNumberGenerator.last = n + session.packer.packetNumberGenerator.next = n + 1 } // Now, we send a single packet, and expect that it was retransmitted later err := session.sentPacketHandler.SentPacket(&ackhandlerlegacy.Packet{