SecureSocketImpl::shutdown() regression since 1.14.0

[micheleselea]

The function SecureSocketImpl::shutdown() was changed in 1.14.0
before the function was coded like that:

void SecureSocketImpl::shutdown()
{
	if (_pSSL)
	{
        // Don't shut down the socket more than once.
        int shutdownState = SSL_get_shutdown(_pSSL);
        bool shutdownSent = (shutdownState & SSL_SENT_SHUTDOWN) == SSL_SENT_SHUTDOWN;
        if (!shutdownSent)
        {
			// A proper clean shutdown would require us to
			// retry the shutdown if we get a zero return
			// value, until SSL_shutdown() returns 1.
			// However, this will lead to problems with
			// most web browsers, so we just set the shutdown
			// flag by calling SSL_shutdown() once and be
			// done with it.
#if OPENSSL_VERSION_NUMBER >= 0x30000000L
			int rc = 0;
			if (!_bidirectShutdown)
				rc = SSL_shutdown(_pSSL);
			else
			{
				Poco::Timespan recvTimeout = _pSocket->getReceiveTimeout();
				Poco::Timespan pollTimeout(0, 100000);
				Poco::Timestamp tsNow;
				do
				{
					rc = SSL_shutdown(_pSSL);
					if (rc == 1) break;
					if (rc < 0)
					{
						int err = SSL_get_error(_pSSL, rc);
						if (err == SSL_ERROR_WANT_READ)
							_pSocket->poll(pollTimeout, Poco::Net::Socket::SELECT_READ);
						else if (err == SSL_ERROR_WANT_WRITE)
							_pSocket->poll(pollTimeout, Poco::Net::Socket::SELECT_WRITE);
						else
						{
							int socketError = SocketImpl::lastError();
							long lastError = ERR_get_error();
							if ((err == SSL_ERROR_SSL) && (socketError == 0) && (lastError == 0x0A000123))
								rc = 0;
							break;
						}
					}
					else _pSocket->poll(pollTimeout, Poco::Net::Socket::SELECT_READ);
				} while (!tsNow.isElapsed(recvTimeout.totalMicroseconds()));
			}
#else
			int rc = SSL_shutdown(_pSSL);
#endif
			if (rc < 0) handleError(rc);
			if (_pSocket->getBlocking())
			{
				_pSocket->shutdown();
			}
		}
	}
}

The function now is coded like that

int SecureSocketImpl::shutdown()
{
	if (_pSSL)
	{
		UnLockT l(_mutex);

		int shutdownState = ::SSL_get_shutdown(_pSSL);
		bool shutdownSent = (shutdownState & SSL_SENT_SHUTDOWN) == SSL_SENT_SHUTDOWN;
		if (!shutdownSent)
		{
			int rc = ::SSL_shutdown(_pSSL);
			if (rc < 0) 
			{
				if (SocketImpl::lastError() == POCO_EWOULDBLOCK)
					rc = SecureStreamSocket::ERR_SSL_WANT_WRITE;
				else
					rc = handleError(rc);
			}

			l.unlock();

			if (rc >= 0)
			{
				_pSocket->shutdownSend();
			}
			return rc;
		}
		else 
		{
			return (shutdownState & SSL_RECEIVED_SHUTDOWN) == SSL_RECEIVED_SHUTDOWN;
		}
	}
	return 1;
}

The code was used to address the problem of SSL connection established for sending data and never receiving data (like FTP DATA connections)
is there a reason why the function was changed?

[matejk]

Changed in 1811f2f.

@obiltschnig ?

[obiltschnig]

This was done to support non-blocking operation. If you want to re-introduce this changes, it has to be done in such a way as to not block if the socket is nonblocking, meaning that calls to poll() and the loop must not be done in non-blocking mode.

[obiltschnig]

Also, for these kind of changes, a test case would be good.

[micheleselea]

The scenario is this

Socket TCP SSL connecting to a SSL server
send 5MB of data
closing socket

without that code server can receive less than 5MB of data
the pattern is the FTP data socket connection