Made socket self-aware of when it is in a fatal state #4697

Added code to cleanly terminate connection on fatal socket state #4697

src/lib/net/IDataSocket.h

@@ -68,5 +68,6 @@ public:
 	virtual void		shutdownInput() = 0;
 	virtual void		shutdownOutput() = 0;
 	virtual bool		isReady() const = 0;
+	virtual bool		isFatal() const = 0;
 	virtual UInt32		getSize() const = 0;
 };

src/lib/net/TCPSocket.cpp

@@ -253,6 +253,14 @@ TCPSocket::isReady() const
 	return (m_inputBuffer.getSize() > 0);
 }
 
+bool
+TCPSocket::isFatal() const
+{
+	// TCP sockets aren't ever left in a fatal state.
+	LOG((CLOG_ERR "isFatal() not valid for non-secure connections"));
+	return false;
+}
+
 UInt32
 TCPSocket::getSize() const
 {
@@ -481,6 +489,9 @@ TCPSocket::serviceConnected(ISocketMultiplexerJob* job,
 					if (n > 0) {
 						s_retryOutputBufferSize = 0;
 					}
+					else if (n < 0) {
+						return NULL;
+					}
 				}
 				else {
 					return job;
@@ -537,6 +548,9 @@ TCPSocket::serviceConnected(ISocketMultiplexerJob* job,
 			if (isSecure()) {
 				if (isSecureReady()) {
 					n = secureRead(buffer, sizeof(buffer));
+					if (n < 0) {
+						return NULL;
+					}
 				}
 				else {
 					return job;
@@ -555,6 +569,9 @@ TCPSocket::serviceConnected(ISocketMultiplexerJob* job,
 
 					if (isSecure() && isSecureReady()) {
 						n = secureRead(buffer, sizeof(buffer));
+						if (n < 0) {
+							return NULL;
+						}
 					}
 					else {
 						n = ARCH->readSocket(m_socket, buffer, sizeof(buffer));

src/lib/net/TCPSocket.h

@@ -52,6 +52,7 @@ public:
 	virtual void		shutdownInput();
 	virtual void		shutdownOutput();
 	virtual bool		isReady() const;
+	virtual bool		isFatal() const;
 	virtual UInt32		getSize() const;
 
 	// IDataSocket overrides

src/lib/plugin/ns/SecureSocket.cpp

@@ -52,7 +52,8 @@ SecureSocket::SecureSocket(
 		SocketMultiplexer* socketMultiplexer) :
 	TCPSocket(events, socketMultiplexer),
 	m_secureReady(false),
-	m_maxRetry(MAX_RETRY_COUNT)
+	m_maxRetry(MAX_RETRY_COUNT),
+	m_fatal(false)
 {
 }
 
@@ -62,12 +63,14 @@ SecureSocket::SecureSocket(
 		ArchSocket socket) :
 	TCPSocket(events, socketMultiplexer, socket),
 	m_secureReady(false),
-	m_maxRetry(MAX_RETRY_COUNT)
+	m_maxRetry(MAX_RETRY_COUNT),
+	m_fatal(false)
 {
 }
 
 SecureSocket::~SecureSocket()
 {
+	isFatal(true);
 	if (m_ssl->m_ssl != NULL) {
 		SSL_shutdown(m_ssl->m_ssl);
 
@@ -78,13 +81,15 @@ SecureSocket::~SecureSocket()
 		SSL_CTX_free(m_ssl->m_context);
 		m_ssl->m_context = NULL;
 	}
-
+	ARCH->sleep(1);
 	delete m_ssl;
 }
 
 void
 SecureSocket::close()
 {
+	isFatal(true);
+
 	SSL_shutdown(m_ssl->m_ssl);
 
 	TCPSocket::close();
@@ -114,17 +119,23 @@ SecureSocket::secureRead(void* buffer, UInt32 n)
 		LOG((CLOG_DEBUG2 "reading secure socket"));
 		r = SSL_read(m_ssl->m_ssl, buffer, n);
 		
-		bool fatal;
 		static int retry;
 
-		checkResult(r, fatal, retry);
+		// Check result will cleanup the connection in the case of a fatal
+		checkResult(r, retry);
 		
 		if (retry) {
-			r = 0;
+			return 0;
-		}
 		}
 
-	return r > 0 ? (UInt32)r : 0;
+		if (isFatal()) {
+			return -1;
+		}
+	}
+	// According to SSL spec, r must not be negative and not have an error code
+	// from SSL_get_error(). If this happens, it is itself an error. Let the
+	// parent handle the case
+	return (UInt32)r;
 }
 
 UInt32
@@ -132,20 +143,27 @@ SecureSocket::secureWrite(const void* buffer, UInt32 n)
 {
 	int r = 0;
 	if (m_ssl->m_ssl != NULL) {
-		LOG((CLOG_DEBUG2 "writing secure socket"));
+		LOG((CLOG_DEBUG2 "writing secure socket:%p", this));
+
 		r = SSL_write(m_ssl->m_ssl, buffer, n);
 		
-		bool fatal;
 		static int retry;
 
-		checkResult(r, fatal, retry);
+		// Check result will cleanup the connection in the case of a fatal
+		checkResult(r, retry);
 
 		if (retry) {
-			r = 0;
+			return 0;
-		}
 		}
 
-	return r > 0 ? (UInt32)r : 0;
+		if (isFatal()) {
+			return -1;
+		}
+	}
+	// According to SSL spec, r must not be negative and not have an error code
+	// from SSL_get_error(). If this happens, it is itself an error. Let the
+	// parent handle the case
+	return (UInt32)r;
 }
 
 bool
@@ -260,12 +278,11 @@ SecureSocket::secureAccept(int socket)
 	LOG((CLOG_DEBUG2 "accepting secure socket"));
 	int r = SSL_accept(m_ssl->m_ssl);
 	
-	bool fatal;
 	static int retry;
 
-	checkResult(r, fatal, retry);
+	checkResult(r, retry);
 
-	if (fatal) {
+	if (isFatal()) {
 		// tell user and sleep so the socket isn't hammered.
 		LOG((CLOG_ERR "failed to accept secure socket"));
 		LOG((CLOG_INFO "client connection may not be secure"));
@@ -304,12 +321,11 @@ SecureSocket::secureConnect(int socket)
 	LOG((CLOG_DEBUG2 "connecting secure socket"));
 	int r = SSL_connect(m_ssl->m_ssl);
 	
-	bool fatal;
 	static int retry;
 
-	checkResult(r, fatal, retry);
+	checkResult(r, retry);
 
-	if (fatal) {
+	if (isFatal()) {
 		LOG((CLOG_ERR "failed to connect secure socket"));
 		return -1;
 	}
@@ -362,13 +378,11 @@ SecureSocket::showCertificate()
 }
 
 void
-SecureSocket::checkResult(int n, bool& fatal, int& retry)
+SecureSocket::checkResult(int n, int& retry)
 {
 	// ssl errors are a little quirky. the "want" errors are normal and
 	// should result in a retry.
 
-	fatal = false;
-
 	int errorCode = SSL_get_error(m_ssl->m_ssl, n);
 	switch (errorCode) {
 	case SSL_ERROR_NONE:
@@ -378,8 +392,8 @@ SecureSocket::checkResult(int n, bool& fatal, int& retry)
 
 	case SSL_ERROR_ZERO_RETURN:
 		// connection closed
-		retry = 0;
+		isFatal(true);
-		LOG((CLOG_DEBUG2 "SSL connection has been closed"));
+		LOG((CLOG_DEBUG "SSL connection has been closed"));
 		break;
 
 	case SSL_ERROR_WANT_READ:
@@ -389,7 +403,7 @@ SecureSocket::checkResult(int n, bool& fatal, int& retry)
 		retry += 1;
 		// If there are a lot of retrys, it's worth warning about
 		if ( retry % 5 == 0 ) {
-			LOG((CLOG_INFO "need to retry the same SSL function(%d) retry:%d", errorCode, retry));
+			LOG((CLOG_DEBUG "need to retry the same SSL function(%d) retry:%d", errorCode, retry));
 		}
 		else if ( retry == (maxRetry() / 2) ) {
 			LOG((CLOG_WARN "need to retry the same SSL function(%d) retry:%d", errorCode, retry));
@@ -416,27 +430,27 @@ SecureSocket::checkResult(int n, bool& fatal, int& retry)
 			}
 		}
 
-		fatal = true;
+		isFatal(true);
 		break;
 
 	case SSL_ERROR_SSL:
 		LOG((CLOG_ERR "a failure in the SSL library occurred"));
-		fatal = true;
+		isFatal(true);
 		break;
 
 	default:
 		LOG((CLOG_ERR "unknown secure socket error"));
-		fatal = true;
+		isFatal(true);
 		break;
 	}
 
 	// If the retry max would exceed the allowed, treat it as a fatal error
 	if (retry > maxRetry()) {
 		LOG((CLOG_ERR "Maximum retry count exceeded:%d",retry));
-		fatal = true;
+		isFatal(true);
 	}
 
-	if (fatal) {
+	if (isFatal()) {
 		retry = 0;
 		showError();
 		disconnect();

src/lib/plugin/ns/SecureSocket.h

@@ -44,6 +44,8 @@ public:
 	void				secureConnect();
 	void				secureAccept();
 	bool				isReady() const { return m_secureReady; }
+	bool				isFatal() const { return m_fatal; }
+	void				isFatal(bool b) { m_fatal = b; }
 	bool				isSecureReady();
 	bool				isSecure() { return true; }
 	UInt32				secureRead(void* buffer, UInt32 n);
@@ -60,7 +62,7 @@ private:
 	int					secureAccept(int s);
 	int					secureConnect(int s);
 	bool				showCertificate();
-	void				checkResult(int n, bool& fatal, int& retry);
+	void				checkResult(int n, int& retry);
 	void				showError(const char* reason = NULL);
 	String				getError();
 	void				disconnect();
@@ -80,5 +82,6 @@ private:
 private:
 	Ssl*				m_ssl;
 	bool				m_secureReady;
+	bool				m_fatal;
 	int					m_maxRetry;
 };

src/lib/server/ClientListener.cpp

@@ -142,26 +142,34 @@ ClientListener::handleClientConnecting(const Event&, void*)
 {
 	// accept client connection
 	IDataSocket* socket	= m_listen->accept();
-	synergy::IStream* stream  = socket;
 
-	if (stream == NULL) {
+	if (socket == NULL) {
 		return;
 	}
 
 	LOG((CLOG_NOTE "accepted client connection"));
 
+	if (m_useSecureNetwork) {
+		LOG((CLOG_DEBUG2 "attempting sercure Connection"));
+		while(!socket->isReady()) {
+			if(socket->isFatal()) {
+				m_listen->deleteSocket(socket);
+				return;
+			}
+			LOG((CLOG_DEBUG2 "retrying sercure Connection"));
+			ARCH->sleep(.5f);
+		}
+	}
+
+	LOG((CLOG_DEBUG2 "sercure Connection established:%d"));
+
+	synergy::IStream* stream  = socket;
 	// filter socket messages, including a packetizing filter
 	bool adopt = !m_useSecureNetwork;
 	stream = new PacketStreamFilter(m_events, stream, adopt);
 
 	assert(m_server != NULL);
 
-	if (m_useSecureNetwork) {
-		while(!socket->isReady()) {
-			ARCH->sleep(.5f);
-		}
-	}
-
 	// create proxy for unknown client
 	ClientProxyUnknown* client = new ClientProxyUnknown(stream, 30.0, m_server, m_events);
 	m_newClients.insert(client);