fix packing of maximum-size packets

framer.go

@@ -4,6 +4,7 @@ import (
 	"sync"
 
 	"github.com/lucas-clemente/quic-go/internal/protocol"
+	"github.com/lucas-clemente/quic-go/internal/utils"
 	"github.com/lucas-clemente/quic-go/internal/wire"
 )
 
@@ -75,11 +76,12 @@ func (f *framerI) AddActiveStream(id protocol.StreamID) {
 
 func (f *framerI) AppendStreamFrames(frames []wire.Frame, maxLen protocol.ByteCount) ([]wire.Frame, protocol.ByteCount) {
 	var length protocol.ByteCount
+	var frameAdded bool
 	f.mutex.Lock()
 	// pop STREAM frames, until less than MinStreamFrameSize bytes are left in the packet
 	numActiveStreams := len(f.streamQueue)
 	for i := 0; i < numActiveStreams; i++ {
-		if maxLen-length < protocol.MinStreamFrameSize {
+		if protocol.MinStreamFrameSize+length > maxLen {
 			break
 		}
 		id := f.streamQueue[0]
@@ -92,7 +94,12 @@ func (f *framerI) AppendStreamFrames(frames []wire.Frame, maxLen protocol.ByteCo
 			delete(f.activeStreams, id)
 			continue
 		}
-		frame, hasMoreData := str.popStreamFrame(maxLen - length)
+		remainingLen := maxLen - length
+		// For the last STREAM frame, we'll remove the DataLen field later.
+		// Therefore, we can pretend to have more bytes avaibalbe when popping
+		// the STREAM frame (which will always have the DataLen set).
+		remainingLen += utils.VarIntLen(uint64(remainingLen))
+		frame, hasMoreData := str.popStreamFrame(remainingLen)
 		if hasMoreData { // put the stream back in the queue (at the end)
 			f.streamQueue = append(f.streamQueue, id)
 		} else { // no more data to send. Stream is not active any more
@@ -103,7 +110,11 @@ func (f *framerI) AppendStreamFrames(frames []wire.Frame, maxLen protocol.ByteCo
 		}
 		frames = append(frames, frame)
 		length += frame.Length(f.version)
+		frameAdded = true
 	}
 	f.mutex.Unlock()
+	if frameAdded {
+		frames[len(frames)-1].(*wire.StreamFrame).DataLenPresent = false
+	}
 	return frames, length
 }

framer_test.go

@@ -7,11 +7,12 @@ import (
 
 	"github.com/lucas-clemente/quic-go/internal/protocol"
 	"github.com/lucas-clemente/quic-go/internal/wire"
+
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
 )
 
-var _ = Describe("Stream Framer", func() {
+var _ = Describe("Framer", func() {
 	const (
 		id1 = protocol.StreamID(10)
 		id2 = protocol.StreamID(11)
@@ -216,9 +217,63 @@ var _ = Describe("Stream Framer", func() {
 			Expect(length).To(BeZero())
 		})
 
-		It("pops frames that have the minimum size", func() {
+		It("pops maximum size STREAM frames", func() {
+			for i := protocol.MinStreamFrameSize; i < 2000; i++ {
+				streamGetter.EXPECT().GetOrOpenSendStream(id1).Return(stream1, nil)
+				stream1.EXPECT().popStreamFrame(gomock.Any()).DoAndReturn(func(size protocol.ByteCount) (*wire.StreamFrame, bool) {
+					f := &wire.StreamFrame{
+						StreamID:       id1,
+						DataLenPresent: true,
+					}
+					f.Data = make([]byte, f.MaxDataLen(size, version))
+					Expect(f.Length(version)).To(Equal(size))
+					return f, false
+				})
+				framer.AddActiveStream(id1)
+				frames, _ := framer.AppendStreamFrames(nil, i)
+				Expect(frames).To(HaveLen(1))
+				f := frames[0].(*wire.StreamFrame)
+				Expect(f.DataLenPresent).To(BeFalse())
+				Expect(f.Length(version)).To(Equal(i))
+			}
+		})
+
+		It("pops multiple STREAM frames", func() {
+			for i := 2 * protocol.MinStreamFrameSize; i < 2000; i++ {
+				streamGetter.EXPECT().GetOrOpenSendStream(id1).Return(stream1, nil)
+				streamGetter.EXPECT().GetOrOpenSendStream(id2).Return(stream2, nil)
+				stream1.EXPECT().popStreamFrame(gomock.Any()).DoAndReturn(func(size protocol.ByteCount) (*wire.StreamFrame, bool) {
+					f := &wire.StreamFrame{
+						StreamID:       id2,
+						DataLenPresent: true,
+					}
+					f.Data = make([]byte, f.MaxDataLen(protocol.MinStreamFrameSize, version))
+					return f, false
+				})
+				stream2.EXPECT().popStreamFrame(gomock.Any()).DoAndReturn(func(size protocol.ByteCount) (*wire.StreamFrame, bool) {
+					f := &wire.StreamFrame{
+						StreamID:       id2,
+						DataLenPresent: true,
+					}
+					f.Data = make([]byte, f.MaxDataLen(size, version))
+					Expect(f.Length(version)).To(Equal(size))
+					return f, false
+				})
+				framer.AddActiveStream(id1)
+				framer.AddActiveStream(id2)
+				frames, _ := framer.AppendStreamFrames(nil, i)
+				Expect(frames).To(HaveLen(2))
+				f1 := frames[0].(*wire.StreamFrame)
+				f2 := frames[1].(*wire.StreamFrame)
+				Expect(f1.DataLenPresent).To(BeTrue())
+				Expect(f2.DataLenPresent).To(BeFalse())
+				Expect(f1.Length(version) + f2.Length(version)).To(Equal(i))
+			}
+		})
+
+		It("pops frames that when asked for the the minimum STREAM frame size", func() {
 			streamGetter.EXPECT().GetOrOpenSendStream(id1).Return(stream1, nil)
-			stream1.EXPECT().popStreamFrame(protocol.MinStreamFrameSize).Return(&wire.StreamFrame{Data: []byte("foobar")}, false)
+			stream1.EXPECT().popStreamFrame(gomock.Any()).Return(&wire.StreamFrame{Data: []byte("foobar")}, false)
 			framer.AddActiveStream(id1)
 			framer.AppendStreamFrames(nil, protocol.MinStreamFrameSize)
 		})
@@ -235,7 +290,7 @@ var _ = Describe("Stream Framer", func() {
 				StreamID: id1,
 				Data:     bytes.Repeat([]byte("f"), int(500-protocol.MinStreamFrameSize)),
 			}
-			stream1.EXPECT().popStreamFrame(protocol.ByteCount(500)).Return(f, false)
+			stream1.EXPECT().popStreamFrame(gomock.Any()).Return(f, false)
 			framer.AddActiveStream(id1)
 			fs, length := framer.AppendStreamFrames(nil, 500)
 			Expect(fs).To(Equal([]wire.Frame{f}))

packet_packer.go

@@ -353,18 +353,8 @@ func (p *packetPacker) composeNextPacket(maxFrameSize protocol.ByteCount) (paylo
 	frames, lengthAdded := p.framer.AppendControlFrames(payload.frames, maxFrameSize-payload.length)
 	payload.length += lengthAdded
 
-	// temporarily increase the maxFrameSize by the (minimum) length of the DataLen field
-	// this leads to a properly sized packet in all cases, since we do all the packet length calculations with STREAM frames that have the DataLen set
-	// however, for the last STREAM frame in the packet, we can omit the DataLen, thus yielding a packet of exactly the correct size
-	// the length is encoded to either 1 or 2 bytes
-	maxFrameSize++
-
 	frames, lengthAdded = p.framer.AppendStreamFrames(frames, maxFrameSize-payload.length)
 	if len(frames) > 0 {
-		lastFrame := frames[len(frames)-1]
-		if sf, ok := lastFrame.(*wire.StreamFrame); ok {
-			sf.DataLenPresent = false
-		}
 		payload.frames = append(payload.frames, frames...)
 		payload.length += lengthAdded
 	}

packet_packer_test.go

@@ -316,7 +316,7 @@ var _ = Describe("Packet packer", func() {
 						return fs, 444
 					}),
 					framer.EXPECT().AppendStreamFrames(gomock.Any(), gomock.Any()).Do(func(fs []wire.Frame, maxLen protocol.ByteCount) ([]wire.Frame, protocol.ByteCount) {
-						Expect(maxLen).To(Equal(maxSize - 444 + 1 /* data length of the STREAM frame */))
+						Expect(maxLen).To(Equal(maxSize - 444))
 						return fs, 0
 					}),
 				)
@@ -435,9 +435,8 @@ var _ = Describe("Packet packer", func() {
 					sealingManager.EXPECT().GetSealer().Return(protocol.Encryption1RTT, sealer)
 					expectAppendControlFrames()
 					sf := &wire.StreamFrame{
-						Offset:         1,
+						Offset:   1,
-						StreamID:       5,
+						StreamID: 5,
-						DataLenPresent: true,
 					}
 					framer.EXPECT().AppendStreamFrames(gomock.Any(), gomock.Any()).DoAndReturn(func(_ []wire.Frame, maxSize protocol.ByteCount) ([]wire.Frame, protocol.ByteCount) {
 						sf.Data = bytes.Repeat([]byte{'f'}, int(maxSize-sf.Length(packer.version)))
@@ -478,11 +477,8 @@ var _ = Describe("Packet packer", func() {
 					Expect(err).ToNot(HaveOccurred())
 					Expect(p.frames).To(HaveLen(3))
 					Expect(p.frames[0].(*wire.StreamFrame).Data).To(Equal([]byte("frame 1")))
-					Expect(p.frames[0].(*wire.StreamFrame).DataLenPresent).To(BeTrue())
 					Expect(p.frames[1].(*wire.StreamFrame).Data).To(Equal([]byte("frame 2")))
-					Expect(p.frames[1].(*wire.StreamFrame).DataLenPresent).To(BeTrue())
 					Expect(p.frames[2].(*wire.StreamFrame).Data).To(Equal([]byte("frame 3")))
-					Expect(p.frames[2].(*wire.StreamFrame).DataLenPresent).To(BeFalse())
 				})
 			})