Remove handshake timer on server sockets that die before completing handshake

This fixes some very subtle and hard-to-trace bugs that are triggered when a
file descriptor and memory address of an EventHandler* are reused after being
deallocated. Impossible to trigger in valgrind; has been seen in live networks.

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

4 files changed, 27 insertions, 21 deletions

diff --git a/src/modules/m_spanningtree/handshaketimer.cpp b/src/modules/m_spanningtree/handshaketimer.cpp
index ae41bad0a..14b27472c 100644
--- a/src/modules/m_spanningtree/handshaketimer.cpp
+++ b/src/modules/m_spanningtree/handshaketimer.cpp
@@ -32,28 +32,24 @@ HandshakeTimer::HandshakeTimer(InspIRCd* Inst, TreeSocket* s, Link* l, SpanningT
 	thefd = sock->GetFd();
 }
 
+HandshakeTimer::~HandshakeTimer()
+{
+	sock->hstimer = NULL;
+}
+
 void HandshakeTimer::Tick(time_t TIME)
 {
-	if (Instance->SE->GetRef(thefd) == sock)
+	if (!sock->GetHook())
+	{
+		CancelRepeat();
+		sock->SendCapabilities();
+	}
+	else if (BufferedSocketHSCompleteRequest(sock, (Module*)Utils->Creator, sock->GetHook()).Send())
 	{
-		if (!sock->GetHook())
-		{
-			CancelRepeat();
-			sock->SendCapabilities();
-		}
-		else
-		{
-			if (sock->GetHook() && BufferedSocketHSCompleteRequest(sock, (Module*)Utils->Creator, sock->GetHook()).Send())
-			{
-				CancelRepeat();
-				BufferedSocketAttachCertRequest(sock, (Module*)Utils->Creator, sock->GetHook()).Send();
-				sock->SendCapabilities();
-			}
-			else
-			{
-				// Try again later...
-			}
-		}
+		CancelRepeat();
+		BufferedSocketAttachCertRequest(sock, (Module*)Utils->Creator, sock->GetHook()).Send();
+		sock->SendCapabilities();
 	}
+	// otherwise, try again later
 }
 
diff --git a/src/modules/m_spanningtree/handshaketimer.h b/src/modules/m_spanningtree/handshaketimer.h
index 26379c8a6..0a60c78fc 100644
--- a/src/modules/m_spanningtree/handshaketimer.h
+++ b/src/modules/m_spanningtree/handshaketimer.h
@@ -31,6 +31,7 @@ class HandshakeTimer : public Timer
 	int thefd;
  public:
 	HandshakeTimer(InspIRCd* Inst, TreeSocket* s, Link* l, SpanningTreeUtilities* u, int delay);
+	~HandshakeTimer();
 	virtual void Tick(time_t TIME);
 };
 
diff --git a/src/modules/m_spanningtree/treesocket.h b/src/modules/m_spanningtree/treesocket.h
index 31a3d8cbc..d4def42bc 100644
--- a/src/modules/m_spanningtree/treesocket.h
+++ b/src/modules/m_spanningtree/treesocket.h
@@ -22,6 +22,7 @@
 #include "transport.h"
 
 #include "m_spanningtree/utils.h"
+#include "m_spanningtree/handshaketimer.h"
 
 /*
  * The server list in InspIRCd is maintained as two structures
@@ -90,6 +91,7 @@ class TreeSocket : public BufferedSocket
 	std::string OutboundPass;		/* Outbound password */
 	bool sentcapab;				/* Have sent CAPAB already */
  public:
+	HandshakeTimer* hstimer;		/* Handshake timer, needed to work around I/O hook buffering */
 
 	/** Because most of the I/O gubbins are encapsulated within
 	 * BufferedSocket, we just call the superclass constructor for
diff --git a/src/modules/m_spanningtree/treesocket1.cpp b/src/modules/m_spanningtree/treesocket1.cpp
index 12272d3b8..d0db01dec 100644
--- a/src/modules/m_spanningtree/treesocket1.cpp
+++ b/src/modules/m_spanningtree/treesocket1.cpp
@@ -46,6 +46,7 @@ TreeSocket::TreeSocket(SpanningTreeUtilities* Util, InspIRCd* SI, std::string sh
 	Utils->timeoutlist[this] = std::pair<std::string, int>(ServerName, maxtime);
 	if (Hook)
 		BufferedSocketHookRequest(this, (Module*)Utils->Creator, Hook).Send();
+	hstimer = NULL;
 }
 
 /** When a listening socket gives us a new file descriptor,
@@ -65,7 +66,8 @@ TreeSocket::TreeSocket(SpanningTreeUtilities* Util, InspIRCd* SI, int newfd, cha
 	if (Hook)
 		BufferedSocketHookRequest(this, (Module*)Utils->Creator, Hook).Send();
 
-	ServerInstance->Timers->AddTimer(new HandshakeTimer(ServerInstance, this, &(Utils->LinkBlocks[0]), this->Utils, 1));
+	hstimer = new HandshakeTimer(ServerInstance, this, &(Utils->LinkBlocks[0]), this->Utils, 1);
+	ServerInstance->Timers->AddTimer(hstimer);
 
 	/* Fix by Brain - inbound sockets need a timeout, too. 30 secs should be pleanty */
 	Utils->timeoutlist[this] = std::pair<std::string, int>("<unknown>", 30);
@@ -85,6 +87,8 @@ TreeSocket::~TreeSocket()
 {
 	if (Hook)
 		BufferedSocketUnhookRequest(this, (Module*)Utils->Creator, Hook).Send();
+	if (hstimer)
+		ServerInstance->Timers->DelTimer(hstimer);
 	Utils->timeoutlist.erase(this);
 }
 
@@ -115,7 +119,10 @@ bool TreeSocket::OnConnected()
 
 				/* found who we're supposed to be connecting to, send the neccessary gubbins. */
 				if (this->GetHook())
-					ServerInstance->Timers->AddTimer(new HandshakeTimer(ServerInstance, this, &(*x), this->Utils, 1));
+				{
+					hstimer = new HandshakeTimer(ServerInstance, this, &(*x), this->Utils, 1);
+					ServerInstance->Timers->AddTimer(hstimer);
+				}
 				else
 					this->SendCapabilities();