From 7a532326ec29026ae5686e6324c909c6375b9881 Mon Sep 17 00:00:00 2001 From: Marten Seemann Date: Fri, 14 Feb 2020 14:43:31 +0700 Subject: [PATCH] don't pack ACK frames in the second part of a coalesced packet This prevents a possible overflow of the maximum packet size if the ACK frames ends up being really large. --- packet_packer.go | 21 ++++++++++++++------- packet_packer_test.go | 38 ++++++++++++++++++++++++++++++++++++-- 2 files changed, 50 insertions(+), 9 deletions(-) diff --git a/packet_packer.go b/packet_packer.go index 4128d3e3..2e0c3b2b 100644 --- a/packet_packer.go +++ b/packet_packer.go @@ -365,7 +365,11 @@ func (p *packetPacker) maybeAppendCryptoPacket(buffer *packetBuffer, encLevel pr return nil, err } } - ack := p.acks.GetAckFrame(encLevel) + + var ack *wire.AckFrame + if encLevel != protocol.EncryptionHandshake || buffer.Len() == 0 { + ack = p.acks.GetAckFrame(encLevel) + } if !s.HasData() && !hasRetransmission && ack == nil { // nothing to send return nil, nil @@ -430,7 +434,7 @@ func (p *packetPacker) maybeAppendAppDataPacket(buffer *packetBuffer) (*packetCo headerLen := header.GetLength(p.version) maxSize := p.maxPacketSize - buffer.Len() - protocol.ByteCount(sealer.Overhead()) - headerLen - payload := p.composeNextPacket(maxSize) + payload := p.composeNextPacket(maxSize, encLevel != protocol.Encryption0RTT && buffer.Len() == 0) // check if we have anything to send if len(payload.frames) == 0 && payload.ack == nil { @@ -452,13 +456,16 @@ func (p *packetPacker) maybeAppendAppDataPacket(buffer *packetBuffer) (*packetCo return p.appendPacket(buffer, header, payload, encLevel, sealer) } -func (p *packetPacker) composeNextPacket(maxFrameSize protocol.ByteCount) payload { +func (p *packetPacker) composeNextPacket(maxFrameSize protocol.ByteCount, ackAllowed bool) payload { var payload payload - // TODO: we don't need to request ACKs when sending 0-RTT packets - if ack := p.acks.GetAckFrame(protocol.Encryption1RTT); ack != nil { - payload.ack = ack - payload.length += ack.Length(p.version) + var ack *wire.AckFrame + if ackAllowed { + ack = p.acks.GetAckFrame(protocol.Encryption1RTT) + if ack != nil { + payload.ack = ack + payload.length += ack.Length(p.version) + } } for { diff --git a/packet_packer_test.go b/packet_packer_test.go index d980125f..1456252a 100644 --- a/packet_packer_test.go +++ b/packet_packer_test.go @@ -647,7 +647,7 @@ var _ = Describe("Packet packer", func() { checkLength(p.buffer.Data) }) - It("packs a coalesced packet", func() { + It("packs a coalesced packet with Initial / Handshake", func() { pnManager.EXPECT().PeekPacketNumber(protocol.EncryptionInitial).Return(protocol.PacketNumber(0x24), protocol.PacketNumberLen2) pnManager.EXPECT().PopPacketNumber(protocol.EncryptionInitial).Return(protocol.PacketNumber(0x24)) pnManager.EXPECT().PeekPacketNumber(protocol.EncryptionHandshake).Return(protocol.PacketNumber(0x42), protocol.PacketNumberLen2) @@ -656,7 +656,7 @@ var _ = Describe("Packet packer", func() { sealingManager.EXPECT().GetHandshakeSealer().Return(getSealer(), nil) sealingManager.EXPECT().Get1RTTSealer().Return(nil, handshake.ErrKeysNotYetAvailable) ackFramer.EXPECT().GetAckFrame(protocol.EncryptionInitial) - ackFramer.EXPECT().GetAckFrame(protocol.EncryptionHandshake) + // don't EXPECT any calls for a Handshake ACK frame initialStream.EXPECT().HasData().Return(true).Times(2) initialStream.EXPECT().PopCryptoFrame(gomock.Any()).DoAndReturn(func(size protocol.ByteCount) *wire.CryptoFrame { return &wire.CryptoFrame{Offset: 0x42, Data: []byte("initial")} @@ -683,6 +683,40 @@ var _ = Describe("Packet packer", func() { Expect(rest).To(BeEmpty()) }) + It("packs a coalesced packet with Handshake / 1-RTT", func() { + pnManager.EXPECT().PeekPacketNumber(protocol.EncryptionHandshake).Return(protocol.PacketNumber(0x24), protocol.PacketNumberLen2) + pnManager.EXPECT().PopPacketNumber(protocol.EncryptionHandshake).Return(protocol.PacketNumber(0x24)) + pnManager.EXPECT().PeekPacketNumber(protocol.Encryption1RTT).Return(protocol.PacketNumber(0x42), protocol.PacketNumberLen2) + pnManager.EXPECT().PopPacketNumber(protocol.Encryption1RTT).Return(protocol.PacketNumber(0x42)) + sealingManager.EXPECT().GetInitialSealer().Return(nil, handshake.ErrKeysDropped) + sealingManager.EXPECT().GetHandshakeSealer().Return(getSealer(), nil) + sealingManager.EXPECT().Get1RTTSealer().Return(getSealer(), nil) + ackFramer.EXPECT().GetAckFrame(protocol.EncryptionHandshake) + // don't EXPECT any calls for a 1-RTT ACK frame + handshakeStream.EXPECT().HasData().Return(true).Times(2) + handshakeStream.EXPECT().PopCryptoFrame(gomock.Any()).DoAndReturn(func(size protocol.ByteCount) *wire.CryptoFrame { + return &wire.CryptoFrame{Offset: 0x1337, Data: []byte("handshake")} + }) + expectAppendControlFrames() + expectAppendStreamFrames(ackhandler.Frame{Frame: &wire.StreamFrame{Data: []byte("foobar")}}) + p, err := packer.PackCoalescedPacket() + Expect(err).ToNot(HaveOccurred()) + Expect(p.packets).To(HaveLen(2)) + Expect(p.packets[0].EncryptionLevel()).To(Equal(protocol.EncryptionHandshake)) + Expect(p.packets[0].frames).To(HaveLen(1)) + Expect(p.packets[0].frames[0].Frame.(*wire.CryptoFrame).Data).To(Equal([]byte("handshake"))) + Expect(p.packets[1].EncryptionLevel()).To(Equal(protocol.Encryption1RTT)) + Expect(p.packets[1].frames).To(HaveLen(1)) + Expect(p.packets[1].frames[0].Frame.(*wire.StreamFrame).Data).To(Equal([]byte("foobar"))) + hdr, _, rest, err := wire.ParsePacket(p.buffer.Data, 0) + Expect(err).ToNot(HaveOccurred()) + Expect(hdr.Type).To(Equal(protocol.PacketTypeHandshake)) + hdr, _, rest, err = wire.ParsePacket(rest, 0) + Expect(err).ToNot(HaveOccurred()) + Expect(hdr.IsLongHeader).To(BeFalse()) + Expect(rest).To(BeEmpty()) + }) + It("doesn't add a coalesced packet if the remaining size is smaller than MaxCoalescedPacketSize", func() { pnManager.EXPECT().PeekPacketNumber(protocol.EncryptionInitial).Return(protocol.PacketNumber(0x24), protocol.PacketNumberLen2) pnManager.EXPECT().PopPacketNumber(protocol.EncryptionInitial).Return(protocol.PacketNumber(0x24))