Repository: quic-go/quic-go
Pull request #4986 (2025-03-15 05:01:36)

qlog: fix unset addresses in preferred_address transport parameter
For #4965.

Related qlog specification change: https://github.com/quicwg/qlog/pull/458

codecov[bot]: ## [Codecov](https://app.codecov.io/gh/quic-go/quic-go/pull/4986?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=quic-go) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Project coverage is 83.99%. Comparing base [(`9ca74c7`)](https://app.codecov.io/gh/quic-go/quic-go/commit/9ca74c70493a7268a8866495ab4dd8ec849f9111?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=quic-go) to head [(`230d053`)](https://app.codecov.io/gh/quic-go/quic-go/commit/230d053a6e3bc15b23d8280c74fc1a76d3d7eb2c?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=quic-go).

<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##           master    #4986      +/-   ##
==========================================
- Coverage   84.11%   83.99%   -0.12%     
==========================================
  Files         153      150       -3     
  Lines       16859    16518     -341     
==========================================
- Hits        14180    13873     -307     
+ Misses       2148     2129      -19     
+ Partials      531      516      -15     
```

</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/quic-go/quic-go/pull/4986?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=quic-go).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=quic-go).

<details><summary>🚀 New features to boost your workflow: </summary>

- ❄ [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
</details>

Patch:
diff --git a/connection.go b/connection.go
index 9c711d44..b266d7a4 100644
--- a/connection.go
+++ b/connection.go
@@ -1054,7 +1054,7 @@ func (s *connection) handleShortHeaderPacket(p receivedPacket) (wasProcessed boo
 	}
 
 	var shouldSwitchPath bool
-	if pn == s.largestRcvdAppData && !addrsEqual(p.remoteAddr, s.RemoteAddr()) {
+	if !addrsEqual(p.remoteAddr, s.RemoteAddr()) {
 		if s.pathManager == nil {
 			s.pathManager = newPathManager(
 				s.connIDManager.GetConnIDForPath,
@@ -1075,9 +1075,11 @@ func (s *connection) handleShortHeaderPacket(p receivedPacket) (wasProcessed boo
 			s.registerPackedShortHeaderPacket(probe, protocol.ECNNon, p.rcvTime)
 			s.sendQueue.SendProbe(buf, p.remoteAddr)
 		}
-	}
-
-	if shouldSwitchPath {
+		// We only switch paths in response to the highest-numbered non-probing packet,
+		// see section 9.3 of RFC 9000.
+		if !shouldSwitchPath || pn != s.largestRcvdAppData {
+			return true, nil
+		}
 		s.pathManager.SwitchToPath(p.remoteAddr)
 		s.sentPacketHandler.MigratedPath(p.rcvTime, protocol.ByteCount(s.config.InitialPacketSize))
 		maxPacketSize := protocol.ByteCount(protocol.MaxPacketBufferSize)
diff --git a/http3/headers.go b/http3/headers.go
index 05d13ff3..27af9500 100644
--- a/http3/headers.go
+++ b/http3/headers.go
@@ -55,24 +55,34 @@ func parseHeaders(headers []qpack.HeaderField, isRequest bool) (header, error) {
 				// all pseudo headers must appear before regular header fields, see section 4.3 of RFC 9114
 				return header{}, fmt.Errorf("received pseudo header %s after a regular header field", h.Name)
 			}
-			var isResponsePseudoHeader bool // pseudo headers are either valid for requests or for responses
+			var isResponsePseudoHeader bool  // pseudo headers are either valid for requests or for responses
+			var isDuplicatePseudoHeader bool // pseudo headers are allowed to appear exactly once
 			switch h.Name {
 			case ":path":
+				isDuplicatePseudoHeader = hdr.Path != ""
 				hdr.Path = h.Value
 			case ":method":
+				isDuplicatePseudoHeader = hdr.Method != ""
 				hdr.Method = h.Value
 			case ":authority":
+				isDuplicatePseudoHeader = hdr.Authority != ""
 				hdr.Authority = h.Value
 			case ":protocol":
+				isDuplicatePseudoHeader = hdr.Protocol != ""
 				hdr.Protocol = h.Value
 			case ":scheme":
+				isDuplicatePseudoHeader = hdr.Scheme != ""
 				hdr.Scheme = h.Value
 			case ":status":
+				isDuplicatePseudoHeader = hdr.Status != ""
 				hdr.Status = h.Value
 				isResponsePseudoHeader = true
 			default:
 				return header{}, fmt.Errorf("unknown pseudo header: %s", h.Name)
 			}
+			if isDuplicatePseudoHeader {
+				return header{}, fmt.Errorf("duplicate pseudo header: %s", h.Name)
+			}
 			if isRequest && isResponsePseudoHeader {
 				return header{}, fmt.Errorf("invalid request pseudo header: %s", h.Name)
 			}
diff --git a/qlog/event.go b/qlog/event.go
index e97b057b..9d20e215 100644
--- a/qlog/event.go
+++ b/qlog/event.go
@@ -492,10 +492,14 @@ var _ gojay.MarshalerJSONObject = &preferredAddress{}
 
 func (a preferredAddress) IsNil() bool { return false }
 func (a preferredAddress) MarshalJSONObject(enc *gojay.Encoder) {
-	enc.StringKey("ip_v4", a.IPv4.Addr().String())
-	enc.Uint16Key("port_v4", a.IPv4.Port())
-	enc.StringKey("ip_v6", a.IPv6.Addr().String())
-	enc.Uint16Key("port_v6", a.IPv6.Port())
+	if a.IPv4.IsValid() {
+		enc.StringKey("ip_v4", a.IPv4.Addr().String())
+		enc.Uint16Key("port_v4", a.IPv4.Port())
+	}
+	if a.IPv6.IsValid() {
+		enc.StringKey("ip_v6", a.IPv6.Addr().String())
+		enc.Uint16Key("port_v6", a.IPv6.Port())
+	}
 	enc.StringKey("connection_id", a.ConnectionID.String())
 	enc.StringKey("stateless_reset_token", fmt.Sprintf("%x", a.StatelessResetToken))
 }


Test patch:
diff --git a/http3/headers_test.go b/http3/headers_test.go
index a1cedc82..3f23cc3b 100644
--- a/http3/headers_test.go
+++ b/http3/headers_test.go
@@ -235,6 +235,18 @@ var _ = Describe("Request", func() {
 		Expect(err).To(MatchError(":protocol must be empty"))
 	})
 
+	It("errors with duplicate pseudo header in request", func() {
+		headers := []qpack.HeaderField{
+			{Name: ":path", Value: "/foo"},
+			{Name: ":authority", Value: "quic.clemente.io"},
+			{Name: ":method", Value: "GET"},
+			{Name: ":scheme", Value: "https"},
+			{Name: ":method", Value: "POST"},
+		}
+		_, err := requestFromHeaders(headers)
+		Expect(err).To(MatchError("duplicate pseudo header: :method"))
+	})
+
 	Context("regular HTTP CONNECT", func() {
 		It("handles CONNECT method", func() {
 			headers := []qpack.HeaderField{
@@ -432,4 +444,13 @@ var _ = Describe("Response", func() {
 		_, err := parseTrailers(headers)
 		Expect(err).To(MatchError("http3: received pseudo header in trailer: :status"))
 	})
+
+	It("errors with duplicate status header in response", func() {
+		headers := []qpack.HeaderField{
+			{Name: ":status", Value: "200"},
+			{Name: ":status", Value: "400"},
+		}
+		err := updateResponseFromHeaders(&http.Response{}, headers)
+		Expect(err).To(MatchError("duplicate pseudo header: :status"))
+	})
 })
diff --git a/qlog/connection_tracer_test.go b/qlog/connection_tracer_test.go
index 1ec0e63e..1705da9b 100644
--- a/qlog/connection_tracer_test.go
+++ b/qlog/connection_tracer_test.go
@@ -267,15 +267,32 @@ func TestTransportParametersWithoutRetrySourceConnectionID(t *testing.T) {
 }
 
 func TestTransportParametersWithPreferredAddress(t *testing.T) {
-	tracer, buf := newConnectionTracer()
-	tracer.SentTransportParameters(&logging.TransportParameters{
-		PreferredAddress: &logging.PreferredAddress{
-			IPv4:                netip.AddrPortFrom(netip.AddrFrom4([4]byte{12, 34, 56, 78}), 123),
-			IPv6:                netip.AddrPortFrom(netip.AddrFrom16([16]byte{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16}), 456),
-			ConnectionID:        protocol.ParseConnectionID([]byte{8, 7, 6, 5, 4, 3, 2, 1}),
-			StatelessResetToken: protocol.StatelessResetToken{15, 14, 13, 12, 11, 10, 9, 8, 7, 6, 5, 4, 3, 2, 1, 0},
-		},
+	t.Run("IPv4 and IPv6", func(t *testing.T) {
+		testTransportParametersWithPreferredAddress(t, true, true)
+	})
+	t.Run("IPv4 only", func(t *testing.T) {
+		testTransportParametersWithPreferredAddress(t, true, false)
 	})
+	t.Run("IPv6 only", func(t *testing.T) {
+		testTransportParametersWithPreferredAddress(t, false, true)
+	})
+}
+
+func testTransportParametersWithPreferredAddress(t *testing.T, hasIPv4, hasIPv6 bool) {
+	addr4 := netip.AddrPortFrom(netip.AddrFrom4([4]byte{12, 34, 56, 78}), 123)
+	addr6 := netip.AddrPortFrom(netip.AddrFrom16([16]byte{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16}), 456)
+	tracer, buf := newConnectionTracer()
+	preferredAddress := &logging.PreferredAddress{
+		ConnectionID:        protocol.ParseConnectionID([]byte{8, 7, 6, 5, 4, 3, 2, 1}),
+		StatelessResetToken: protocol.StatelessResetToken{15, 14, 13, 12, 11, 10, 9, 8, 7, 6, 5, 4, 3, 2, 1, 0},
+	}
+	if hasIPv4 {
+		preferredAddress.IPv4 = addr4
+	}
+	if hasIPv6 {
+		preferredAddress.IPv6 = addr6
+	}
+	tracer.SentTransportParameters(&logging.TransportParameters{PreferredAddress: preferredAddress})
 	tracer.Close()
 	entry := exportAndParseSingle(t, buf)
 	require.WithinDuration(t, time.Now(), entry.Time, scaleDuration(10*time.Millisecond))
@@ -284,10 +301,20 @@ func TestTransportParametersWithPreferredAddress(t *testing.T) {
 	require.Equal(t, "local", ev["owner"])
 	require.Contains(t, ev, "preferred_address")
 	pa := ev["preferred_address"].(map[string]interface{})
-	require.Equal(t, "12.34.56.78", pa["ip_v4"])
-	require.Equal(t, float64(123), pa["port_v4"])
-	require.Equal(t, "102:304:506:708:90a:b0c:d0e:f10", pa["ip_v6"])
-	require.Equal(t, float64(456), pa["port_v6"])
+	if hasIPv4 {
+		require.Equal(t, "12.34.56.78", pa["ip_v4"])
+		require.Equal(t, float64(123), pa["port_v4"])
+	} else {
+		require.NotContains(t, pa, "ip_v4")
+		require.NotContains(t, pa, "port_v4")
+	}
+	if hasIPv6 {
+		require.Equal(t, "102:304:506:708:90a:b0c:d0e:f10", pa["ip_v6"])
+		require.Equal(t, float64(456), pa["port_v6"])
+	} else {
+		require.NotContains(t, pa, "ip_v6")
+		require.NotContains(t, pa, "port_v6")
+	}
 	require.Equal(t, "0807060504030201", pa["connection_id"])
 	require.Equal(t, "0f0e0d0c0b0a09080706050403020100", pa["stateless_reset_token"])
 }