Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: login crash and memory corruption #2951

Merged
merged 16 commits into from
Oct 9, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 0 additions & 4 deletions src/game/game.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1012,7 +1012,6 @@ std::shared_ptr<Player> Game::getPlayerByGUID(const uint32_t &guid, bool allowOf
if (!allowOffline) {
return nullptr;
}

std::shared_ptr<Player> tmpPlayer = std::make_shared<Player>(nullptr);
if (!IOLoginData::loadPlayerById(tmpPlayer, guid)) {
return nullptr;
Expand All @@ -1028,8 +1027,6 @@ std::string Game::getPlayerNameByGUID(const uint32_t &guid) {
if (m_playerNameCache.contains(guid)) {
return m_playerNameCache.at(guid);
}

// This player need read-only purposes and never saved
const auto &player = getPlayerByGUID(guid, true);
auto name = player ? player->getName() : "";
if (!name.empty()) {
Expand Down Expand Up @@ -1069,7 +1066,6 @@ std::vector<std::shared_ptr<Player>> Game::getPlayersByAccount(std::shared_ptr<A
if (error != enumToValue(AccountErrors_t::Ok)) {
return {};
}

std::vector<std::shared_ptr<Player>> ret;
for (const auto &[name, _] : accountPlayers) {
const auto &player = getPlayerByName(name, allowOffline);
Expand Down
3 changes: 0 additions & 3 deletions src/io/functions/iologindata_load_player.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -182,9 +182,6 @@ bool IOLoginDataLoad::loadPlayerBasicInfo(std::shared_ptr<Player> player, DBResu
player->setMaxManaShield(result->getNumber<uint32_t>("max_manashield"));

player->setMarriageSpouse(result->getNumber<int32_t>("marriage_spouse"));

// Experience load
IOLoginDataLoad::loadPlayerExperience(player, result);
return true;
}

Expand Down
18 changes: 13 additions & 5 deletions src/io/iologindata.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -92,21 +92,22 @@ void IOLoginData::updateOnlineStatus(uint32_t guid, bool login) {
Database::getInstance().executeQuery(query.str());
}

bool IOLoginData::loadPlayerById(std::shared_ptr<Player> player, uint32_t id) {
// The boolean "disableIrrelevantInfo" will deactivate the loading of information that is not relevant to the preload, for example, forge, bosstiary, etc. None of this we need to access if the player is offline
bool IOLoginData::loadPlayerById(std::shared_ptr<Player> player, uint32_t id, bool disableIrrelevantInfo /* = true*/) {
Database &db = Database::getInstance();
std::ostringstream query;
query << "SELECT * FROM `players` WHERE `id` = " << id;
return loadPlayer(player, db.storeQuery(query.str()));
return loadPlayer(player, db.storeQuery(query.str()), disableIrrelevantInfo);
}

bool IOLoginData::loadPlayerByName(std::shared_ptr<Player> player, const std::string &name) {
bool IOLoginData::loadPlayerByName(std::shared_ptr<Player> player, const std::string &name, bool disableIrrelevantInfo /* = true*/) {
Database &db = Database::getInstance();
std::ostringstream query;
query << "SELECT * FROM `players` WHERE `name` = " << db.escapeString(name);
return loadPlayer(player, db.storeQuery(query.str()));
return loadPlayer(player, db.storeQuery(query.str()), disableIrrelevantInfo);
}

bool IOLoginData::loadPlayer(std::shared_ptr<Player> player, DBResult_ptr result) {
bool IOLoginData::loadPlayer(std::shared_ptr<Player> player, DBResult_ptr result, bool disableIrrelevantInfo /* = false*/) {
if (!result || !player) {
std::string nullptrType = !result ? "Result" : "Player";
g_logger().warn("[{}] - {} is nullptr", __FUNCTION__, nullptrType);
Expand All @@ -117,6 +118,9 @@ bool IOLoginData::loadPlayer(std::shared_ptr<Player> player, DBResult_ptr result
// First
IOLoginDataLoad::loadPlayerBasicInfo(player, result);

// Experience load
IOLoginDataLoad::loadPlayerExperience(player, result);

// Blessings load
IOLoginDataLoad::loadPlayerBlessings(player, result);

Expand Down Expand Up @@ -174,6 +178,10 @@ bool IOLoginData::loadPlayer(std::shared_ptr<Player> player, DBResult_ptr result
// Load instant spells list
IOLoginDataLoad::loadPlayerInstantSpellList(player, result);

if (disableIrrelevantInfo) {
return true;
}

// load forge history
IOLoginDataLoad::loadPlayerForgeHistory(player, result);

Expand Down
6 changes: 3 additions & 3 deletions src/io/iologindata.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,9 @@ class IOLoginData {
static bool gameWorldAuthentication(const std::string &accountDescriptor, const std::string &sessionOrPassword, std::string &characterName, uint32_t &accountId, bool oldProcotol, const uint32_t ip);
static uint8_t getAccountType(uint32_t accountId);
static void updateOnlineStatus(uint32_t guid, bool login);
static bool loadPlayerById(std::shared_ptr<Player> player, uint32_t id);
static bool loadPlayerByName(std::shared_ptr<Player> player, const std::string &name);
static bool loadPlayer(std::shared_ptr<Player> player, DBResult_ptr result);
static bool loadPlayerById(std::shared_ptr<Player> player, uint32_t id, bool disableIrrelevantInfo = true);
static bool loadPlayerByName(std::shared_ptr<Player> player, const std::string &name, bool disableIrrelevantInfo = true);
static bool loadPlayer(std::shared_ptr<Player> player, DBResult_ptr result, bool disableIrrelevantInfo = false);
static bool savePlayer(std::shared_ptr<Player> player);
static uint32_t getGuidByName(const std::string &name);
static bool getGuidByNameEx(uint32_t &guid, bool &specialVip, std::string &name);
Expand Down
19 changes: 14 additions & 5 deletions src/items/containers/container.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -61,12 +61,21 @@ std::shared_ptr<Container> Container::create(std::shared_ptr<Tile> tile) {

Container::~Container() {
if (getID() == ITEM_BROWSEFIELD) {
if (getParent() && getParent()->getTile()) {
g_game().browseFields.erase(getParent()->getTile());
}
auto parent = getParent();
if (parent) {
auto tile = parent->getTile();
if (tile) {
auto browseField = g_game().browseFields.find(tile);
if (browseField != g_game().browseFields.end()) {
g_game().browseFields.erase(browseField);
}
}

for (std::shared_ptr<Item> item : itemlist) {
item->setParent(getParent());
for (auto &item : itemlist) {
if (item) {
item->setParent(parent);
}
}
}
}
}
Expand Down
31 changes: 22 additions & 9 deletions src/security/argon.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -25,17 +25,30 @@ void Argon2::updateConstants() {
}

uint32_t Argon2::parseBitShift(const std::string &bitShiftStr) const {
std::stringstream ss(bitShiftStr);
int base;
int shift;
char op1;
char op2;

if (!(ss >> base >> op1 >> op2 >> shift) || op1 != '<' || op2 != '<') {
g_logger().warn("Invalid bit shift string");
static const std::regex pattern(R"(^\s*(\d+)\s*<<\s*(\d+)\s*$)");
std::smatch match;

if (!std::regex_match(bitShiftStr, match, pattern)) {
g_logger().warn("Invalid bit shift string format: '{}'", bitShiftStr);
return 0;
}

int base = 0;
int shift = 0;
try {
base = std::stoi(match[1].str());
shift = std::stoi(match[2].str());
} catch (const std::exception &e) {
g_logger().warn("Error parsing bit shift string: '{}'", e.what());
return 0;
}

if (shift < 0 || shift >= 32) {
g_logger().warn("Shift value out of bounds: '{}'", shift);
return 0;
}

return base << shift;
return static_cast<uint32_t>(base) << shift;
}

bool Argon2::verifyPassword(const std::string &password, const std::string &phash) const {
Expand Down
8 changes: 4 additions & 4 deletions src/server/network/message/networkmessage.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -44,24 +44,24 @@ int32_t NetworkMessage::decodeHeader() {
}

// Simply read functions for incoming message
uint8_t NetworkMessage::getByte() {
uint8_t NetworkMessage::getByte(const std::source_location &location /*= std::source_location::current()*/) {
// Check if there is at least 1 byte to read
if (!canRead(1)) {
g_logger().error("[{}] Not enough data to read a byte. Current position: {}, Length: {}", __FUNCTION__, info.position, info.length);
g_logger().error("[{}] Not enough data to read a byte. Current position: {}, Length: {}. Called line {}:{} in {}", __FUNCTION__, info.position, info.length, location.line(), location.column(), location.function_name());
return {};
}

// Ensure that position is within bounds before decrementing
if (info.position == 0) {
g_logger().error("[{}] Position is at the beginning of the buffer. Cannot decrement.", __FUNCTION__);
g_logger().error("[{}] Position is at the beginning of the buffer. Cannot decrement. Called line {}:{} in {}", __FUNCTION__, location.line(), location.column(), location.function_name());
return {};
}

try {
// Decrement position safely and return the byte
return buffer.at(info.position++);
} catch (const std::out_of_range &e) {
g_logger().error("[{}] Out of range error: {}. Position: {}, Buffer size: {}", __FUNCTION__, e.what(), info.position, buffer.size());
g_logger().error("[{}] Out of range error: {}. Position: {}, Buffer size: {}. Called line {}:{} in {}", __FUNCTION__, e.what(), info.position, buffer.size(), location.line(), location.column(), location.function_name());
}

return {};
Expand Down
2 changes: 1 addition & 1 deletion src/server/network/message/networkmessage.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ class NetworkMessage {
}

// simply read functions for incoming message
uint8_t getByte();
uint8_t getByte(const std::source_location &location = std::source_location::current());

uint8_t getPreviousByte();

Expand Down
2 changes: 1 addition & 1 deletion src/server/network/protocol/protocolgame.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -574,7 +574,7 @@ void ProtocolGame::login(const std::string &name, uint32_t accountId, OperatingS
return;
}

if (!IOLoginData::loadPlayerById(player, player->getGUID())) {
if (!IOLoginData::loadPlayerById(player, player->getGUID(), false)) {
g_game().removePlayerUniqueLogin(player);
disconnectClient("Your character could not be loaded.");
g_logger().warn("Player {} could not be loaded", player->getName());
Expand Down
Loading