deal with invalid 6x59 commands

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;

src/Lobby.hh

@@ -173,6 +173,7 @@ struct Lobby : public std::enable_shared_from_this<Lobby> {
       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);

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];
+      size_t new_stack_size = this->inventory.items[y].data.data1[5] + item.data1[5];
-      if (this->inventory.items[y].data.data1[5] > combine_max) {
+      if (new_stack_size > combine_max) {
-        this->inventory.items[y].data.data1[5] = 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;

src/ReceiveSubcommands.cc

@@ -973,11 +973,44 @@ static void on_pick_up_item(shared_ptr<Client> 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());