From a7e478780e33617fb9cc8dade9c35a3167b368e0 Mon Sep 17 00:00:00 2001 From: Martin Michelsen Date: Fri, 3 Nov 2023 23:10:07 -0700 Subject: [PATCH] deal with invalid 6x59 commands --- src/Lobby.cc | 4 ++++ src/Lobby.hh | 1 + src/Player.cc | 9 +++++---- src/ReceiveSubcommands.cc | 41 +++++++++++++++++++++++++++++++++++---- 4 files changed, 47 insertions(+), 8 deletions(-) diff --git a/src/Lobby.cc b/src/Lobby.cc index e7227e0e..9d8b19cc 100644 --- a/src/Lobby.cc +++ b/src/Lobby.cc @@ -322,6 +322,10 @@ bool Lobby::item_exists(uint32_t item_id) const { return this->item_id_to_floor_item.count(item_id); } +const Lobby::FloorItem& Lobby::find_item(uint32_t item_id) const { + return this->item_id_to_floor_item.at(item_id); +} + void Lobby::add_item(const ItemData& data, uint8_t area, float x, float z) { auto& fi = this->item_id_to_floor_item[data.id]; fi.data = data; diff --git a/src/Lobby.hh b/src/Lobby.hh index 0e2a0d34..535da3ae 100644 --- a/src/Lobby.hh +++ b/src/Lobby.hh @@ -173,6 +173,7 @@ struct Lobby : public std::enable_shared_from_this { uint64_t serial_number = 0); bool item_exists(uint32_t item_id) const; + const FloorItem& find_item(uint32_t item_id) const; void add_item(const ItemData& item, uint8_t area, float x, float z); ItemData remove_item(uint32_t item_id); uint32_t generate_item_id(uint8_t client_id); diff --git a/src/Player.cc b/src/Player.cc index c513b3d6..498b7802 100644 --- a/src/Player.cc +++ b/src/Player.cc @@ -338,10 +338,11 @@ void SavedPlayerDataBB::add_item(const ItemData& item) { // If we found an existing stack, add it to the total and return if (y < this->inventory.num_items) { - this->inventory.items[y].data.data1[5] += item.data1[5]; - if (this->inventory.items[y].data.data1[5] > combine_max) { - this->inventory.items[y].data.data1[5] = combine_max; + size_t new_stack_size = this->inventory.items[y].data.data1[5] + item.data1[5]; + if (new_stack_size > combine_max) { + throw out_of_range("stack is too large"); } + this->inventory.items[y].data.data1[5] = new_stack_size; return; } } @@ -349,7 +350,7 @@ void SavedPlayerDataBB::add_item(const ItemData& item) { // If we get here, then it's not meseta and not a combine item, so it needs to // go into an empty inventory slot if (this->inventory.num_items >= 30) { - throw runtime_error("inventory is full"); + throw out_of_range("inventory is full"); } auto& inv_item = this->inventory.items[this->inventory.num_items]; inv_item.present = 1; diff --git a/src/ReceiveSubcommands.cc b/src/ReceiveSubcommands.cc index a52392ed..878b5440 100644 --- a/src/ReceiveSubcommands.cc +++ b/src/ReceiveSubcommands.cc @@ -973,11 +973,44 @@ static void on_pick_up_item(shared_ptr c, uint8_t command, uint8_t flag, } if (l->check_flag(Lobby::Flag::ITEM_TRACKING_ENABLED)) { - auto effective_p = effective_c->game_data.player(); - auto item = l->remove_item(cmd.item_id); - effective_p->add_item(item); - auto s = c->require_server_state(); + auto effective_p = effective_c->game_data.player(); + + // It seems the client just plays it fast and loose with these commands. + // There can be multiple 6x5A (request to pick up item) commands in flight, + // and the leader will respond to *all* of them with 6x59 (pick up item), + // even if the item can't be picked up by the time the command is processed + // (e.g. if the player's inventory has become full due to a previous pick up + // request). In the case of a full inventory, the client is expected to just + // ignore the command; in the case of the floor item not existing, however, + // the client behaves strangely and eventually disconnects, so we let + // l->remove_item throw in that case instead to fail faster. + + // This might be a legitimate bug in the client: the logic for determining + // if an item can be picked up also applies to Meseta, and forbids the + // pickup if the client has 999999 on hand. However, some actions that + // affect Meseta aren't sent to other players (e.g. depositing it in the + // bank), so the game could get into a state where some players see a client + // as able to pick up a Meseta item and some see a client as unable to do + // so. The downstream result of this desynchronization is that if the + // affected player tries to pick up some Meseta, some clients will not allow + // the pickup and will not delete the floor item, so if someone else tries + // to pick it up again, they will disconnect. + + auto item = l->remove_item(cmd.item_id); + try { + effective_p->add_item(item); + } catch (const out_of_range& e) { + auto name = s->describe_item(c->version(), item, false); + l->log.warning("Player %hu attempted to pick up %08" PRIX32 " (%s) but cannot (%s); ignoring command", + cmd.header.client_id.load(), cmd.item_id.load(), name.c_str(), e.what()); + if (c->config.check_flag(Client::Flag::DEBUG_ENABLED)) { + auto name = s->describe_item(c->version(), item, true); + send_text_message_printf(c, "$C5PICK/F %08" PRIX32 "\n%s\n$C4%s", cmd.item_id.load(), name.c_str(), e.what()); + } + return; + } + auto name = s->describe_item(c->version(), item, false); l->log.info("Player %hu picked up %08" PRIX32 " (%s)", cmd.header.client_id.load(), cmd.item_id.load(), name.c_str());