From 80f9baf0a61b691b3f272d90455e0deb02f9612f Mon Sep 17 00:00:00 2001 From: Wolf480pl Date: Wed, 25 Jul 2018 12:08:15 +0200 Subject: [PATCH] Fix memory leak and null dereference in CZNC::LoadUsers Before this commit, when pUser->SetBeingDeleted(true) is executed, pUser is an empty unique_ptr, because release() was already called on it. Therefore, pUser->SetBeingDeleted is unidefined behaviour. Also, AddUser only takes ownership of the passed user pointer if it succeeds. In case of a failure, it's the caller's responsibility to delete the user. Fix this by keeping a raw pointer to the user, and handling it accordingly when AddUser fails. I have no idea whether SetBeingDeleted is necessary there, leaving it just in case. Maybe it would be better if we could change the semantics of AddUser to always take ownership of the pointer, or even take unique_ptr, but I have no idea how to adapt Python bindings in modpython to such change. --- src/znc.cpp | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/znc.cpp b/src/znc.cpp index 4e7216ee..b33491d5 100644 --- a/src/znc.cpp +++ b/src/znc.cpp @@ -1275,13 +1275,12 @@ bool CZNC::LoadUsers(CConfig& config, CString& sError) { } CString sErr; - if (!AddUser(pUser.release(), sErr, true)) { + CUser* pRawUser = pUser.release(); + if (!AddUser(pRawUser, sErr, true)) { sError = "Invalid user [" + sUserName + "] " + sErr; - } - - if (!sError.empty()) { CUtils::PrintError(sError); - pUser->SetBeingDeleted(true); + pRawUser->SetBeingDeleted(true); + delete pRawUser; return false; } }