> 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.
Daniel Fuchs has updated the pull request incrementally with one additional commit since the last revision: Review feedback: add comment ------------- Changes: - all: https://git.openjdk.org/jdk/pull/29956/files - new: https://git.openjdk.org/jdk/pull/29956/files/10866036..8fbdb046 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=29956&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=29956&range=00-01 Stats: 5 lines in 1 file changed: 5 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/29956.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/29956/head:pull/29956 PR: https://git.openjdk.org/jdk/pull/29956
