On Fri, 27 Feb 2026 11:47:39 GMT, Daniel Fuchs <[email protected]> wrote:
> PeerConnectionId has a constructor that takes a ByteBuffer containing the > peer connection id bytes. This constructor extracts the connection id bytes > from the given byte buffer and makes a private copy for itself. However, > while doing so, it uses a relative bulk get instead of an absolute bulk get, > which causes the original byte buffer position to move to the limit. > > This is causing trouble on the server side, this is the only place where that > constructor is used without making a slice of the original buffer first. In > some circumstances (if the connection id is not found) it can cause the first > initial packet to get dropped after creating the connection. This went > unnoticed because the client will then retransmit that packet, and the > connection id will be found the next time around. > > Though the issue is in product source code it only affects the server side > used by httpclient tests. > > The proposed change first adds some assert statements to > `QuicConnectionImpl::internalProcessIncoming` and ensure that the connection > will get closed if the assert fires. This made it possible to reproduce and > surface the issue quite easily using existing httpclient tests: a lot of > tests started failing with that change ([this is done by the first commit in > this > PR](https://github.com/openjdk/jdk/pull/29956/changes/b81ce5d405054b06afdf70738119916bba08ce72)). > The [second > commit](https://github.com/openjdk/jdk/pull/29956/changes/108660362d4fa78e61c784931fe6d45e002381dc) > fixes the issue by making `PeerConnectionId::cloneBuffer` use an absolute > bulk get instead of a relative bulk get. The absolute bulk get ensures that > the position of the input buffer is not moved while copying it. > > I am marking the issue as `noreg-hard` - though it could be very easily > verified by undoing the second commit, recompiling the JDK, and running the > test with that. Then add the second commit, recompile, and see that all tests > are passing. src/java.net.http/share/classes/jdk/internal/net/http/quic/PeerConnectionId.java line 77: > 75: final byte[] idBytes = new byte[src.remaining()]; > 76: src.get(src.position(), idBytes); > 77: return ByteBuffer.wrap(idBytes); You can alternatively consider replacing these 3 lines with `return src.slice(src.position(), src.limit())`, I guess. src/java.net.http/share/classes/jdk/internal/net/http/quic/QuicConnectionImpl.java line 1949: > 1947: if (t instanceof AssertionError) { > 1948: > this.terminator.terminate(TerminationCause.forException(t)); > 1949: } Would you mind explaining the rationale for this change, please? ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/29956#discussion_r2864338979 PR Review Comment: https://git.openjdk.org/jdk/pull/29956#discussion_r2864355187
