From c0286b0c2e28837f45b495ea481599b750514eab Mon Sep 17 00:00:00 2001 From: Marten Seemann Date: Sat, 10 Aug 2019 13:50:09 +0700 Subject: [PATCH 1/3] fix calculation of the length of appended STREAM frames For the last STREAM frame we omit the Length field. When packing STREAM frames, we need to account for this byte saving when calculating the length of the payload. --- framer.go | 6 +++++- framer_test.go | 29 ++++++++++++++++++----------- 2 files changed, 23 insertions(+), 12 deletions(-) diff --git a/framer.go b/framer.go index 2f4d131b7..84cf1a8ea 100644 --- a/framer.go +++ b/framer.go @@ -114,7 +114,11 @@ func (f *framerI) AppendStreamFrames(frames []wire.Frame, maxLen protocol.ByteCo } f.mutex.Unlock() if frameAdded { - frames[len(frames)-1].(*wire.StreamFrame).DataLenPresent = false + lastFrame := frames[len(frames)-1].(*wire.StreamFrame) + lastFrameLen := lastFrame.Length(f.version) + // acount for the smaller size of the last STREAM frame + lastFrame.DataLenPresent = false + length += lastFrame.Length(f.version) - lastFrameLen } return frames, length } diff --git a/framer_test.go b/framer_test.go index 536bc64b3..37f68342f 100644 --- a/framer_test.go +++ b/framer_test.go @@ -80,22 +80,25 @@ var _ = Describe("Framer", func() { It("returns STREAM frames", func() { streamGetter.EXPECT().GetOrOpenSendStream(id1).Return(stream1, nil) f := &wire.StreamFrame{ - StreamID: id1, - Data: []byte("foobar"), - Offset: 42, + StreamID: id1, + Data: []byte("foobar"), + Offset: 42, + DataLenPresent: true, } stream1.EXPECT().popStreamFrame(gomock.Any()).Return(f, false) framer.AddActiveStream(id1) fs, length := framer.AppendStreamFrames(nil, 1000) Expect(fs).To(Equal([]wire.Frame{f})) + Expect(fs[0].(*wire.StreamFrame).DataLenPresent).To(BeFalse()) Expect(length).To(Equal(f.Length(version))) }) It("appends to a frame slice", func() { streamGetter.EXPECT().GetOrOpenSendStream(id1).Return(stream1, nil) f := &wire.StreamFrame{ - StreamID: id1, - Data: []byte("foobar"), + StreamID: id1, + Data: []byte("foobar"), + DataLenPresent: true, } stream1.EXPECT().popStreamFrame(gomock.Any()).Return(f, false) framer.AddActiveStream(id1) @@ -103,6 +106,7 @@ var _ = Describe("Framer", func() { frames := []wire.Frame{mdf} fs, length := framer.AppendStreamFrames(frames, 1000) Expect(fs).To(Equal([]wire.Frame{mdf, f})) + Expect(fs[1].(*wire.StreamFrame).DataLenPresent).To(BeFalse()) Expect(length).To(Equal(f.Length(version))) }) @@ -110,8 +114,9 @@ var _ = Describe("Framer", func() { streamGetter.EXPECT().GetOrOpenSendStream(id1).Return(nil, nil) streamGetter.EXPECT().GetOrOpenSendStream(id2).Return(stream2, nil) f := &wire.StreamFrame{ - StreamID: id2, - Data: []byte("foobar"), + StreamID: id2, + Data: []byte("foobar"), + DataLenPresent: true, } stream2.EXPECT().popStreamFrame(gomock.Any()).Return(f, false) framer.AddActiveStream(id1) @@ -124,8 +129,9 @@ var _ = Describe("Framer", func() { streamGetter.EXPECT().GetOrOpenSendStream(id1).Return(stream1, nil) streamGetter.EXPECT().GetOrOpenSendStream(id2).Return(stream2, nil) f := &wire.StreamFrame{ - StreamID: id2, - Data: []byte("foobar"), + StreamID: id2, + Data: []byte("foobar"), + DataLenPresent: true, } stream1.EXPECT().popStreamFrame(gomock.Any()).Return(nil, false) stream2.EXPECT().popStreamFrame(gomock.Any()).Return(f, false) @@ -287,8 +293,9 @@ var _ = Describe("Framer", func() { streamGetter.EXPECT().GetOrOpenSendStream(id1).Return(stream1, nil) // pop a frame such that the remaining size is one byte less than the minimum STREAM frame size f := &wire.StreamFrame{ - StreamID: id1, - Data: bytes.Repeat([]byte("f"), int(500-protocol.MinStreamFrameSize)), + StreamID: id1, + Data: bytes.Repeat([]byte("f"), int(500-protocol.MinStreamFrameSize)), + DataLenPresent: true, } stream1.EXPECT().popStreamFrame(gomock.Any()).Return(f, false) framer.AddActiveStream(id1) From 1f950da7518475dbc4aaa7eb89d5e032f2a0ec26 Mon Sep 17 00:00:00 2001 From: Marten Seemann Date: Sat, 10 Aug 2019 14:57:13 +0700 Subject: [PATCH 2/3] fix calculation of the length of retransmitted STREAM frames --- packet_packer.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packet_packer.go b/packet_packer.go index e86d6b8a3..9c3a80da6 100644 --- a/packet_packer.go +++ b/packet_packer.go @@ -295,7 +295,9 @@ func (p *packetPacker) PackRetransmission(packet *ackhandler.Packet) ([]*packedP frames = append(frames, frameToAdd) } if sf, ok := frames[len(frames)-1].(*wire.StreamFrame); ok { + sfLen := sf.Length(p.version) sf.DataLenPresent = false + length += sf.Length(p.version) - sfLen } p, err := p.writeAndSealPacket(hdr, payload{frames: frames, length: length}, packet.EncryptionLevel, sealer) if err != nil { From fc77dee22d269ffdb30951fb4becf0fb83254e74 Mon Sep 17 00:00:00 2001 From: Marten Seemann Date: Sat, 10 Aug 2019 14:57:39 +0700 Subject: [PATCH 3/3] add a bug check for consistent payload length in the packet packer --- packet_packer.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/packet_packer.go b/packet_packer.go index 9c3a80da6..ed9589c87 100644 --- a/packet_packer.go +++ b/packet_packer.go @@ -546,6 +546,9 @@ func (p *packetPacker) writeAndSealPacketWithPadding( } } + if payloadSize := protocol.ByteCount(buffer.Len()-payloadOffset) - paddingLen; payloadSize != payload.length { + return nil, fmt.Errorf("PacketPacker BUG: payload size inconsistent (expected %d, got %d bytes)", payload.length, payloadSize) + } if size := protocol.ByteCount(buffer.Len() + sealer.Overhead()); size > p.maxPacketSize { return nil, fmt.Errorf("PacketPacker BUG: packet too large (%d bytes, allowed %d bytes)", size, p.maxPacketSize) }