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

Non-blocking socket not handled correctly. #610

Open
ioquatix opened this issue Mar 19, 2023 · 4 comments
Open

Non-blocking socket not handled correctly. #610

ioquatix opened this issue Mar 19, 2023 · 4 comments

Comments

@ioquatix
Copy link
Member

ret = SSL_shutdown(ssl);

According to the documentation, SSL_shutdown can:

If the underlying BIO is nonblocking, SSL_shutdown() will also return when the underlying BIO could not satisfy the needs of SSL_shutdown() to continue the handshake. In this case a call to SSL_get_error() with the return value of SSL_shutdown() will yield SSL_ERROR_WANT_READ or SSL_ERROR_WANT_WRITE. The calling process then must repeat the call after taking appropriate action to satisfy the needs of SSL_shutdown(). The action depends on the underlying BIO. When using a nonblocking socket, nothing is to be done, but select() can be used to check for the required condition. When using a buffering BIO, like a BIO pair, data must be written into or retrieved out of the BIO before being able to continue.

I don't know if we need to explicitly handle this when in non-blocking mode or not, but I'd assume so until otherwise confirmed/checked.

@rhenium
Copy link
Member

rhenium commented Jun 6, 2023

The underlying socket is always set to non-blocking by us in SSLSocket#initialize.

That SSL_shutdown() is not retried on error is a known issue, but I'm not sure how we're supposed to fix it.

openssl/ext/openssl/ossl_ssl.c

Lines 2130 to 2136 in 94fb921

/*
* XXX: Something happened. Possibly it failed because the underlying socket
* is not writable/readable, since it is in non-blocking mode. We should do
* some proper error handling using SSL_get_error() and maybe retry, but we
* can't block here. Give up for now.
*/
ossl_clear_error();

Would it be acceptable for IOish#close to potentially block? I have an impression it could be surprising.

@ioquatix
Copy link
Member Author

I think it's okay. IO#close can block in some cases.

@rhenium
Copy link
Member

rhenium commented Feb 5, 2024

I don't know about other IO, but as far as I understand TCP sockets' #close can only block with the setsockopt(SO_LINGER) option.

I think the blocking behavior (ensuring close_notify to be sent) needs the user's explicit request. Changing the default behavior can be a DoS issue as I think people usually call SSLSocket#close without a timeout.

Perhaps we can just expose SSL_shutdown() as a new method?

@ioquatix
Copy link
Member Author

ioquatix commented Feb 5, 2024

Maybe in the first instance we can log when it fails, and then collect data about how frequently it happens?

I think we could consider exposing SSL_shutdown as part of #609

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants