IndySockets / Indy

Indy - Internet Direct
https://www.indyproject.org
451 stars 153 forks source link

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

Open rapa4362 opened 9 months ago

rapa4362 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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.

rapa4362 commented 9 months ago

Hi sorry for confusion, I thought I'm posting an issue to pull request "for OpenSSL 1.1.1" https://github.com/IndySockets/Indy/pull/299

rlebeau commented 9 months ago

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.