m_spanningtree Throw an exception on protocol violations instead of returning CMD_INVALID

Catch CoreExceptions, log and close the link in OnDataReady()

10 files changed, 48 insertions, 59 deletions

diff --git a/src/modules/m_spanningtree/commands.h b/src/modules/m_spanningtree/commands.h
index 0c1ea8e49..d2d138ab2 100644
--- a/src/modules/m_spanningtree/commands.h
+++ b/src/modules/m_spanningtree/commands.h
@@ -130,7 +130,7 @@ class CommandFJoin : public ServerCommand
 	 * @param newname The new name of the channel; must be the same or a case change of the current name
 	 */
 	static void LowerTS(Channel* chan, time_t TS, const std::string& newname);
-	bool ProcessModeUUIDPair(const std::string& item, TreeSocket* src_socket, Channel* chan, irc::modestacker* modestack);
+	void ProcessModeUUIDPair(const std::string& item, TreeSocket* src_socket, Channel* chan, irc::modestacker* modestack);
  public:
 	CommandFJoin(Module* Creator) : ServerCommand(Creator, "FJOIN", 3) { }
 	CmdResult Handle(User* user, std::vector<std::string>& params);
diff --git a/src/modules/m_spanningtree/fjoin.cpp b/src/modules/m_spanningtree/fjoin.cpp
index ea7711332..ec7932766 100644
--- a/src/modules/m_spanningtree/fjoin.cpp
+++ b/src/modules/m_spanningtree/fjoin.cpp
@@ -157,8 +157,7 @@ CmdResult CommandFJoin::Handle(User* srcuser, std::vector<std::string>& params)
 	irc::modestacker* modestackptr = (apply_other_sides_modes ? &modestack : NULL);
 	while (users.GetToken(item))
 	{
-		if (!ProcessModeUUIDPair(item, src_socket, chan, modestackptr))
-			return CMD_INVALID;
+		ProcessModeUUIDPair(item, src_socket, chan, modestackptr);
 	}
 
 	/* Flush mode stacker if we lost the FJOIN or had equal TS */
@@ -168,7 +167,7 @@ CmdResult CommandFJoin::Handle(User* srcuser, std::vector<std::string>& params)
 	return CMD_SUCCESS;
 }
 
-bool CommandFJoin::ProcessModeUUIDPair(const std::string& item, TreeSocket* src_socket, Channel* chan, irc::modestacker* modestack)
+void CommandFJoin::ProcessModeUUIDPair(const std::string& item, TreeSocket* src_socket, Channel* chan, irc::modestacker* modestack)
 {
 	std::string::size_type comma = item.find(',');
 
@@ -178,14 +177,14 @@ bool CommandFJoin::ProcessModeUUIDPair(const std::string& item, TreeSocket* src_
 	if (!who)
 	{
 		// Probably KILLed, ignore
-		return true;
+		return;
 	}
 
 	/* Check that the user's 'direction' is correct */
 	TreeServer* route_back_again = TreeServer::Get(who);
 	if (route_back_again->GetSocket() != src_socket)
 	{
-		return true;
+		return;
 	}
 
 	/* Check if the user received at least one mode */
@@ -196,10 +195,7 @@ bool CommandFJoin::ProcessModeUUIDPair(const std::string& item, TreeSocket* src_
 		for (std::string::const_iterator i = item.begin(); i != commait; ++i)
 		{
 			if (!ServerInstance->Modes->FindMode(*i, MODETYPE_CHANNEL))
-			{
-				ServerInstance->SNO->WriteToSnoMask('d', "Unrecognised mode '%c' for a user in FJOIN, dropping link", *i);
-				return false;
-			}
+				throw ProtocolException("Unrecognised mode '" + std::string(1, *i) + "'");
 
 			/* Add any modes this user had to the mode stack */
 			modestack->Push(*i, who->nick);
@@ -207,7 +203,6 @@ bool CommandFJoin::ProcessModeUUIDPair(const std::string& item, TreeSocket* src_
 	}
 
 	chan->ForceJoin(who, NULL, route_back_again->bursting);
-	return true;
 }
 
 void CommandFJoin::RemoveStatus(Channel* c)
diff --git a/src/modules/m_spanningtree/fmode.cpp b/src/modules/m_spanningtree/fmode.cpp
index 48e4a19b9..5627b023b 100644
--- a/src/modules/m_spanningtree/fmode.cpp
+++ b/src/modules/m_spanningtree/fmode.cpp
@@ -50,7 +50,7 @@ CmdResult CommandFMode::Handle(User* who, std::vector<std::string>& params)
 			return CMD_FAILURE;
 
 		if (IS_SERVER(user))
-			return CMD_INVALID;
+			throw ProtocolException("Invalid target");
 
 		ourTS = user->age;
 	}
diff --git a/src/modules/m_spanningtree/ijoin.cpp b/src/modules/m_spanningtree/ijoin.cpp
index a579848c8..637321bcb 100644
--- a/src/modules/m_spanningtree/ijoin.cpp
+++ b/src/modules/m_spanningtree/ijoin.cpp
@@ -48,10 +48,7 @@ CmdResult CommandIJoin::HandleRemote(RemoteUser* user, std::vector<std::string>&
 		}
 
 		if (RemoteTS < chan->age)
-		{
-			ServerInstance->Logs->Log(MODNAME, LOG_DEFAULT, "Attempted to lower TS via IJOIN. Channel=" + params[0] + " RemoteTS=" + params[1] + " LocalTS=" + ConvToStr(chan->age));
-			return CMD_INVALID;
-		}
+			throw ProtocolException("Attempted to lower TS via IJOIN. LocalTS=" + ConvToStr(chan->age));
 		apply_modes = ((params.size() > 2) && (RemoteTS == chan->age));
 	}
 	else
@@ -73,10 +70,7 @@ CmdResult CommandResync::HandleServer(TreeServer* server, std::vector<std::strin
 	}
 
 	if (!server->IsLocal())
-	{
-		ServerInstance->Logs->Log(MODNAME, LOG_DEFAULT, "Received RESYNC with a source that is not directly connected: " + server->GetID());
-		return CMD_INVALID;
-	}
+		throw ProtocolException("RESYNC from a server that is not directly connected");
 
 	// Send all known information about the channel
 	server->GetSocket()->SyncChannel(chan);
diff --git a/src/modules/m_spanningtree/metadata.cpp b/src/modules/m_spanningtree/metadata.cpp
index 5dea7ffae..d151bc450 100644
--- a/src/modules/m_spanningtree/metadata.cpp
+++ b/src/modules/m_spanningtree/metadata.cpp
@@ -35,7 +35,7 @@ CmdResult CommandMetadata::Handle(User* srcuser, std::vector<std::string>& param
 		// Channel METADATA has an additional parameter: the channel TS
 		// :22D METADATA #channel 12345 extname :extdata
 		if (params.size() < 3)
-			return CMD_INVALID;
+			throw ProtocolException("Insufficient parameters for channel METADATA");
 
 		Channel* c = ServerInstance->FindChan(params[0]);
 		if (!c)
diff --git a/src/modules/m_spanningtree/nick.cpp b/src/modules/m_spanningtree/nick.cpp
index 49ce9a767..733901632 100644
--- a/src/modules/m_spanningtree/nick.cpp
+++ b/src/modules/m_spanningtree/nick.cpp
@@ -33,7 +33,7 @@
 CmdResult CommandNick::HandleRemote(RemoteUser* user, std::vector<std::string>& params)
 {
 	if ((isdigit(params[0][0])) && (params[0] != user->uuid))
-		return CMD_INVALID;
+		throw ProtocolException("Attempted to change nick to an invalid or non-matching UUID");
 
 	/* Update timestamp on user when they change nicks */
 	const time_t newts = ConvToInt(params[1]);
diff --git a/src/modules/m_spanningtree/servercommand.h b/src/modules/m_spanningtree/servercommand.h
index 156b405e4..f99942079 100644
--- a/src/modules/m_spanningtree/servercommand.h
+++ b/src/modules/m_spanningtree/servercommand.h
@@ -22,6 +22,15 @@
 #include "utils.h"
 #include "treeserver.h"
 
+class ProtocolException : public ModuleException
+{
+ public:
+	ProtocolException(const std::string& msg)
+		: ModuleException("Protocol violation: " + msg)
+	{
+	}
+};
+
 /** Base class for server-to-server commands that may have a (remote) user source or server source.
  */
 class ServerCommand : public CommandBase
@@ -47,7 +56,7 @@ class UserOnlyServerCommand : public ServerCommand
 	{
 		RemoteUser* remoteuser = IS_REMOTE(user);
 		if (!remoteuser)
-			return CMD_INVALID;
+			throw ProtocolException("Invalid source");
 		return static_cast<T*>(this)->HandleRemote(remoteuser, parameters);
 	}
 };
@@ -65,7 +74,7 @@ class ServerOnlyServerCommand : public ServerCommand
 	CmdResult Handle(User* user, std::vector<std::string>& parameters)
 	{
 		if (!IS_SERVER(user))
-			return CMD_INVALID;
+			throw ProtocolException("Invalid source");
 		TreeServer* server = TreeServer::Get(user);
 		return static_cast<T*>(this)->HandleServer(server, parameters);
 	}
diff --git a/src/modules/m_spanningtree/treesocket1.cpp b/src/modules/m_spanningtree/treesocket1.cpp
index 9da06e829..931bd3f9f 100644
--- a/src/modules/m_spanningtree/treesocket1.cpp
+++ b/src/modules/m_spanningtree/treesocket1.cpp
@@ -251,7 +251,18 @@ void TreeSocket::OnDataReady()
 			SendError("Read null character from socket");
 			break;
 		}
-		ProcessLine(line);
+
+		try
+		{
+			ProcessLine(line);
+		}
+		catch (CoreException& ex)
+		{
+			ServerInstance->Logs->Log(MODNAME, LOG_DEFAULT, "Error while processing: " + line);
+			ServerInstance->Logs->Log(MODNAME, LOG_DEFAULT, ex.GetReason());
+			SendError(ex.GetReason() + " - check the log file for details");
+		}
+
 		if (!getError().empty())
 			break;
 	}
diff --git a/src/modules/m_spanningtree/treesocket2.cpp b/src/modules/m_spanningtree/treesocket2.cpp
index 4639a237e..8d939d22a 100644
--- a/src/modules/m_spanningtree/treesocket2.cpp
+++ b/src/modules/m_spanningtree/treesocket2.cpp
@@ -313,21 +313,13 @@ void TreeSocket::ProcessConnectedLine(std::string& prefix, std::string& command,
 				return;
 			}
 
-			ServerInstance->Logs->Log(MODNAME, LOG_SPARSE, "Unrecognised S2S command :%s %s %s",
-				who->uuid.c_str(), command.c_str(), irc::stringjoiner(params).c_str());
-			SendError("Unrecognised command '" + command + "' -- possibly loaded mismatched modules");
-			return;
+			throw ProtocolException("Unknown command");
 		}
 		cmdbase = cmd;
 	}
 
 	if (params.size() < cmdbase->min_params)
-	{
-		ServerInstance->Logs->Log(MODNAME, LOG_SPARSE, "Insufficient parameters for S2S command :%s %s %s",
-			who->uuid.c_str(), command.c_str(), irc::stringjoiner(params).c_str());
-		SendError("Insufficient parameters for command '" + command + "'");
-		return;
-	}
+		throw ProtocolException("Insufficient parameters");
 
 	if ((!params.empty()) && (params.back().empty()) && (!cmdbase->allow_empty_last_param))
 	{
@@ -341,15 +333,13 @@ void TreeSocket::ProcessConnectedLine(std::string& prefix, std::string& command,
 	if (scmd)
 		res = scmd->Handle(who, params);
 	else
-		res = cmd->Handle(params, who);
-
-	if (res == CMD_INVALID)
 	{
-		ServerInstance->Logs->Log(MODNAME, LOG_SPARSE, "Error handling S2S command :%s %s %s",
-			who->uuid.c_str(), command.c_str(), irc::stringjoiner(params).c_str());
-		SendError("Error handling '" + command + "' -- possibly loaded mismatched modules");
+		res = cmd->Handle(params, who);
+		if (res == CMD_INVALID)
+			throw ProtocolException("Error in command handler");
 	}
-	else if (res == CMD_SUCCESS)
+
+	if (res == CMD_SUCCESS)
 		Utils->RouteCommand(route_back_again, cmdbase, params, who);
 }
 
diff --git a/src/modules/m_spanningtree/uid.cpp b/src/modules/m_spanningtree/uid.cpp
index 79bd6936f..206f79a48 100644
--- a/src/modules/m_spanningtree/uid.cpp
+++ b/src/modules/m_spanningtree/uid.cpp
@@ -38,14 +38,14 @@ CmdResult CommandUID::HandleServer(TreeServer* remoteserver, std::vector<std::st
 
 	/* Is this a valid UID, and not misrouted? */
 	if (params[0].length() != UIDGenerator::UUID_LENGTH || params[0].substr(0, 3) != remoteserver->GetID())
-		return CMD_INVALID;
+		throw ProtocolException("Bogus UUID");
 	/* Check parameters for validity before introducing the client, discovered by dmb */
 	if (!age_t)
 		return CMD_INVALID;
 	if (!signon)
 		return CMD_INVALID;
 	if (modestr[0] != '+')
-		return CMD_INVALID;
+		throw ProtocolException("Invalid mode string");
 
 	/* check for collision */
 	User* collideswith = ServerInstance->FindNickOnly(params[2]);
@@ -66,17 +66,10 @@ CmdResult CommandUID::HandleServer(TreeServer* remoteserver, std::vector<std::st
 
 	/* IMPORTANT NOTE: For remote users, we pass the UUID in the constructor. This automatically
 	 * sets it up in the UUID hash for us.
+	 *
+	 * If the UUID already exists User::User() throws an exception which causes this connection to be closed.
 	 */
-	User* _new = NULL;
-	try
-	{
-		_new = new RemoteUser(params[0], remoteserver);
-	}
-	catch (...)
-	{
-		ServerInstance->Logs->Log(MODNAME, LOG_DEFAULT, "Duplicate UUID %s in client introduction", params[0].c_str());
-		return CMD_INVALID;
-	}
+	RemoteUser* _new = new RemoteUser(params[0], remoteserver);
 	ServerInstance->Users->clientlist[params[2]] = _new;
 	_new->nick = params[2];
 	_new->host = params[3];
@@ -98,15 +91,12 @@ CmdResult CommandUID::HandleServer(TreeServer* remoteserver, std::vector<std::st
 		/* For each mode thats set, find the mode handler and set it on the new user */
 		ModeHandler* mh = ServerInstance->Modes->FindMode(*v, MODETYPE_USER);
 		if (!mh)
-		{
-			ServerInstance->Logs->Log(MODNAME, LOG_DEFAULT, "Unrecognised mode '%c' for a user in UID, dropping link", *v);
-			return CMD_INVALID;
-		}
+			throw ProtocolException("Unrecognised mode '" + std::string(1, *v) + "'");
 
 		if (mh->GetNumParams(true))
 		{
 			if (paramptr >= params.size() - 1)
-				return CMD_INVALID;
+				throw ProtocolException("Out of parameters while processing modes");
 			std::string mp = params[paramptr++];
 			/* IMPORTANT NOTE:
 			 * All modes are assumed to succeed here as they are being set by a remote server.