search

uNetworking / uSockets

Miniscule cross-platform eventing, networking & crypto for async applications
Apache License 2.0
1.29k stars 268 forks source link

SSL error queue must be clear before calling SSL_get_error. #43

Closed MDFL64 closed 5 years ago

MDFL64 commented 5 years ago

In my application using uWebsockets.js with SSL, all sockets connected to the server will disconnect if one is closed with WebSocket.end. I believe I've narrowed the cause down to the two calls to SSL_get_error in ssl.c. After the first socket is closed, the rest will report incorrect error codes and be disconnected as well.

From the OpenSSL docs:

SSL_get_error() must be used in the same thread that performed the TLS/SSL I/O operation, and no other OpenSSL function calls should appear in between. The current thread's error queue must be empty before the TLS/SSL I/O operation is attempted, or SSL_get_error() will not work reliably.

Adding calls to ERR_clear_error before SSL_read/SSL_write fixed the issue for me. I'm submitting an issue rather than a PR because I'm not sure this was the best way to solve the problem, or if the intermediate call to us_socket_flush in us_ssl_socket_write could cause any issues.

ghost commented 5 years ago

Good report :+1: Will look into this and come back

ghost commented 5 years ago

I cannot reproduce this, you mind creating a reproducing test?

MDFL64 commented 5 years ago

I am updating my fork today, and I'm still encountering it. I probably should have mentioned I've only encountered this on linux, and only when the connection is terminated by the server. I'll try to put together a test in the next couple of days.

ghost commented 5 years ago

Yes please do, I run Linux

MDFL64 commented 5 years ago

I have created a gist with information, a test program, and the fix I used.

https://gist.github.com/birdbrainswagtrain/149ca5e64729628168855395e494ea8c

ghost commented 5 years ago

Early Christmas, thanks :1st_place_medal:

ghost commented 5 years ago

I cannot reproduce it, but let's merge this anyways. It does make sense to clear it, even if OpenSSL is a complete mess - I checked what that ERR_clear_error() function does and it seems super complex with a bunch of unnecessary garbage.

ghost commented 5 years ago

Calling ERR_clear_error() before SSL_read and SSL_write has a measurable cost,

I reliably get 110k messages/sec without it and 107k with it. That one call is costly.

Tracking WHEN we get an error and clearing it THEN would be a much better solution - we know when functions fail so we can track when we need to clear and when not to.

ghost commented 5 years ago

@birdbrainswagtrain Do you want to try the latest master? I think it should be fixed now