From f8178e9bc8e983f007a46ab46a15fd870a04c0ec Mon Sep 17 00:00:00 2001 From: psychon Date: Wed, 22 Jul 2009 16:54:52 +0000 Subject: [PATCH] Fix a user-after-free bug When a user is deleted we didn't properly clean up after it. Active DCC connections (CDCCBounce and CDCCSock) where left laying around and at some later point of time they used their CUser* pointer which now pointed to invalid data. This bug is similar to the one from r1557. Thanks to cnu, our beloved master of destruction, for finding yet another way to make ZNC break and for testing the patch. git-svn-id: https://znc.svn.sourceforge.net/svnroot/znc/trunk@1575 726aef4b-f618-498e-8847-2d620e286838 --- DCCBounce.cpp | 13 +++++++++---- DCCSock.cpp | 42 ++++++++++++++++++++++++++++++++++++++---- DCCSock.h | 38 ++------------------------------------ User.cpp | 7 +++++++ User.h | 9 +++++++++ 5 files changed, 65 insertions(+), 44 deletions(-) diff --git a/DCCBounce.cpp b/DCCBounce.cpp index e07104a5..b2f43f7f 100644 --- a/DCCBounce.cpp +++ b/DCCBounce.cpp @@ -32,6 +32,8 @@ CDCCBounce::CDCCBounce(CUser* pUser, unsigned long uLongIP, unsigned short uPort if (bIsChat) { EnableReadLine(); } + + m_pUser->AddDCCBounce(this); } CDCCBounce::CDCCBounce(const CString& sHostname, unsigned short uPort, CUser* pUser, @@ -50,6 +52,8 @@ CDCCBounce::CDCCBounce(const CString& sHostname, unsigned short uPort, CUser* pU if (bIsChat) { EnableReadLine(); } + + m_pUser->AddDCCBounce(this); } CDCCBounce::~CDCCBounce() { @@ -57,10 +61,11 @@ CDCCBounce::~CDCCBounce() { m_pPeer->Shutdown(); m_pPeer = NULL; } - if (m_pUser) { - m_pUser->AddBytesRead(GetBytesRead()); - m_pUser->AddBytesWritten(GetBytesWritten()); - } + + m_pUser->AddBytesRead(GetBytesRead()); + m_pUser->AddBytesWritten(GetBytesWritten()); + + m_pUser->DelDCCBounce(this); } void CDCCBounce::ReadLine(const CString& sData) { diff --git a/DCCSock.cpp b/DCCSock.cpp index d3ecfaf5..0090c51a 100644 --- a/DCCSock.cpp +++ b/DCCSock.cpp @@ -10,15 +10,49 @@ #include "User.h" #include "Utils.h" +CDCCSock::CDCCSock(CUser* pUser, const CString& sRemoteNick, const CString& sLocalFile, const CString& sModuleName, + unsigned long uFileSize, CFile* pFile) : CZNCSock() { + m_sRemoteNick = sRemoteNick; + m_uFileSize = uFileSize; + m_uRemotePort = 0; + m_uBytesSoFar = 0; + m_pUser = pUser; + m_pFile = pFile; + m_sLocalFile = sLocalFile; + m_sModuleName = sModuleName; + m_bSend = true; + m_bNoDelFile = false; + + m_pUser->AddDCCSock(this); +} + +CDCCSock::CDCCSock(CUser* pUser, const CString& sRemoteNick, const CString& sRemoteIP, unsigned short uRemotePort, + const CString& sLocalFile, unsigned long uFileSize, const CString& sModuleName) : CZNCSock() { + m_sRemoteNick = sRemoteNick; + m_sRemoteIP = sRemoteIP; + m_uRemotePort = uRemotePort; + m_uFileSize = uFileSize; + m_uBytesSoFar = 0; + m_pUser = pUser; + m_pFile = NULL; + m_sLocalFile = sLocalFile; + m_sModuleName = sModuleName; + m_bSend = false; + m_bNoDelFile = false; + + m_pUser->AddDCCSock(this); +} + CDCCSock::~CDCCSock() { if ((m_pFile) && (!m_bNoDelFile)) { m_pFile->Close(); delete m_pFile; } - if (m_pUser) { - m_pUser->AddBytesRead(GetBytesRead()); - m_pUser->AddBytesWritten(GetBytesWritten()); - } + + m_pUser->AddBytesRead(GetBytesRead()); + m_pUser->AddBytesWritten(GetBytesWritten()); + + m_pUser->DelDCCSock(this); } void CDCCSock::ReadData(const char* data, int len) { diff --git a/DCCSock.h b/DCCSock.h index 0d16763f..0d7ea3f5 100644 --- a/DCCSock.h +++ b/DCCSock.h @@ -17,42 +17,8 @@ class CUser; class CDCCSock : public CZNCSock { public: - CDCCSock(CUser* pUser, const CString& sRemoteNick, const CString& sLocalFile, const CString& sModuleName, unsigned long uFileSize = 0, CFile* pFile = NULL) : CZNCSock() { - m_sRemoteNick = sRemoteNick; - m_uFileSize = uFileSize; - m_uRemotePort = 0; - m_uBytesSoFar = 0; - m_pUser = pUser; - m_pFile = pFile; - m_sLocalFile = sLocalFile; - m_sModuleName = sModuleName; - m_bSend = true; - m_bNoDelFile = false; - } - - CDCCSock(CUser* pUser, const CString& sRemoteNick, const CString& sRemoteIP, unsigned short uRemotePort, const CString& sLocalFile, unsigned long uFileSize, const CString& sModuleName) : CZNCSock() { - m_sRemoteNick = sRemoteNick; - m_sRemoteIP = sRemoteIP; - m_uRemotePort = uRemotePort; - m_uFileSize = uFileSize; - m_uBytesSoFar = 0; - m_pUser = pUser; - m_pFile = NULL; - m_sLocalFile = sLocalFile; - m_sModuleName = sModuleName; - m_bSend = false; - m_bNoDelFile = false; - } - -/* CDCCSock(CUser* pUser, const CString& sHostname, unsigned short uPort, int iTimeout = 60) : Csock(sHostname, uPort, iTimeout) { - m_uRemotePort = 0; - m_uBytesSoFar = 0; - m_uFileSize = 0; - m_pFile = NULL; - m_pUser = pUser; - m_bNoDelFile = false; - } -*/ + CDCCSock(CUser* pUser, const CString& sRemoteNick, const CString& sLocalFile, const CString& sModuleName, unsigned long uFileSize = 0, CFile* pFile = NULL); + CDCCSock(CUser* pUser, const CString& sRemoteNick, const CString& sRemoteIP, unsigned short uRemotePort, const CString& sLocalFile, unsigned long uFileSize, const CString& sModuleName); virtual ~CDCCSock(); virtual void ReadData(const char* data, int len); diff --git a/User.cpp b/User.cpp index 8c4bc754..084a6d2b 100644 --- a/User.cpp +++ b/User.cpp @@ -69,6 +69,13 @@ CUser::~CUser() { delete m_vChans[b]; } + // This will cause an endless loop if the destructor doesn't remove the + // socket from this list / if the socket doesn't exist any more. + while (!m_sDCCBounces.empty()) + CZNC::Get().GetManager().DelSockByAddr((CZNCSock*) *m_sDCCBounces.begin()); + while (!m_sDCCSocks.empty()) + CZNC::Get().GetManager().DelSockByAddr((CZNCSock*) *m_sDCCSocks.begin()); + CZNC::Get().GetManager().DelCronByAddr(m_pJoinTimer); CZNC::Get().GetManager().DelCronByAddr(m_pMiscTimer); } diff --git a/User.h b/User.h index 3c64c807..ab8ae0a7 100644 --- a/User.h +++ b/User.h @@ -27,6 +27,8 @@ class CIRCSock; class CJoinTimer; class CMiscTimer; class CServer; +class CDCCBounce; +class CDCCSock; class CUser { public: @@ -97,6 +99,11 @@ public: void IRCDisconnected(); void CheckIRCConnect(); + void AddDCCBounce(CDCCBounce* p) { m_sDCCBounces.insert(p); } + void DelDCCBounce(CDCCBounce* p) { m_sDCCBounces.erase(p); } + void AddDCCSock(CDCCSock* p) { m_sDCCSocks.insert(p); } + void DelDCCSock(CDCCSock* p) { m_sDCCSocks.erase(p); } + CString ExpandString(const CString& sStr) const; CString& ExpandString(const CString& sStr, CString& sRet) const; @@ -246,6 +253,8 @@ protected: vector m_vServers; vector m_vChans; vector m_vClients; + set m_sDCCBounces; + set m_sDCCSocks; set m_ssAllowedHosts; unsigned int m_uServerIdx; unsigned int m_uBufferCount;