On Fri, 27 Feb 2026 13:30:12 GMT, Volkan Yazici <[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/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? This is to make tests fail if the assertion is fired. Typically an exception while decoding/parsing would just cause the packet to be skipped. An assertion here indicates that invariants have been violated (a problem in our own code which should never happen), not that bad/unexpected bytes were received. If that happens (assert fired), we want to know about it. Assertions are typically enabled in testing (they are when running JDK tests) but not in production. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/29956#discussion_r2864616262
