From 4bb7204fa91cc86daca35f816265e2258bd95a7f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Jeli=C5=84ski?= Date: Fri, 3 Apr 2026 05:48:24 +0000 Subject: [PATCH] 8377181: HttpClient may leak closed QUIC connection objects Reviewed-by: dfuchs --- .../http/quic/ConnectionTerminatorImpl.java | 8 +++++- .../net/http/quic/PeerConnIdManager.java | 25 ++++++++++++++++--- .../net/http/quic/QuicConnectionImpl.java | 4 +++ .../internal/net/http/quic/QuicEndpoint.java | 12 +++++---- .../net/httpclient/CancelRequestTest.java | 2 +- .../httpclient/http3/H3ErrorHandlingTest.java | 2 +- 6 files changed, 41 insertions(+), 12 deletions(-) diff --git a/src/java.net.http/share/classes/jdk/internal/net/http/quic/ConnectionTerminatorImpl.java b/src/java.net.http/share/classes/jdk/internal/net/http/quic/ConnectionTerminatorImpl.java index 5e2384dce27..3fc013b4fde 100644 --- a/src/java.net.http/share/classes/jdk/internal/net/http/quic/ConnectionTerminatorImpl.java +++ b/src/java.net.http/share/classes/jdk/internal/net/http/quic/ConnectionTerminatorImpl.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2024, 2025, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2024, 2026, Oracle and/or its affiliates. All rights reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it @@ -203,6 +203,9 @@ final class ConnectionTerminatorImpl implements ConnectionTerminator { // an endpoint has been established (which is OK) return; } + // close the connection ID managers; any in-flight connection ID changes should be ignored. + connection.localConnectionIdManager().close(); + connection.peerConnectionIdManager().close(); endpoint.removeConnection(this.connection); } @@ -434,6 +437,9 @@ final class ConnectionTerminatorImpl implements ConnectionTerminator { final QuicPacket packet = connection.newQuicPacket(keySpace, List.of(toSend)); final ProtectionRecord protectionRecord = ProtectionRecord.single(packet, connection::allocateDatagramForEncryption); + // close the connection ID managers; any in-flight connection ID changes should be ignored. + connection.localConnectionIdManager().close(); + connection.peerConnectionIdManager().close(); // while sending the packet containing the CONNECTION_CLOSE frame, the pushDatagram will // remap the QuicConnectionImpl in QuicEndpoint. connection.pushDatagram(protectionRecord); diff --git a/src/java.net.http/share/classes/jdk/internal/net/http/quic/PeerConnIdManager.java b/src/java.net.http/share/classes/jdk/internal/net/http/quic/PeerConnIdManager.java index 2bc759a920a..0646026e28b 100644 --- a/src/java.net.http/share/classes/jdk/internal/net/http/quic/PeerConnIdManager.java +++ b/src/java.net.http/share/classes/jdk/internal/net/http/quic/PeerConnIdManager.java @@ -65,6 +65,7 @@ final class PeerConnIdManager { private final QuicConnectionImpl connection; private final String logTag; private final boolean isClient; + private boolean closed; // when true, no more reset tokens are registered private enum State { INITIAL_PKT_NOT_RECEIVED_FROM_PEER, @@ -267,6 +268,7 @@ final class PeerConnIdManager { if (handshakeConnId == null) { throw new IllegalStateException("No handshake peer connection available"); } + if (closed) return; // recreate the conn id with the stateless token this.peerConnectionIds.put(0L, new PeerConnectionId(handshakeConnId.asReadOnlyBuffer(), statelessResetToken)); @@ -283,6 +285,10 @@ final class PeerConnIdManager { public List activeResetTokens() { lock.lock(); try { + // this method is currently only used to remove a connection from the endpoint + // after the connection is closed. + // The below assert can be removed if the method is needed elsewhere. + assert closed; // we only support one active connection ID at the time PeerConnectionId cid = peerConnectionIds.get(activeConnIdSeq); byte[] statelessResetToken = null; @@ -305,7 +311,7 @@ final class PeerConnIdManager { QuicConnectionId getPeerConnId() { lock.lock(); try { - if (activeConnIdSeq < largestReceivedRetirePriorTo) { + if (activeConnIdSeq < largestReceivedRetirePriorTo && !closed) { // stop using the old connection ID switchConnectionId(); } @@ -496,9 +502,11 @@ final class PeerConnIdManager { // connection ids. It does however store the peer-issued stateless reset token of a // peer connection id, so we let the endpoint know that the stateless reset token needs // to be forgotten since the corresponding peer connection id is being retired - final byte[] resetTokenToForget = entry.getValue().getStatelessResetToken(); - if (resetTokenToForget != null) { - this.connection.endpoint().forgetStatelessResetToken(resetTokenToForget); + if (seqNumToRetire == activeConnIdSeq) { + final byte[] resetTokenToForget = entry.getValue().getStatelessResetToken(); + if (resetTokenToForget != null) { + this.connection.endpoint().forgetStatelessResetToken(resetTokenToForget); + } } } for (Iterator iterator = gaps.iterator(); iterator.hasNext(); ) { @@ -540,4 +548,13 @@ final class PeerConnIdManager { lock.unlock(); } } + + public void close() { + lock.lock(); + try { + closed = true; + } finally { + lock.unlock(); + } + } } diff --git a/src/java.net.http/share/classes/jdk/internal/net/http/quic/QuicConnectionImpl.java b/src/java.net.http/share/classes/jdk/internal/net/http/quic/QuicConnectionImpl.java index 41b814a551c..b13d49ead7d 100644 --- a/src/java.net.http/share/classes/jdk/internal/net/http/quic/QuicConnectionImpl.java +++ b/src/java.net.http/share/classes/jdk/internal/net/http/quic/QuicConnectionImpl.java @@ -1758,6 +1758,10 @@ public class QuicConnectionImpl extends QuicConnection implements QuicPacketRece return localConnIdManager; } + PeerConnIdManager peerConnectionIdManager() { + return peerConnIdManager; + } + /** * {@return the local connection id} */ diff --git a/src/java.net.http/share/classes/jdk/internal/net/http/quic/QuicEndpoint.java b/src/java.net.http/share/classes/jdk/internal/net/http/quic/QuicEndpoint.java index ef342d4cb56..3dee814e1f1 100644 --- a/src/java.net.http/share/classes/jdk/internal/net/http/quic/QuicEndpoint.java +++ b/src/java.net.http/share/classes/jdk/internal/net/http/quic/QuicEndpoint.java @@ -1532,12 +1532,16 @@ public abstract sealed class QuicEndpoint implements AutoCloseable */ void removeConnection(final QuicPacketReceiver connection) { if (debug.on()) debug.log("removing connection " + connection); - // remove the connection completely - connection.connectionIds().forEach(connections::remove); - assert !connections.containsValue(connection) : connection; // remove references to this connection from the map which holds the peer issued // reset tokens dropPeerIssuedResetTokensFor(connection); + // remove the connection completely + connection.connectionIds().forEach(connections::remove); + assert !connections.containsValue(connection) : connection; + // Check that if there are no connections, there are no reset tokens either. + // This is safe because connections are added before reset tokens and removed after, + // except when we're closing the endpoint and don't bother with removing tokens. + assert peerIssuedResetTokens.isEmpty() || !connections.isEmpty() || closed : peerIssuedResetTokens; } /** @@ -1587,7 +1591,6 @@ public abstract sealed class QuicEndpoint implements AutoCloseable if (closed) return; final long idleTimeout = connection.peerPtoMs() * 3; // 3 PTO - connection.localConnectionIdManager().close(); DrainingConnection draining = new DrainingConnection(connection.connectionIds(), connection.activeResetTokens(), idleTimeout); // we can ignore stateless reset in the draining state. @@ -1626,7 +1629,6 @@ public abstract sealed class QuicEndpoint implements AutoCloseable closingDatagram.flip(); final long idleTimeout = connection.peerPtoMs() * 3; // 3 PTO - connection.localConnectionIdManager().close(); var closingConnection = new ClosingConnection(connection.connectionIds(), connection.activeResetTokens(), idleTimeout, datagram); remapPeerIssuedResetToken(closingConnection); diff --git a/test/jdk/java/net/httpclient/CancelRequestTest.java b/test/jdk/java/net/httpclient/CancelRequestTest.java index df808ad2dab..dcb60a55061 100644 --- a/test/jdk/java/net/httpclient/CancelRequestTest.java +++ b/test/jdk/java/net/httpclient/CancelRequestTest.java @@ -23,7 +23,7 @@ /* * @test - * @bug 8245462 8229822 8254786 8297075 8297149 8298340 8302635 + * @bug 8245462 8229822 8254786 8297075 8297149 8298340 8302635 8377181 * @summary Tests cancelling the request. * @library /test/lib /test/jdk/java/net/httpclient/lib * @key randomness diff --git a/test/jdk/java/net/httpclient/http3/H3ErrorHandlingTest.java b/test/jdk/java/net/httpclient/http3/H3ErrorHandlingTest.java index 8dfef7417e3..8e20eac95fa 100644 --- a/test/jdk/java/net/httpclient/http3/H3ErrorHandlingTest.java +++ b/test/jdk/java/net/httpclient/http3/H3ErrorHandlingTest.java @@ -71,7 +71,7 @@ import static org.junit.jupiter.api.Assertions.*; /* * @test - * @bug 8373409 + * @bug 8373409 8377181 * @key intermittent * @comment testResetControlStream may fail if the client doesn't read the stream type * before the stream is reset,