From a5bd11f3a75c2a2e1a9611db2aa1ae679c452aa8 Mon Sep 17 00:00:00 2001
From: Simon Rettberg
Date: Mon, 13 Mar 2023 12:23:40 +0100
Subject: [*] Better error handling and messaging

Server now properly sends a connection termination reason to the client,
which will log it to the console, for better debugging.
---
 src/client/net/serverconnection.cpp |  5 ++++
 src/server/net/client.cpp           | 46 +++++++++++++++++++++++--------------
 src/server/net/client.h             |  2 +-
 3 files changed, 35 insertions(+), 18 deletions(-)

diff --git a/src/client/net/serverconnection.cpp b/src/client/net/serverconnection.cpp
index aae7538..690ba23 100644
--- a/src/client/net/serverconnection.cpp
+++ b/src/client/net/serverconnection.cpp
@@ -103,6 +103,11 @@ void ServerConnection::handleMsg()
 	_lastData = QDateTime::currentMSecsSinceEpoch() + PING_TIMEOUT_MS;
 	const QString &id = _fromServer.getFieldString(_ID);
 
+	if (id == _ERROR) {
+		qWarning() << "Server sent error message:" << _fromServer.getFieldString(_ERROR);
+		return;
+	}
+
 	if (_authed == 0) {
 		if (id == _CHALLENGE) {
 			// Initial challenge request by server
diff --git a/src/server/net/client.cpp b/src/server/net/client.cpp
index bf1ad25..51ffb61 100644
--- a/src/server/net/client.cpp
+++ b/src/server/net/client.cpp
@@ -15,6 +15,7 @@
 #include <QNetworkInterface>
 #include <QTcpSocket>
 #include <QSslSocket>
+#include <QTimer>
 
 #define CHALLENGE_LEN 20
 
@@ -24,19 +25,25 @@ Client::Client(QTcpSocket* socket)
 {
 	assert(socket != nullptr);
 	_desiredSource = NO_SOURCE;
-	_socket->setParent(this);
+	_socket->setParent(nullptr);
 	_id = ++_clientIdCounter;
 	//_ip = _socket->peerAddress().toString();
 	qDebug("*** Client %s created.", qPrintable(_socket->peerAddress().toString()));
 	// Connect important signals
 	connect(_socket, &QTcpSocket::disconnected,
-	        this, &Client::disconnect);
+			[this]() {
+		this->disconnect("Client closed connection");
+	});
 	connect(_socket, QOverload<QAbstractSocket::SocketError>::of(&QTcpSocket::errorOccurred),
-	        this, &Client::disconnect);
+			[this](QAbstractSocket::SocketError) {
+		this->disconnect("Client socket error");
+	});
 	auto *ssl = qobject_cast<QSslSocket*>(_socket);
 	if (ssl != nullptr) {
 		connect(ssl, QOverload<const QList<QSslError> &>::of(&QSslSocket::sslErrors),
-				this, &Client::disconnect);
+				[this](const QList<QSslError> &) {
+			this->disconnect("Client SSL Errors");
+		});
 	}
 	connect(_socket, &QTcpSocket::readyRead,
 	        this, &Client::onDataArrival);
@@ -58,21 +65,25 @@ Client::Client(QTcpSocket* socket)
 Client::~Client()
 {
 	qDebug() << "*** Client"  << _host  << " destroyed.";
+	_socket->blockSignals(true);
+	QTcpSocket *sck = _socket;
+	QTimer::singleShot(10, [sck]() {
+		sck->deleteLater();
+	});
 }
 
 void Client::timerEvent(QTimerEvent* event)
 {
 	if (event->timerId() == _timerPingTimeout) {
 		if (_pingTimeout < QDateTime::currentMSecsSinceEpoch()) {
-			qDebug() << "Client" << _socket->peerAddress().toString() << "has a ping timeout.";
 			killTimer(_timerPingTimeout);
-			this->disconnect();
+			this->disconnect("Disconnecting client because of ping timeout");
 		}
 	} else if (event->timerId() == _timerIdAuthTimeout) {
 		// Client did not send login request within 3 seconds
 		killTimer(_timerIdAuthTimeout);
 		_timerIdAuthTimeout = 0;
-		this->disconnect();
+		this->disconnect("Did not authenticate withing three seconds");
 	} else
 		killTimer(event->timerId());
 }
@@ -121,7 +132,7 @@ void Client::onDataArrival()
 	while (_socket->bytesAvailable() > 0) {
 		int ret = _fromClient.readMessage(_socket); // let the message read data from socket
 		if (ret == NM_READ_FAILED) { // error parsing msg, disconnect client!
-			this->disconnect();
+			this->disconnect("Malformed message received from client.");
 			return;
 		}
 		if (ret == NM_READ_INCOMPLETE)
@@ -209,8 +220,7 @@ void Client::handleMsg()
 			// emit event, see if request is accepted
 			emit authenticating(this, &request);
 			if (!request.accept) {
-				qDebug("Request denied.");
-				this->disconnect(); // Nope
+				this->disconnect("Login request denied."); // Nope
 				return;
 			}
 			// Accepted
@@ -235,10 +245,7 @@ void Client::handleMsg()
 			if (genSha1(&serverApp->sessionNameArray(), &_challenge) != hash
 			        && !(serverApp->getCurrentRoom()->clientPositions.contains(_socket->peerAddress().toString()))) {
 				// Challenge reply is invalid, drop client
-				NetworkMessage msgErr;
-				msgErr.buildErrorMessage("Challenge reply invalid.");
-				msgErr.writeMessage(_socket);
-				this->disconnect();
+				this->disconnect("Challenge reply invalid.");
 				return;
 			}
 			// Now answer to challenge by client
@@ -325,11 +332,16 @@ void Client::lockScreen(bool lock)
 	emit stateChanged();
 }
 
-void Client::disconnect()
+void Client::disconnect(const char *errmsg)
 {
-	qDebug("*** Client %s disconnected.", qPrintable(_socket->peerAddress().toString()));
+	qDebug() << "*** Client" << _socket->peerAddress().toString() << "disconnected:" << errmsg;
+	if (_socket->state() == QAbstractSocket::ConnectedState) {
+		NetworkMessage msgErr;
+		msgErr.buildErrorMessage(errmsg);
+		msgErr.writeMessage(_socket);
+		_socket->flush();
+	}
 	_socket->blockSignals(true);
-	_socket->abort();
 	this->deleteLater();
 	emit disconnected();
 }
diff --git a/src/server/net/client.h b/src/server/net/client.h
index 3b95321..5a4ac79 100644
--- a/src/server/net/client.h
+++ b/src/server/net/client.h
@@ -101,7 +101,7 @@ signals:
 
 private slots:
 	void onDataArrival(); // triggered if data is available for reading
-	void disconnect();
+	void disconnect(const char *errmsg);
 
 };
 
-- 
cgit v1.2.3-55-g7522