Fixed possible leak in AccountHandler::handleUnregisterMessage

Fixed by changing account instances to be managed by std::unique_ptr, so
we don't forget to delete them somewhere, like in that function as well
as during shutdown in AccountHandler.

1 files changed, 35 insertions, 32 deletions

diff --git a/src/account-server/accounthandler.cpp b/src/account-server/accounthandler.cpp
index 8f871455..856f07d0 100644
--- a/src/account-server/accounthandler.cpp
+++ b/src/account-server/accounthandler.cpp
@@ -102,7 +102,7 @@ private:
     /** List of all accounts which requested a random seed, but are not logged
      *  yet. This list will be regularly remove (after timeout) old accounts
      */
-    std::list<Account*> mPendingAccounts;
+    std::list<std::unique_ptr<Account>> mPendingAccounts;
 
     /** List of attributes that the client can send at account creation. */
     std::vector<int> mModifiableAttributes;
@@ -308,13 +308,14 @@ static void sendFullCharacterData(AccountClient *client,
 
 static std::string getRandomString(int length)
 {
-    char s[length];
+    std::string s;
+    s.resize(length);
     // No need to care about zeros. They can be handled.
     // But care for endianness
     for (int i = 0; i < length; ++i)
         s[i] = (char)rand();
 
-    return std::string(s, length);
+    return s;
 }
 
 void AccountHandler::handleLoginRandTriggerMessage(AccountClient &client, MessageIn &msg)
@@ -322,10 +323,10 @@ void AccountHandler::handleLoginRandTriggerMessage(AccountClient &client, Messag
     std::string salt = getRandomString(4);
     std::string username = msg.readString();
 
-    if (Account *acc = storage->getAccount(username))
+    if (auto acc = storage->getAccount(username))
     {
         acc->setRandomSalt(salt);
-        mPendingAccounts.push_back(acc);
+        mPendingAccounts.push_back(std::move(acc));
     }
     MessageOut reply(APMSG_LOGIN_RNDTRGR_RESPONSE);
     reply.writeString(salt);
@@ -389,17 +390,22 @@ void AccountHandler::handleLoginMessage(AccountClient &client, MessageIn &msg)
     }
 
     // Check if the account exists
-    Account *acc = nullptr;
-    for (Account *account : mPendingAccounts)
-        if (account->getName() == username)
-            acc = account;
-    mPendingAccounts.remove(acc);
+    auto accIt = std::find_if(mPendingAccounts.begin(),
+                              mPendingAccounts.end(),
+                              [&] (const std::unique_ptr<Account> &acc) {
+        return acc->getName() == username;
+    });
+
+    std::unique_ptr<Account> acc;
+    if (accIt != mPendingAccounts.end()) {
+        acc = std::move(*accIt);
+        mPendingAccounts.erase(accIt);
+    }
 
     if (!acc || sha256(acc->getPassword() + acc->getRandomSalt()) != password)
     {
         reply.writeInt8(ERRMSG_INVALID_ARGUMENT);
         client.send(reply);
-        delete acc;
         return;
     }
 
@@ -407,7 +413,6 @@ void AccountHandler::handleLoginMessage(AccountClient &client, MessageIn &msg)
     {
         reply.writeInt8(LOGIN_BANNED);
         client.send(reply);
-        delete acc;
         return;
     }
 
@@ -417,16 +422,12 @@ void AccountHandler::handleLoginMessage(AccountClient &client, MessageIn &msg)
     time_t login;
     time(&login);
     acc->setLastLogin(login);
-    storage->updateLastLogin(acc);
-
-    // Associate account with connection.
-    client.setAccount(acc);
-    client.status = CLIENT_CONNECTED;
+    storage->updateLastLogin(*acc);
 
     reply.writeInt8(ERRMSG_OK);
     addServerInfo(&reply);
 
-    Characters &chars = acc->getCharacters();
+    const Characters &chars = acc->getCharacters();
 
     if (client.version < 10) {
         client.send(reply);
@@ -436,6 +437,10 @@ void AccountHandler::handleLoginMessage(AccountClient &client, MessageIn &msg)
             sendCharacterData(reply, charIt.second);
         client.send(reply);
     }
+
+    // Associate account with connection.
+    client.setAccount(std::move(acc));
+    client.status = CLIENT_CONNECTED;
 }
 
 void AccountHandler::handleLogoutMessage(AccountClient &client)
@@ -529,7 +534,7 @@ void AccountHandler::handleRegisterMessage(AccountClient &client,
     }
     else
     {
-        Account *acc = new Account;
+        std::unique_ptr<Account> acc { new Account };
         acc->setName(username);
         acc->setPassword(sha256(password));
         // We hash email server-side for additional privacy
@@ -544,12 +549,12 @@ void AccountHandler::handleRegisterMessage(AccountClient &client,
         acc->setRegistrationDate(regdate);
         acc->setLastLogin(regdate);
 
-        storage->addAccount(acc);
+        storage->addAccount(*acc);
         reply.writeInt8(ERRMSG_OK);
         addServerInfo(&reply);
 
         // Associate account with connection
-        client.setAccount(acc);
+        client.setAccount(std::move(acc));
         client.status = CLIENT_CONNECTED;
     }
 
@@ -581,20 +586,19 @@ void AccountHandler::handleUnregisterMessage(AccountClient &client,
     }
 
     // See whether the account exists
-    Account *acc = storage->getAccount(username);
+    auto acc = storage->getAccount(username);
 
     if (!acc || acc->getPassword() != sha256(password))
     {
         reply.writeInt8(ERRMSG_INVALID_ARGUMENT);
         client.send(reply);
-        delete acc;
         return;
     }
 
     // Delete account and associated characters
     LOG_INFO("Unregistered \"" << username
              << "\", AccountID: " << acc->getID());
-    storage->delAccount(acc);
+    storage->delAccount(*acc);
     reply.writeInt8(ERRMSG_OK);
 
     client.send(reply);
@@ -654,7 +658,7 @@ void AccountHandler::handleEmailChangeMessage(AccountClient &client,
     {
         acc->setEmail(emailHash);
         // Keep the database up to date otherwise we will go out of sync
-        storage->flush(acc);
+        storage->flush(*acc);
         reply.writeInt8(ERRMSG_OK);
     }
     client.send(reply);
@@ -685,7 +689,7 @@ void AccountHandler::handlePasswordChangeMessage(AccountClient &client,
     {
         acc->setPassword(newPassword);
         // Keep the database up to date otherwise we will go out of sync
-        storage->flush(acc);
+        storage->flush(*acc);
         reply.writeInt8(ERRMSG_OK);
     }
 
@@ -822,7 +826,7 @@ void AccountHandler::handleCharacterCreateMessage(AccountClient &client,
             LOG_INFO("Character " << name << " was created for "
                      << acc->getName() << "'s account.");
 
-            storage->flush(acc); // flush changes
+            storage->flush(*acc); // flush changes
 
             // log transaction
             Transaction trans;
@@ -938,7 +942,7 @@ void AccountHandler::handleCharacterDeleteMessage(AccountClient &client,
         return;
     }
 
-    std::string characterName = chars[slot]->getName();
+    const std::string &characterName = chars[slot]->getName();
     LOG_INFO("Character deleted:" << characterName);
 
     // Log transaction
@@ -950,7 +954,7 @@ void AccountHandler::handleCharacterDeleteMessage(AccountClient &client,
     storage->addTransaction(trans);
 
     acc->delCharacter(slot);
-    storage->flush(acc);
+    storage->flush(*acc);
 
     reply.writeInt8(ERRMSG_OK);
     client.send(reply);
@@ -976,15 +980,14 @@ void AccountHandler::tokenMatched(AccountClient *client, int accountID)
     MessageOut reply(APMSG_RECONNECT_RESPONSE);
 
     // Associate account with connection.
-    Account *acc = storage->getAccount(accountID);
-    client->setAccount(acc);
+    client->setAccount(storage->getAccount(accountID));
     client->status = CLIENT_CONNECTED;
 
     reply.writeInt8(ERRMSG_OK);
     client->send(reply);
 
     // Return information about available characters
-    Characters &chars = acc->getCharacters();
+    const Characters &chars = client->getAccount()->getCharacters();
 
     // Send characters list
     sendFullCharacterData(client, chars);