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

EIdOpenSSLShutdownError on TCP server shutdown after verifying SSL_shutdown result code #513

Open
rapa4362 opened this issue Dec 14, 2023 · 4 comments · May be fixed by #299
Open

EIdOpenSSLShutdownError on TCP server shutdown after verifying SSL_shutdown result code #513

rapa4362 opened this issue Dec 14, 2023 · 4 comments · May be fixed by #299
Labels
Element: SSL/TLS Issues related to SSL/TLS handling, TIdSSLIOHandlerSocketBase and descendants Element: TCP Issues related to TCP handling, TIdTCPClient and TIdTCPServer descendants, etc Status: More Info Needed Issue needs further information to continue progress Type: Bug Issue is a bug in existing code

Comments

@rapa4362
Copy link

With recent IdCustomTCPServer code is no longer disconnecting in TIdCustomTCPServer.DoTerminateContext(AContext: TIdContext), but instead closes socket with AContext.Binding.CloseSocket. This then causes SSL_shutdown in TIdOpenSSLSocket.Shutdown to return -1 that leads to EIdOpenSSLShutdownError. Original OpenSSL handler calls SSL_shutdown and gets -1 in the same way but doesn't check the result and no exception is raised. Does it really need to raise exception on server shutdown? As noted by RLebeau calling disconnect might cause AVs (and I was getting AV with AContext.Connection.Disconnect), but at least it was not raising exception on every shutdown.

@rapa4362 rapa4362 added Status: Reported Issue has been reported for review Type: Question Issue is asking a question, or requesting support/clarity labels Dec 14, 2023
@rapa4362
Copy link
Author

BTW, when SSL_get_error returns 5 (SSL_ERROR_SYSCALL), It would be wise to include actual message from SysErrorMessage(GetLastError) that was in this case returning 'An operation was attempted on something that is not a socket". Raising 'Failed to shutdown the TLS connection.', doesn't give much clues what happened.

@rlebeau rlebeau added Type: Bug Issue is a bug in existing code Element: SSL/TLS Issues related to SSL/TLS handling, TIdSSLIOHandlerSocketBase and descendants Element: TCP Issues related to TCP handling, TIdTCPClient and TIdTCPServer descendants, etc Status: Review Needed Issue needs further review to decide next status and removed Type: Question Issue is asking a question, or requesting support/clarity Status: Reported Issue has been reported for review labels Dec 14, 2023
@rlebeau
Copy link
Member

rlebeau commented Dec 14, 2023

Can you be a little more specific about which code is failing exactly?

There is no TIdOpenSSLSocket class in Indy, did you mean TIdSSLSocket? But it does not have a Shutdown() method. In fact, there is no Shutdown() method in Indy that calls SSL_shutdown(). And for that matter, there is no EIdOpenSSLShutdownError class in Indy, either!

There are only 3 calls to SSL_shutdown() in Indy - 2 in TIdSSLIOHandlerSocketOpenSSL.SetPassThrough(), and 1 in TIdSSLSocket.Destroy(), and none of them check the error code, so there should be no exception raised.

@rlebeau rlebeau added Status: More Info Needed Issue needs further information to continue progress and removed Status: Review Needed Issue needs further review to decide next status labels Dec 14, 2023
@rapa4362
Copy link
Author

Hi sorry for confusion, I thought I'm posting an issue to pull request "for OpenSSL 1.1.1" #299

@rlebeau rlebeau linked a pull request Dec 20, 2023 that will close this issue
@rlebeau
Copy link
Member

rlebeau commented Dec 20, 2023

Ah, OK. That makes more sense why I didn't recognize what you are describing, since I'm not very familiar with that new code yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Element: SSL/TLS Issues related to SSL/TLS handling, TIdSSLIOHandlerSocketBase and descendants Element: TCP Issues related to TCP handling, TIdTCPClient and TIdTCPServer descendants, etc Status: More Info Needed Issue needs further information to continue progress Type: Bug Issue is a bug in existing code
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants