Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
robert-s-ubi opened this issue Jun 7, 2024 · 2 comments · Fixed by #1419

Comments

@robert-s-ubi
Copy link
Contributor

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:

7bfa83d

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 7bfa83d should work reliably, and not miss out under certain circumstances.

Debug log
Unfortunately, I cannot offer that either.

Environment(please complete the following information):

  • Version used: v1.5.3
  • Java version: 8
  • Operating System and version: Ubuntu 22.04
  • Endpoint Name and version: -
  • Link to your project: -

Additional context
None

@PhilipRoman
Copy link
Collaborator

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

robert-s-ubi added a commit to ubitricity/Java-WebSocket that referenced this issue Jun 8, 2024
On Ubuntu 22.04 with Linux 6.5, it was observed that when the server
gets the SSL records containing the client handshake finished message
and the first HTTP GET request in ONE read operation, the latter SSL
record is never processed.

Commit 89eaf41 should have fixed this,
but it turned out that when SSLSocketChannel2#processHandshake() is
called from SSLSocketChannel2#write(), the second SSL record containing
the HTTP GET request is stashed away, but never retrieved, since the
calling code in WebSocketServer#doWrite() has no provisions for this,
only WebSocketServer#doRead() does.

Change SSLSocketChannel2#processHandshake() to only read from the socket
when called from SSLSocketChannel#read(), to ensure that when two SSL
records are read, the second one is processed as well.

This fixes issue TooTallNate#1418.
@robert-s-ubi
Copy link
Contributor Author

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

@marci4 marci4 added this to the Release 1.5.7 milestone Jun 8, 2024
PhilipRoman added a commit that referenced this issue Jun 10, 2024
…er_misses_get

Fix issue #1418: WebSocketServer sometimes misses GET request after SSL handshake
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants