Repository: quic-go/quic-go
Pull request #3849 (2023-05-22 14:06:59)

packet packer: don't try packing a 0-RTT packet with only an ACK
0-RTT packets can't contain ACK frames. This was correctly reflected in the code path for packing a 0-RTT packet, but we would ignore the ACK-only flag when packing the coalesced packet, leading to a full-size 0-RTT packet being sent out when we're only allowed to send an ACK-only packet.

codecov[bot]: ## [Codecov](https://app.codecov.io/gh/quic-go/quic-go/pull/3849?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) Report
> Merging [#3849](https://app.codecov.io/gh/quic-go/quic-go/pull/3849?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) (79a1808) into [master](https://app.codecov.io/gh/quic-go/quic-go/commit/cb3453db2508eedf4ea3ce49f86bbe029de5c1c9?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) (cb3453d) will **decrease** coverage by `0.01%`.
> The diff coverage is `100.00%`.

```diff
@@            Coverage Diff             @@
##           master    #3849      +/-   ##
==========================================
- Coverage   83.94%   83.93%   -0.01%     
==========================================
  Files         146      146              
  Lines       14307    14307              
==========================================
- Hits        12009    12008       -1     
- Misses       1858     1859       +1     
  Partials      440      440              
```


| [Impacted Files](https://app.codecov.io/gh/quic-go/quic-go/pull/3849?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) | Coverage Δ | |
|---|---|---|
| [packet\_packer.go](https://app.codecov.io/gh/quic-go/quic-go/pull/3849?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-cGFja2V0X3BhY2tlci5nbw==) | `84.67% <100.00%> (ø)` | |

... and [1 file with indirect coverage changes](https://app.codecov.io/gh/quic-go/quic-go/pull/3849/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None)



Patch:
diff --git a/internal/utils/rtt_stats.go b/internal/utils/rtt_stats.go
index 527539e1..2cd9a191 100644
--- a/internal/utils/rtt_stats.go
+++ b/internal/utils/rtt_stats.go
@@ -103,8 +103,12 @@ func (r *RTTStats) SetMaxAckDelay(mad time.Duration) {
 // SetInitialRTT sets the initial RTT.
 // It is used during the 0-RTT handshake when restoring the RTT stats from the session state.
 func (r *RTTStats) SetInitialRTT(t time.Duration) {
+	// On the server side, by the time we get to process the session ticket,
+	// we might already have obtained an RTT measurement.
+	// This can happen if we received the ClientHello in multiple pieces, and one of those pieces was lost.
+	// Discard the restored value. A fresh measurement is always better.
 	if r.hasMeasurement {
-		panic("initial RTT set after first measurement")
+		return
 	}
 	r.smoothedRTT = t
 	r.latestRTT = t
diff --git a/internal/wire/ack_frame.go b/internal/wire/ack_frame.go
index f145c8b4..2f816c78 100644
--- a/internal/wire/ack_frame.go
+++ b/internal/wire/ack_frame.go
@@ -90,13 +90,22 @@ func parseAckFrame(r *bytes.Reader, typ uint64, ackDelayExponent uint8, _ protoc
 		return nil, errInvalidAckRanges
 	}
 
-	// parse (and skip) the ECN section
 	if ecn {
-		for i := 0; i < 3; i++ {
-			if _, err := quicvarint.Read(r); err != nil {
-				return nil, err
-			}
+		ect0, err := quicvarint.Read(r)
+		if err != nil {
+			return nil, err
+		}
+		frame.ECT0 = ect0
+		ect1, err := quicvarint.Read(r)
+		if err != nil {
+			return nil, err
+		}
+		frame.ECT1 = ect1
+		ecnce, err := quicvarint.Read(r)
+		if err != nil {
+			return nil, err
 		}
+		frame.ECNCE = ecnce
 	}
 
 	return frame, nil
diff --git a/packet_packer.go b/packet_packer.go
index 14befd46..180a03e1 100644
--- a/packet_packer.go
+++ b/packet_packer.go
@@ -417,7 +417,7 @@ func (p *packetPacker) PackCoalescedPacket(onlyAck bool, v protocol.VersionNumbe
 			if oneRTTPayload.length > 0 {
 				size += p.shortHeaderPacketLength(connID, oneRTTPacketNumberLen, oneRTTPayload) + protocol.ByteCount(oneRTTSealer.Overhead())
 			}
-		} else if p.perspective == protocol.PerspectiveClient { // 0-RTT
+		} else if p.perspective == protocol.PerspectiveClient && !onlyAck { // 0-RTT packets can't contain ACK frames
 			var err error
 			zeroRTTSealer, err = p.cryptoSetup.Get0RTTSealer()
 			if err != nil && err != handshake.ErrKeysDropped && err != handshake.ErrKeysNotYetAvailable {


Test patch:
diff --git a/integrationtests/tools/proxy/proxy.go b/integrationtests/tools/proxy/proxy.go
index 01e6a4a9..4ed2d89d 100644
--- a/integrationtests/tools/proxy/proxy.go
+++ b/integrationtests/tools/proxy/proxy.go
@@ -169,6 +169,12 @@ func NewQuicProxy(local string, opts *Opts) (*QuicProxy, error) {
 	if err != nil {
 		return nil, err
 	}
+	if err := conn.SetReadBuffer(protocol.DesiredReceiveBufferSize); err != nil {
+		return nil, err
+	}
+	if err := conn.SetWriteBuffer(protocol.DesiredSendBufferSize); err != nil {
+		return nil, err
+	}
 	raddr, err := net.ResolveUDPAddr("udp", opts.RemoteAddr)
 	if err != nil {
 		return nil, err
@@ -225,13 +231,19 @@ func (p *QuicProxy) LocalPort() int {
 }
 
 func (p *QuicProxy) newConnection(cliAddr *net.UDPAddr) (*connection, error) {
-	srvudp, err := net.DialUDP("udp", nil, p.serverAddr)
+	conn, err := net.DialUDP("udp", nil, p.serverAddr)
 	if err != nil {
 		return nil, err
 	}
+	if err := conn.SetReadBuffer(protocol.DesiredReceiveBufferSize); err != nil {
+		return nil, err
+	}
+	if err := conn.SetWriteBuffer(protocol.DesiredSendBufferSize); err != nil {
+		return nil, err
+	}
 	return &connection{
 		ClientAddr:      cliAddr,
-		ServerConn:      srvudp,
+		ServerConn:      conn,
 		incomingPackets: make(chan packetEntry, 10),
 		Incoming:        newQueue(),
 		Outgoing:        newQueue(),
diff --git a/internal/utils/rtt_stats_test.go b/internal/utils/rtt_stats_test.go
index 29892c6f..eae31926 100644
--- a/internal/utils/rtt_stats_test.go
+++ b/internal/utils/rtt_stats_test.go
@@ -154,4 +154,14 @@ var _ = Describe("RTT stats", func() {
 		Expect(rttStats.SmoothedRTT()).To(Equal(200 * time.Millisecond))
 		Expect(rttStats.MeanDeviation()).To(Equal(100 * time.Millisecond))
 	})
+
+	It("doesn't restore the RTT if we already have a measurement", func() {
+		const rtt = 10 * time.Millisecond
+		rttStats.UpdateRTT(rtt, 0, time.Now())
+		Expect(rttStats.LatestRTT()).To(Equal(rtt))
+		Expect(rttStats.SmoothedRTT()).To(Equal(rtt))
+		rttStats.SetInitialRTT(time.Minute)
+		Expect(rttStats.LatestRTT()).To(Equal(rtt))
+		Expect(rttStats.SmoothedRTT()).To(Equal(rtt))
+	})
 })
diff --git a/internal/wire/ack_frame_test.go b/internal/wire/ack_frame_test.go
index 49d11374..f80335c0 100644
--- a/internal/wire/ack_frame_test.go
+++ b/internal/wire/ack_frame_test.go
@@ -170,6 +170,9 @@ var _ = Describe("ACK Frame (for IETF QUIC)", func() {
 				Expect(frame.LargestAcked()).To(Equal(protocol.PacketNumber(100)))
 				Expect(frame.LowestAcked()).To(Equal(protocol.PacketNumber(90)))
 				Expect(frame.HasMissingRanges()).To(BeFalse())
+				Expect(frame.ECT0).To(BeEquivalentTo(0x42))
+				Expect(frame.ECT1).To(BeEquivalentTo(0x12345))
+				Expect(frame.ECNCE).To(BeEquivalentTo(0x12345678))
 				Expect(b.Len()).To(BeZero())
 			})
 
diff --git a/packet_packer_test.go b/packet_packer_test.go
index 31caa17a..05dc8689 100644
--- a/packet_packer_test.go
+++ b/packet_packer_test.go
@@ -316,6 +316,12 @@ var _ = Describe("Packet packer", func() {
 				Expect(p.longHdrPackets[0].EncryptionLevel()).To(Equal(protocol.Encryption0RTT))
 				Expect(p.longHdrPackets[0].frames).To(Equal([]*ackhandler.Frame{cf}))
 			})
+
+			It("doesn't add an ACK-only 0-RTT packet", func() { // ACK frames cannot be sent in 0-RTT packets
+				p, err := packer.PackCoalescedPacket(true, protocol.Version1)
+				Expect(err).ToNot(HaveOccurred())
+				Expect(p).To(BeNil())
+			})
 		})
 
 		Context("packing CONNECTION_CLOSE", func() {