From 1d72306bae40c38a8a684cc412b0478fb0920d63 Mon Sep 17 00:00:00 2001 From: Tiger Wang Date: Sun, 3 Oct 2021 18:09:25 +0100 Subject: Authenticator: avoid move assignments to self (#5315) If authentication was off cClientHandle::m_Username ended up moved into itself. Add a copy to avoid this. Thanks @Seadragon91! --- src/ClientHandle.cpp | 74 ++++++++++++++++++++---------------------- src/Protocol/Authenticator.cpp | 22 ++++++++----- src/Protocol/Authenticator.h | 4 +-- 3 files changed, 52 insertions(+), 48 deletions(-) diff --git a/src/ClientHandle.cpp b/src/ClientHandle.cpp index efe4c9d48..4745ff7cd 100644 --- a/src/ClientHandle.cpp +++ b/src/ClientHandle.cpp @@ -335,49 +335,47 @@ bool cClientHandle::BungeeAuthenticate() void cClientHandle::Authenticate(AString && a_Name, const cUUID & a_UUID, Json::Value && a_Properties) { - { - cCSLock lock(m_CSState); - /* - LOGD("Processing authentication for client %s @ %s (%p), state = %d", - m_Username.c_str(), m_IPString.c_str(), static_cast(this), m_State.load() - ); - //*/ + cCSLock Lock(m_CSState); + /* + LOGD("Processing authentication for client %s @ %s (%p), state = %d", + m_Username.c_str(), m_IPString.c_str(), static_cast(this), m_State.load() + ); + //*/ - if (m_State != csAuthenticating) - { - return; - } + if (m_State != csAuthenticating) + { + return; + } - ASSERT(m_Player == nullptr); + ASSERT(m_Player == nullptr); - if (!BungeeAuthenticate()) - { - return; - } + if (!BungeeAuthenticate()) + { + return; + } - m_Username = std::move(a_Name); + m_Username = std::move(a_Name); - // Only assign UUID and properties if not already pre-assigned (BungeeCord sends those in the Handshake packet): - if (m_UUID.IsNil()) - { - m_UUID = a_UUID; - } - if (m_Properties.empty()) - { - m_Properties = std::move(a_Properties); - } + // Only assign UUID and properties if not already pre-assigned (BungeeCord sends those in the Handshake packet): + if (m_UUID.IsNil()) + { + m_UUID = a_UUID; + } + if (m_Properties.empty()) + { + m_Properties = std::move(a_Properties); + } - // Send login success (if the protocol supports it): - m_Protocol->SendLoginSuccess(); + // Send login success (if the protocol supports it): + m_Protocol->SendLoginSuccess(); - if (m_ForgeHandshake.IsForgeClient) - { - m_ForgeHandshake.BeginForgeHandshake(*this); - } - else - { - FinishAuthenticate(); - } + if (m_ForgeHandshake.IsForgeClient) + { + m_ForgeHandshake.BeginForgeHandshake(*this); + } + else + { + FinishAuthenticate(); } } @@ -745,8 +743,8 @@ bool cClientHandle::HandleLogin() m_State = csAuthenticating; } // lock(m_CSState) - // Schedule for authentication; until then, let the player wait (but do not block) - cRoot::Get()->GetAuthenticator().Authenticate(GetUniqueID(), std::move(m_Username), m_Protocol->GetAuthServerID()); + // Schedule for authentication; until then, let the player wait (but do not block): + cRoot::Get()->GetAuthenticator().Authenticate(GetUniqueID(), m_Username, m_Protocol->GetAuthServerID()); return true; } diff --git a/src/Protocol/Authenticator.cpp b/src/Protocol/Authenticator.cpp index 15a246f6e..b0669a354 100644 --- a/src/Protocol/Authenticator.cpp +++ b/src/Protocol/Authenticator.cpp @@ -57,17 +57,23 @@ void cAuthenticator::ReadSettings(cSettingsRepositoryInterface & a_Settings) -void cAuthenticator::Authenticate(int a_ClientID, AString && a_Username, const AString & a_ServerHash) +void cAuthenticator::Authenticate(int a_ClientID, const std::string_view a_Username, const std::string_view a_ServerHash) { if (!m_ShouldAuthenticate) { - const auto UUID = cClientHandle::GenerateOfflineUUID(a_Username); - cRoot::Get()->GetServer()->AuthenticateUser(a_ClientID, std::move(a_Username), UUID, Json::Value{}); + // An "authenticated" username, which is just what the client sent since auth is off. + std::string OfflineUsername(a_Username); + + // A specially constructed UUID based wholly on the username. + const auto OfflineUUID = cClientHandle::GenerateOfflineUUID(OfflineUsername); + + // "Authenticate" the user based on what little information we have: + cRoot::Get()->GetServer()->AuthenticateUser(a_ClientID, std::move(OfflineUsername), OfflineUUID, Json::Value()); return; } cCSLock Lock(m_CS); - m_Queue.emplace_back(a_ClientID, std::move(a_Username), a_ServerHash); + m_Queue.emplace_back(a_ClientID, a_Username, a_ServerHash); m_QueueNonempty.Set(); } @@ -114,17 +120,17 @@ void cAuthenticator::Execute(void) cAuthenticator::cUser User = std::move(m_Queue.front()); int & ClientID = User.m_ClientID; - AString & UserName = User.m_Name; + AString & Username = User.m_Name; AString & ServerID = User.m_ServerID; m_Queue.pop_front(); Lock.Unlock(); cUUID UUID; Json::Value Properties; - if (AuthWithYggdrasil(UserName, ServerID, UUID, Properties)) + if (AuthWithYggdrasil(Username, ServerID, UUID, Properties)) { - LOGINFO("User %s authenticated with UUID %s", UserName.c_str(), UUID.ToShortString().c_str()); - cRoot::Get()->GetServer()->AuthenticateUser(ClientID, std::move(UserName), UUID, std::move(Properties)); + LOGINFO("User %s authenticated with UUID %s", Username.c_str(), UUID.ToShortString().c_str()); + cRoot::Get()->GetServer()->AuthenticateUser(ClientID, std::move(Username), UUID, std::move(Properties)); } else { diff --git a/src/Protocol/Authenticator.h b/src/Protocol/Authenticator.h index 39a50ac47..66dcf20b4 100644 --- a/src/Protocol/Authenticator.h +++ b/src/Protocol/Authenticator.h @@ -41,7 +41,7 @@ public: void ReadSettings(cSettingsRepositoryInterface & a_Settings); /** Queues a request for authenticating a user. If the auth fails, the user will be kicked */ - void Authenticate(int a_ClientID, AString && a_Username, const AString & a_ServerHash); + void Authenticate(int a_ClientID, std::string_view a_Username, std::string_view a_ServerHash); /** Starts the authenticator thread. The thread may be started and stopped repeatedly */ void Start(cSettingsRepositoryInterface & a_Settings); @@ -58,7 +58,7 @@ private: AString m_Name; AString m_ServerID; - cUser(int a_ClientID, const AString & a_Name, const AString & a_ServerID) : + cUser(int a_ClientID, const std::string_view a_Name, const std::string_view a_ServerID) : m_ClientID(a_ClientID), m_Name(a_Name), m_ServerID(a_ServerID) -- cgit v1.2.3