TooTallNate / Java-WebSocket

A barebones WebSocket client and server implementation written in 100% Java.
http://tootallnate.github.io/Java-WebSocket
MIT License
10.47k stars 2.57k forks source link

WebSocketServer may miss the first GET request if transmitted back-to-back with the SSL handshake finish #1418

Closed robert-s-ubi closed 3 months ago

robert-s-ubi commented 3 months ago

Describe the bug If an SSL WebSocket client sends the first GET request so quickly after the SSL handshake finish message that the server gets both SSL data records in one read, the GET request may be lost.

Cause: The logic that was introduced to ensure the remainder of two concatenated SSL records is recovered with this commit:

https://github.com/TooTallNate/Java-WebSocket/commit/7bfa83d1c9a567e2cdf4ecb80edbf5a5e914cf32

only takes effect when SSLSocketChannel2#processHandshake() is called from WebSocketServer#doRead(). But the method is also called from WebSocketServer#doWrite(). And in that case, the remainder is stored, but there is no mechanism in WebSocketServer#doWrite() that takes care of retrieving remainders, only WebSocketServer#doRead() has that code (by calling isNeededRead(), which checks for remainders in SSLSocketChannel2).

Adding a boolean parameter to SSLSocketChannel2#processHandshake() that is true when called from doRead() and false otherwise, which wraps lines 170 through 185 in an if block depending on that parameter, to prevent processing received handshake data when called through WebSocketServer#doWrite() seems to fix this.

To Reproduce Steps to reproduce the behavior: Difficult. We never ran into this until we tried executing a test (connecting a Java-WebSocketClient to a JavaWebSocketServer listening on localhost) in an Ubuntu 22.04 VM with Linux Kernel 6.5, and then it only happens sporadically. We never hit that issue on Ubuntu 20.04 nor on MacOS. Maybe the new Linux Kernel version makes the difference?

Example application to reproduce the issue Unfortunately, I don't have an example for publication. I hope the description is good enough that this can be covered by code review?

Expected behavior The recovery mechanism introduced with commit 7bfa83d1c9a567e2cdf4ecb80edbf5a5e914cf32 should work reliably, and not miss out under certain circumstances.

Debug log Unfortunately, I cannot offer that either.

Environment(please complete the following information):

Additional context None

PhilipRoman commented 3 months ago

Thanks a lot for the detailed report! Your change looks sensible, I will have a look at this over the weekend.

robert-s-ubi commented 3 months ago

I have submitted a pull request with the fix that works for us.