quic: move ack_delay_exponent handling out of frame parsing

The ACK Delay field of ACK frames contains a duration.
The field contains an integer which is multiplied by two to the
power of the sender's ack_delay_exponent transport parameter to
arrive at the delay in microseconds.

Change the frame parsing and encoding layer to operate on the
unscaled field value, rather than passing the ack_delay_exponent
and a duration. This better expresses the fact that we may
parse an ACK frame without knowing the ack_delay_exponent, if
the ACK is received before transport parameters.

For golang/go#58547

Change-Id: Ic26256761961ce89aea0618b849e5661b0502b12
Reviewed-on: https://go-review.googlesource.com/c/net/+/504855
TryBot-Result: Gopher Robot <[email protected]>
Run-TryBot: Damien Neil <[email protected]>
Reviewed-by: Jonathan Amsterdam <[email protected]>

Author: neild

internal/quic/ack_delay.go

@@ -0,0 +1,28 @@
+// Copyright 2023 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+//go:build go1.21
+
+package quic
+
+import (
+	"math"
+	"time"
+)
+
+// An unscaledAckDelay is an ACK Delay field value from an ACK packet,
+// without the ack_delay_exponent scaling applied.
+type unscaledAckDelay int64
+
+func unscaledAckDelayFromDuration(d time.Duration, ackDelayExponent uint8) unscaledAckDelay {
+	return unscaledAckDelay(d.Microseconds() >> ackDelayExponent)
+}
+
+func (d unscaledAckDelay) Duration(ackDelayExponent uint8) time.Duration {
+	if int64(d) > (math.MaxInt64>>ackDelayExponent)/int64(time.Microsecond) {
+		// If scaling the delay would overflow, ignore the delay.
+		return 0
+	}
+	return time.Duration(d<<ackDelayExponent) * time.Microsecond
+}

internal/quic/ack_delay_test.go

@@ -0,0 +1,81 @@
+// Copyright 2023 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+//go:build go1.21
+
+package quic
+
+import (
+	"math"
+	"testing"
+	"time"
+)
+
+func TestAckDelayFromDuration(t *testing.T) {
+	for _, test := range []struct {
+		d                time.Duration
+		ackDelayExponent uint8
+		want             unscaledAckDelay
+	}{{
+		d:                8 * time.Microsecond,
+		ackDelayExponent: 3,
+		want:             1,
+	}, {
+		d:                1 * time.Nanosecond,
+		ackDelayExponent: 3,
+		want:             0, // rounds to zero
+	}, {
+		d:                3 * (1 << 20) * time.Microsecond,
+		ackDelayExponent: 20,
+		want:             3,
+	}} {
+		got := unscaledAckDelayFromDuration(test.d, test.ackDelayExponent)
+		if got != test.want {
+			t.Errorf("unscaledAckDelayFromDuration(%v, %v) = %v, want %v",
+				test.d, test.ackDelayExponent, got, test.want)
+		}
+	}
+}
+
+func TestAckDelayToDuration(t *testing.T) {
+	for _, test := range []struct {
+		d                unscaledAckDelay
+		ackDelayExponent uint8
+		want             time.Duration
+	}{{
+		d:                1,
+		ackDelayExponent: 3,
+		want:             8 * time.Microsecond,
+	}, {
+		d:                0,
+		ackDelayExponent: 3,
+		want:             0,
+	}, {
+		d:                3,
+		ackDelayExponent: 20,
+		want:             3 * (1 << 20) * time.Microsecond,
+	}, {
+		d:                math.MaxInt64 / 1000,
+		ackDelayExponent: 0,
+		want:             (math.MaxInt64 / 1000) * time.Microsecond,
+	}, {
+		d:                (math.MaxInt64 / 1000) + 1,
+		ackDelayExponent: 0,
+		want:             0, // return 0 on overflow
+	}, {
+		d:                math.MaxInt64 / 1000 / 8,
+		ackDelayExponent: 3,
+		want:             (math.MaxInt64 / 1000 / 8) * 8 * time.Microsecond,
+	}, {
+		d:                (math.MaxInt64 / 1000 / 8) + 1,
+		ackDelayExponent: 3,
+		want:             0, // return 0 on overflow
+	}} {
+		got := test.d.Duration(test.ackDelayExponent)
+		if got != test.want {
+			t.Errorf("unscaledAckDelay(%v).Duration(%v) = %v, want %v",
+				test.d, test.ackDelayExponent, int64(got), int64(test.want))
+		}
+	}
+}

internal/quic/frame_debug.go

@@ -8,7 +8,6 @@ package quic
 
 import (
 	"fmt"
-	"time"
 )
 
 // A debugFrame is a representation of the contents of a QUIC frame,
@@ -115,13 +114,13 @@ func (f debugFramePing) write(w *packetWriter) bool {
 
 // debugFrameAck is an ACK frame.
 type debugFrameAck struct {
-	ackDelay time.Duration
+	ackDelay unscaledAckDelay
 	ranges   []i64range[packetNumber]
 }
 
 func parseDebugFrameAck(b []byte) (f debugFrameAck, n int) {
 	f.ranges = nil
-	_, f.ackDelay, n = consumeAckFrame(b, ackDelayExponent, func(start, end packetNumber) {
+	_, f.ackDelay, n = consumeAckFrame(b, func(start, end packetNumber) {
 		f.ranges = append(f.ranges, i64range[packetNumber]{
 			start: start,
 			end:   end,
@@ -144,7 +143,7 @@ func (f debugFrameAck) String() string {
 }
 
 func (f debugFrameAck) write(w *packetWriter) bool {
-	return w.appendAckFrame(rangeset[packetNumber](f.ranges), ackDelayExponent, f.ackDelay)
+	return w.appendAckFrame(rangeset[packetNumber](f.ranges), f.ackDelay)
 }
 
 // debugFrameResetStream is a RESET_STREAM frame.

internal/quic/packet_codec_test.go

@@ -11,7 +11,6 @@ import (
 	"crypto/tls"
 	"reflect"
 	"testing"
-	"time"
 )
 
 func TestParseLongHeaderPacket(t *testing.T) {
@@ -219,9 +218,9 @@ func TestFrameEncodeDecode(t *testing.T) {
 			0x01, // TYPE(i) = 0x01
 		},
 	}, {
-		s: "ACK Delay=80µs [0,16) [17,32) [48,64)",
+		s: "ACK Delay=10 [0,16) [17,32) [48,64)",
 		f: debugFrameAck{
-			ackDelay: (10 << ackDelayExponent) * time.Microsecond,
+			ackDelay: 10,
 			ranges: []i64range[packetNumber]{
 				{0x00, 0x10},
 				{0x11, 0x20},
@@ -594,7 +593,7 @@ func TestFrameDecode(t *testing.T) {
 	}, {
 		desc: "ACK frame with ECN counts",
 		want: debugFrameAck{
-			ackDelay: (10 << ackDelayExponent) * time.Microsecond,
+			ackDelay: 10,
 			ranges: []i64range[packetNumber]{
 				{0, 1},
 			},

internal/quic/packet_parser.go

@@ -6,10 +6,6 @@
 
 package quic
 
-import (
-	"time"
-)
-
 // parseLongHeaderPacket parses a QUIC long header packet.
 //
 // It does not parse Version Negotiation packets.
@@ -166,7 +162,7 @@ func parse1RTTPacket(pkt []byte, k keys, pnumMax packetNumber) (p shortPacket, n
 // which includes both general parse failures and specific violations of frame
 // constraints.
 
-func consumeAckFrame(frame []byte, ackDelayExponent uint8, f func(start, end packetNumber)) (largest packetNumber, ackDelay time.Duration, n int) {
+func consumeAckFrame(frame []byte, f func(start, end packetNumber)) (largest packetNumber, ackDelay unscaledAckDelay, n int) {
 	b := frame[1:] // type
 
 	largestAck, n := consumeVarint(b)
@@ -175,12 +171,12 @@ func consumeAckFrame(frame []byte, ackDelayExponent uint8, f func(start, end pac
 	}
 	b = b[n:]
 
-	ackDelayScaled, n := consumeVarint(b)
+	v, n := consumeVarintInt64(b)
 	if n < 0 {
 		return 0, 0, -1
 	}
 	b = b[n:]
-	ackDelay = time.Duration(ackDelayScaled*(1<<ackDelayExponent)) * time.Microsecond
+	ackDelay = unscaledAckDelay(v)
 
 	ackRangeCount, n := consumeVarint(b)
 	if n < 0 {

internal/quic/packet_writer.go

@@ -8,7 +8,6 @@ package quic
 
 import (
 	"encoding/binary"
-	"time"
 )
 
 // A packetWriter constructs QUIC datagrams.
@@ -257,21 +256,20 @@ func (w *packetWriter) appendPingFrame() (added bool) {
 // to the peer potentially failing to receive an acknowledgement
 // for an older packet during a period of high packet loss or
 // reordering. This may result in unnecessary retransmissions.
-func (w *packetWriter) appendAckFrame(seen rangeset[packetNumber], ackDelayExponent uint8, delay time.Duration) (added bool) {
+func (w *packetWriter) appendAckFrame(seen rangeset[packetNumber], delay unscaledAckDelay) (added bool) {
 	if len(seen) == 0 {
 		return false
 	}
 	var (
 		largest    = uint64(seen.max())
-		mdelay     = uint64(delay.Microseconds() / (1 << ackDelayExponent))
 		firstRange = uint64(seen[len(seen)-1].size() - 1)
 	)
-	if w.avail() < 1+sizeVarint(largest)+sizeVarint(mdelay)+1+sizeVarint(firstRange) {
+	if w.avail() < 1+sizeVarint(largest)+sizeVarint(uint64(delay))+1+sizeVarint(firstRange) {
 		return false
 	}
 	w.b = append(w.b, frameTypeAck)
 	w.b = appendVarint(w.b, largest)
-	w.b = appendVarint(w.b, mdelay)
+	w.b = appendVarint(w.b, uint64(delay))
 	// The range count is technically a varint, but we'll reserve a single byte for it
 	// and never add more than 62 ranges (the maximum varint that fits in a byte).
 	rangeCountOff := len(w.b)