Fix a rather nasty race condition revealed by my reading through the comments and enhancing them. Back when i did executeable includes, i placed a 'todo' in the code which said something along the lines of:

'we cant pass a User* into the config reader any more, because when the config reader thread finishes, that user may be gone and this will crash. Consider using an UID instead so that if the user vanishes, we can
detect this situation.' Of course, nobody ever did this, so i'm doing it now to ensure we dont come up against some particularly ugly race condition crashes!


git-svn-id: http://svn.inspircd.org/repository/trunk/inspircd@10569 e03df62e-2008-0410-955e-edbf42e46eb7

7 files changed, 84 insertions, 41 deletions

diff --git a/include/configreader.h b/include/configreader.h
index 1bd6766be..31551dedb 100644
--- a/include/configreader.h
+++ b/include/configreader.h
@@ -335,7 +335,7 @@ class CoreExport ServerConfig : public Extensible
 	/** User that is currently performing a rehash, needed because the
 	 * rehash code is now threaded and needs to know who to give errors and feedback to.
 	 */
-	User* RehashUser;
+	std::string RehashUserUID;
 
 	/** Rehash parameter, as above
 	 */
@@ -780,7 +780,7 @@ class CoreExport ServerConfig : public Extensible
 	 * and initialize this class. All other methods
 	 * should be used only by the core.
 	 */
-	void Read(bool bail, User* user);
+	void Read(bool bail, const std::string &useruid);
 
 	/** Read a file into a file_cache object
 	 */
@@ -796,7 +796,7 @@ class CoreExport ServerConfig : public Extensible
 	 * this user as SNOTICEs.
 	 * If the parameter is NULL, the messages are spooled to all opers.
 	 */
-	void ReportConfigError(const std::string &errormessage, bool bail, User* user);
+	void ReportConfigError(const std::string &errormessage, bool bail, const std::string &useruid);
 
 	/** Load 'filename' into 'target', with the new config parser everything is parsed into
 	 * tag/key/value at load-time rather than at read-value time.
diff --git a/include/inspircd.h b/include/inspircd.h
index 8e5a5b2a8..4e37fae8d 100644
--- a/include/inspircd.h
+++ b/include/inspircd.h
@@ -283,9 +283,9 @@ class CoreExport ConfigReaderThread : public Thread
 {
 	InspIRCd* ServerInstance;
 	bool do_bail;
-	User* TheUser;
+	std::string TheUserUID;
  public:
-	ConfigReaderThread(InspIRCd* Instance, bool bail, User* user) : Thread(), ServerInstance(Instance), do_bail(bail), TheUser(user)
+	ConfigReaderThread(InspIRCd* Instance, bool bail, const std::string &useruid) : Thread(), ServerInstance(Instance), do_bail(bail), TheUserUID(useruid)
 	{
 	}
 
diff --git a/src/commands/cmd_rehash.cpp b/src/commands/cmd_rehash.cpp
index 9a9384af5..ebbb7c9bd 100644
--- a/src/commands/cmd_rehash.cpp
+++ b/src/commands/cmd_rehash.cpp
@@ -64,10 +64,10 @@ CmdResult CommandRehash::Handle (const std::vector<std::string>& parameters, Use
 
 	if (!ServerInstance->ConfigThread)
 	{
-		ServerInstance->Config->RehashUser = user;
+		ServerInstance->Config->RehashUserUID = user->uuid;
 		ServerInstance->Config->RehashParameter = parameters.size() ? parameters[0] : "";
 
-		ServerInstance->ConfigThread = new ConfigReaderThread(ServerInstance, false, user);
+		ServerInstance->ConfigThread = new ConfigReaderThread(ServerInstance, false, ServerInstance->Config->RehashUserUID);
 		ServerInstance->Threads->Create(ServerInstance->ConfigThread);
 	}
 	else
diff --git a/src/configreader.cpp b/src/configreader.cpp
index 8f50e0762..0e373190b 100644
--- a/src/configreader.cpp
+++ b/src/configreader.cpp
@@ -701,7 +701,7 @@ bool DoneMaxBans(ServerConfig*, const char*)
 	return true;
 }
 
-void ServerConfig::ReportConfigError(const std::string &errormessage, bool bail, User* user)
+void ServerConfig::ReportConfigError(const std::string &errormessage, bool bail, const std::string &useruid)
 {
 	ServerInstance->Logs->Log("CONFIG",DEFAULT, "There were errors in your configuration file: %s", errormessage.c_str());
 	if (bail)
@@ -717,14 +717,18 @@ void ServerConfig::ReportConfigError(const std::string &errormessage, bool bail,
 		unsigned int prefixlen;
 		start = 0;
 		/* ":ServerInstance->Config->ServerName NOTICE user->nick :" */
-		if (user)
+		if (!useruid.empty())
 		{
-			prefixlen = strlen(this->ServerName) + user->nick.length() + 11;
-			user->WriteServ("NOTICE %s :There were errors in the configuration file:",user->nick.c_str());
-			while (start < errors.length())
+			User* user = ServerInstance->FindNick(useruid);
+			if (user)
 			{
-				user->WriteServ("NOTICE %s :%s",user->nick.c_str(), errors.substr(start, 510 - prefixlen).c_str());
-				start += 510 - prefixlen;
+				prefixlen = strlen(this->ServerName) + user->nick.length() + 11;
+				user->WriteServ("NOTICE %s :There were errors in the configuration file:",user->nick.c_str());
+				while (start < errors.length())
+				{
+					user->WriteServ("NOTICE %s :%s",user->nick.c_str(), errors.substr(start, 510 - prefixlen).c_str());
+					start += 510 - prefixlen;
+				}
 			}
 		}
 		else
@@ -740,7 +744,7 @@ void ServerConfig::ReportConfigError(const std::string &errormessage, bool bail,
 	}
 }
 
-void ServerConfig::Read(bool bail, User* user)
+void ServerConfig::Read(bool bail, const std::string &useruid)
 {
 	int rem = 0, add = 0;	   /* Number of modules added, number of modules removed */
 
@@ -942,7 +946,7 @@ void ServerConfig::Read(bool bail, User* user)
 
 	if (!this->DoInclude(newconfig, ServerInstance->ConfigFileName, errstr))
 	{
-		ReportConfigError(errstr.str(), bail, user);
+		ReportConfigError(errstr.str(), bail, useruid);
 		return;
 	}
 
@@ -1174,7 +1178,7 @@ void ServerConfig::Read(bool bail, User* user)
 
 	catch (CoreException &ce)
 	{
-		ReportConfigError(ce.GetReason(), bail, user);
+		ReportConfigError(ce.GetReason(), bail, useruid);
 		return;
 	}
 
@@ -1201,8 +1205,12 @@ void ServerConfig::Read(bool bail, User* user)
 			}
 			if (!foundclass)
 			{
-				if (user)
-					user->WriteServ("NOTICE %s :*** Warning: Oper type '%s' has a missing class named '%s', this does nothing!", user->nick.c_str(), item, classname.c_str());
+				if (!useruid.empty())
+				{
+					User* user = ServerInstance->FindNick(useruid);
+					if (user)
+						user->WriteServ("NOTICE %s :*** Warning: Oper type '%s' has a missing class named '%s', this does nothing!", user->nick.c_str(), item, classname.c_str());
+				}
 				else
 				{
 					if (bail)
@@ -1236,15 +1244,19 @@ void ServerConfig::Read(bool bail, User* user)
 		FailedPortList pl;
 		ServerInstance->BindPorts(false, found_ports, pl);
 
-		if (pl.size() && user)
+		if (pl.size() && !useruid.empty())
 		{
 			ServerInstance->Threads->Lock();
-			user->WriteServ("NOTICE %s :*** Not all your client ports could be bound.", user->nick.c_str());
-			user->WriteServ("NOTICE %s :*** The following port(s) failed to bind:", user->nick.c_str());
-			int j = 1;
-			for (FailedPortList::iterator i = pl.begin(); i != pl.end(); i++, j++)
+			User* user = ServerInstance->FindNick(useruid);
+			if (user)
 			{
-				user->WriteServ("NOTICE %s :*** %d.   Address: %s        Reason: %s", user->nick.c_str(), j, i->first.empty() ? "<all>" : i->first.c_str(), i->second.c_str());
+				user->WriteServ("NOTICE %s :*** Not all your client ports could be bound.", user->nick.c_str());
+				user->WriteServ("NOTICE %s :*** The following port(s) failed to bind:", user->nick.c_str());
+				int j = 1;
+				for (FailedPortList::iterator i = pl.begin(); i != pl.end(); i++, j++)
+				{
+					user->WriteServ("NOTICE %s :*** %d.   Address: %s        Reason: %s", user->nick.c_str(), j, i->first.empty() ? "<all>" : i->first.c_str(), i->second.c_str());
+				}
 			}
 			ServerInstance->Threads->Unlock();
 		}
@@ -1258,15 +1270,27 @@ void ServerConfig::Read(bool bail, User* user)
 				{
 					ServerInstance->SNO->WriteToSnoMask('A', "*** REHASH UNLOADED MODULE: %s",removing->c_str());
 
-					if (user)
-						user->WriteNumeric(RPL_UNLOADEDMODULE, "%s %s :Module %s successfully unloaded.",user->nick.c_str(), removing->c_str(), removing->c_str());
+					if (!useruid.empty())
+					{
+						User* user = ServerInstance->FindNick(useruid);
+						if (user)
+							user->WriteNumeric(RPL_UNLOADEDMODULE, "%s %s :Module %s successfully unloaded.",user->nick.c_str(), removing->c_str(), removing->c_str());
+					}
+					else
+						ServerInstance->SNO->WriteToSnoMask('A', "Module %s successfully unloaded.", removing->c_str());
 
 					rem++;
 				}
 				else
 				{
-					if (user)
-						user->WriteNumeric(ERR_CANTUNLOADMODULE, "%s %s :Failed to unload module %s: %s",user->nick.c_str(), removing->c_str(), removing->c_str(), ServerInstance->Modules->LastError().c_str());
+					if (!useruid.empty())
+					{
+						User* user = ServerInstance->FindNick(useruid);
+						if (user)
+							user->WriteNumeric(ERR_CANTUNLOADMODULE, "%s %s :Failed to unload module %s: %s",user->nick.c_str(), removing->c_str(), removing->c_str(), ServerInstance->Modules->LastError().c_str());
+					}
+					else
+						 ServerInstance->SNO->WriteToSnoMask('A', "Failed to unload module %s: %s", removing->c_str(), ServerInstance->Modules->LastError().c_str());
 				}
 			}
 		}
@@ -1278,16 +1302,27 @@ void ServerConfig::Read(bool bail, User* user)
 				if (ServerInstance->Modules->Load(adding->c_str()))
 				{
 					ServerInstance->SNO->WriteToSnoMask('A', "*** REHASH LOADED MODULE: %s",adding->c_str());
-
-					if (user)
-						user->WriteNumeric(RPL_LOADEDMODULE, "%s %s :Module %s successfully loaded.",user->nick.c_str(), adding->c_str(), adding->c_str());
+					if (!useruid.empty())
+					{
+						User* user = ServerInstance->FindNick(useruid);
+						if (user)
+							user->WriteNumeric(RPL_LOADEDMODULE, "%s %s :Module %s successfully loaded.",user->nick.c_str(), adding->c_str(), adding->c_str());
+					}
+					else
+						ServerInstance->SNO->WriteToSnoMask('A', "Module %s successfully loaded.", adding->c_str());
 
 					add++;
 				}
 				else
 				{
-					if (user)
-						user->WriteNumeric(ERR_CANTLOADMODULE, "%s %s :Failed to load module %s: %s",user->nick.c_str(), adding->c_str(), adding->c_str(), ServerInstance->Modules->LastError().c_str());
+					if (!useruid.empty())
+					{
+						User* user = ServerInstance->FindNick(useruid);
+						if (user)
+							user->WriteNumeric(ERR_CANTLOADMODULE, "%s %s :Failed to load module %s: %s",user->nick.c_str(), adding->c_str(), adding->c_str(), ServerInstance->Modules->LastError().c_str());
+					}
+					else
+						ServerInstance->SNO->WriteToSnoMask('A', "Failed to load module %s: %s", adding->c_str(), ServerInstance->Modules->LastError().c_str());
 				}
 			}
 		}
@@ -1302,13 +1337,20 @@ void ServerConfig::Read(bool bail, User* user)
 	{
 		/** Note: This is safe, the method checks for user == NULL */
 		ServerInstance->Threads->Lock();
+		User* user = NULL;
+		if (!useruid.empty())
+			user = ServerInstance->FindNick(useruid);
 		ServerInstance->Parser->SetupCommandTable(user);
 		ServerInstance->Threads->Unlock();
 	}
 	else
 	{
-		if (user)
-			user->WriteServ("NOTICE %s :*** Successfully rehashed server.", user->nick.c_str());
+		if (!useruid.empty())
+		{
+			User* user = ServerInstance->FindNick(useruid);
+			if (user)
+				user->WriteServ("NOTICE %s :*** Successfully rehashed server.", user->nick.c_str());
+		}
 		else
 			ServerInstance->SNO->WriteToSnoMask('A', "*** Successfully rehashed server.");
 	}
@@ -2331,7 +2373,7 @@ bool DoneELine(ServerConfig* conf, const char* tag)
 void ConfigReaderThread::Run()
 {
 	/* TODO: TheUser may be invalid by the time we get here! Check its validity, or pass a UID would be better */
-	ServerInstance->Config->Read(do_bail, TheUser);
+	ServerInstance->Config->Read(do_bail, TheUserUID);
 	ServerInstance->Threads->Lock();
 	this->SetExitFlag();
 	ServerInstance->Threads->Unlock();
diff --git a/src/inspircd.cpp b/src/inspircd.cpp
index 6ecf688e8..5cd2bab4a 100644
--- a/src/inspircd.cpp
+++ b/src/inspircd.cpp
@@ -774,7 +774,8 @@ int InspIRCd::Run()
 			this->Res->Rehash();
 			this->ResetMaxBans();
 			InitializeDisabledCommands(Config->DisabledCommands, this);
-			FOREACH_MOD_I(this, I_OnRehash, OnRehash(Config->RehashUser, Config->RehashParameter));
+			User* user = !Config->RehashUserUID.empty() ? FindNick(Config->RehashUserUID) : NULL;
+			FOREACH_MOD_I(this, I_OnRehash, OnRehash(user, Config->RehashParameter));
 			this->BuildISupport();
 		}
 
diff --git a/src/modules.cpp b/src/modules.cpp
index 06fcf76d5..9e0e1fae5 100644
--- a/src/modules.cpp
+++ b/src/modules.cpp
@@ -900,7 +900,7 @@ long ConfigReader::GetError()
 
 void ConfigReader::DumpErrors(bool bail, User* user)
 {
-	ServerInstance->Config->ReportConfigError(this->errorlog->str(), bail, user);
+	ServerInstance->Config->ReportConfigError(this->errorlog->str(), bail, user->uuid);
 }
 
 
diff --git a/src/server.cpp b/src/server.cpp
index d8ab76425..8879e1eb8 100644
--- a/src/server.cpp
+++ b/src/server.cpp
@@ -53,10 +53,10 @@ void RehashHandler::Call(const std::string &reason)
 	FOREACH_MOD_I(Server, I_OnGarbageCollect, OnGarbageCollect());
 	if (!Server->ConfigThread)
 	{
-		Server->Config->RehashUser = NULL;
+		Server->Config->RehashUserUID = "";
 		Server->Config->RehashParameter = "";
 
-		Server->ConfigThread = new ConfigReaderThread(Server, false, NULL);
+		Server->ConfigThread = new ConfigReaderThread(Server, false, "");
 		Server->Threads->Create(Server->ConfigThread);
 	}
 }