ITS FIXED! (i hope)

THE CRASH ON NETSPLIT BUG SHOULD NOW BE HISTORY!


git-svn-id: http://svn.inspircd.org/repository/branches/1_0_stable@3963 e03df62e-2008-0410-955e-edbf42e46eb7

1 files changed, 34 insertions, 2 deletions

diff --git a/src/modules/m_spanningtree.cpp b/src/modules/m_spanningtree.cpp
index 9d3d0e5d5..94f559ef5 100644
--- a/src/modules/m_spanningtree.cpp
+++ b/src/modules/m_spanningtree.cpp
@@ -170,6 +170,7 @@ class TreeServer
 	time_t NextPing;			/* After this time, the server should be PINGed*/
 	bool LastPingWasGood;			/* True if the server responded to the last PING with a PONG */
 	std::map<userrec*,userrec*> Users;	/* Users on this server */
+	bool DontModifyHash;			/* When the server is splitting, this is set to true so we dont bash our own iterator to death */
 	
  public:
 
@@ -184,6 +185,7 @@ class TreeServer
 		VersionString = "";
 		UserCount = OperCount = 0;
 		VersionString = Srv->GetVersion();
+		DontModifyHash = false;
 	}
 
 	/* We use this constructor only to create the 'root' item, TreeRoot, which
@@ -268,6 +270,9 @@ class TreeServer
 
 	void AddUser(userrec* user)
 	{
+		if (this->DontModifyHash)
+			return;
+
 		log(DEBUG,"Add user %s to server %s",user->nick,this->ServerName.c_str());
 		std::map<userrec*,userrec*>::iterator iter;
 		iter = Users.find(user);
@@ -277,6 +282,30 @@ class TreeServer
 
 	void DelUser(userrec* user)
 	{
+		/* FIX BY BRAIN:
+		 *
+		 * READ THIS, THIS MEANS YOU!
+		 *
+		 * This is the source of the 'servers crash on netsplit bug.
+		 * What happens (in theory) is this:
+		 *
+		 * When a server splits, QuitUsers (below) is called. When QuitUsers
+		 * is called, it iterates through this->Users and calls kill_link on
+		 * each one. We were under the impression this was safe, however it
+		 * was not because each call to kill_link ended up calling
+		 * SpanningTree::OnUserQuit, which would then... yes, you guess it...
+		 * go and remove the user from the hash in THIS function. So now, when
+		 * we are splitting, we set a value this->DontModifyHash, which tells
+		 * the object its hash isnt to be messed with right now, and skips over
+		 * the unsafe DelUser within this operation!
+		 *
+		 * Smart, or what? :-)
+		 *
+		 * - 24/05/06
+		 */
+		if (this->DontModifyHash)
+			return;
+
 		log(DEBUG,"Remove user %s from server %s",user->nick,this->ServerName.c_str());
 		std::map<userrec*,userrec*>::iterator iter;
 		iter = Users.find(user);
@@ -289,12 +318,15 @@ class TreeServer
 		int x = Users.size();
 		log(DEBUG,"Removing all users from server %s",this->ServerName.c_str());
 		const char* reason_s = reason.c_str();
+		this->DontModifyHash = true;
 		for (std::map<userrec*,userrec*>::iterator n = Users.begin(); n != Users.end(); n++)
 		{
-			log(DEBUG,"Kill %s",n->second->nick);
-			kill_link(n->second,reason_s);
+			log(DEBUG,"Kill %s fd=%d",n->second->nick,n->second->fd);
+			if (!IS_LOCAL(n->second))
+				kill_link(n->second,reason_s);
 		}
 		Users.clear();
+		this->DontModifyHash = false;
 		return x;
 	}