From 88337ed8c0204c25ee15efd88e912599d85a5902 Mon Sep 17 00:00:00 2001 From: Marten Seemann Date: Fri, 17 Apr 2020 10:06:32 +0700 Subject: [PATCH 1/2] fix packing of probe packets when retransmissions are canceled --- packet_packer.go | 2 +- packet_packer_test.go | 12 ++++++++++++ session.go | 2 +- 3 files changed, 14 insertions(+), 2 deletions(-) diff --git a/packet_packer.go b/packet_packer.go index debb3a77..c6fa0199 100644 --- a/packet_packer.go +++ b/packet_packer.go @@ -565,7 +565,7 @@ func (p *packetPacker) MaybePackProbePacket(encLevel protocol.EncryptionLevel) ( default: panic("unknown encryption level") } - if err != nil { + if err != nil || contents == nil { return nil, err } if p.perspective == protocol.PerspectiveClient && encLevel == protocol.EncryptionInitial { diff --git a/packet_packer_test.go b/packet_packer_test.go index 009a5b48..d0a435f5 100644 --- a/packet_packer_test.go +++ b/packet_packer_test.go @@ -1127,6 +1127,18 @@ var _ = Describe("Packet packer", func() { Expect(packet.frames).To(HaveLen(1)) Expect(packet.frames[0].Frame).To(Equal(f)) }) + + It("returns nil if there's no probe data to send", func() { + sealingManager.EXPECT().Get1RTTSealer().Return(getSealer(), nil) + ackFramer.EXPECT().GetAckFrame(protocol.Encryption1RTT) + pnManager.EXPECT().PeekPacketNumber(protocol.Encryption1RTT).Return(protocol.PacketNumber(0x42), protocol.PacketNumberLen2) + expectAppendControlFrames() + expectAppendStreamFrames() + + packet, err := packer.MaybePackProbePacket(protocol.Encryption1RTT) + Expect(err).ToNot(HaveOccurred()) + Expect(packet).To(BeNil()) + }) }) }) }) diff --git a/session.go b/session.go index 3ff5d9b4..d3205602 100644 --- a/session.go +++ b/session.go @@ -1404,7 +1404,7 @@ func (s *session) sendProbePacket(encLevel protocol.EncryptionLevel) error { return err } } - if packet == nil { + if packet == nil || packet.packetContents == nil { return fmt.Errorf("session BUG: couldn't pack %s probe packet", encLevel) } s.sendPackedPacket(packet) From 7b1c4e7d80d4812a736c233a39e890f7e014ae4b Mon Sep 17 00:00:00 2001 From: Marten Seemann Date: Fri, 17 Apr 2020 07:58:41 +0700 Subject: [PATCH 2/2] don't dequeue retransmitted STREAM frames after the stream was canceled --- send_stream.go | 9 ++++----- send_stream_test.go | 16 ++++++++++++++++ 2 files changed, 20 insertions(+), 5 deletions(-) diff --git a/send_stream.go b/send_stream.go index df1b753b..c3191ec6 100644 --- a/send_stream.go +++ b/send_stream.go @@ -166,6 +166,10 @@ func (s *sendStream) popStreamFrame(maxBytes protocol.ByteCount) (*ackhandler.Fr } func (s *sendStream) popNewOrRetransmittedStreamFrame(maxBytes protocol.ByteCount) (*wire.StreamFrame, bool /* has more data to send */) { + if s.canceledWrite || s.closeForShutdownErr != nil { + return nil, false + } + if len(s.retransmissionQueue) > 0 { f, hasMoreRetransmissions := s.maybeGetRetransmission(maxBytes) if f != nil || hasMoreRetransmissions { @@ -195,10 +199,6 @@ func (s *sendStream) popNewOrRetransmittedStreamFrame(maxBytes protocol.ByteCoun } func (s *sendStream) popNewStreamFrame(f *wire.StreamFrame, maxBytes protocol.ByteCount) bool { - if s.canceledWrite || s.closeForShutdownErr != nil { - return false - } - maxDataLen := f.MaxDataLen(maxBytes, s.version) if maxDataLen == 0 { // a STREAM frame must have at least one byte of data return s.dataForWriting != nil @@ -329,7 +329,6 @@ func (s *sendStream) Close() error { func (s *sendStream) CancelWrite(errorCode protocol.ApplicationErrorCode) { s.cancelWriteImpl(errorCode, fmt.Errorf("Write on stream %d canceled with error code %d", s.streamID, errorCode)) - } // must be called after locking the mutex diff --git a/send_stream_test.go b/send_stream_test.go index 3bb4421e..57b92e9d 100644 --- a/send_stream_test.go +++ b/send_stream_test.go @@ -755,6 +755,22 @@ var _ = Describe("Send Stream", func() { Expect(newFrame).ToNot(BeNil()) Expect(newFrame.Frame.(*wire.StreamFrame).Data).To(Equal([]byte("foobar"))) }) + + It("doesn't get a retransmission after a stream was canceled", func() { + str.numOutstandingFrames = 1 + f := &wire.StreamFrame{ + Data: []byte("foobar"), + Offset: 0x42, + DataLenPresent: false, + } + mockSender.EXPECT().onHasStreamData(streamID) + str.queueRetransmission(f) + mockSender.EXPECT().queueControlFrame(gomock.Any()) + str.CancelWrite(0) + frame, hasMoreData := str.popStreamFrame(protocol.MaxByteCount) + Expect(hasMoreData).To(BeFalse()) + Expect(frame).To(BeNil()) + }) }) Context("determining when a stream is completed", func() {