Inventory: prevent item loss when stacking oversized ItemStacks (#14072)

src/inventory.cpp

@@ -777,20 +777,18 @@ u32 InventoryList::moveItem(u32 i, InventoryList *dest, u32 dest_i,
 		return 0;
 
 	// Try to add the item to destination list
-	u32 oldcount = item1.count;
+	count = item1.count;
 	item1 = dest->addItem(dest_i, item1);
 
 	// If something is returned, the item was not fully added
 	if (!item1.empty()) {
-		// If olditem is returned, nothing was added.
-		bool nothing_added = (item1.count == oldcount);
+		bool nothing_added = (item1.count == count);
 
-		// If something else is returned, part of the item was left unadded.
-		// Add the other part back to the source item
-		addItem(i, item1);
+		// Add any leftover stack back to the source stack.
+		item1.add(getItem(i).count); // leftover + source count
+		changeItem(i, item1); // do NOT use addItem to allow oversized stacks!
 
-		// If olditem is returned, nothing was added.
-		// Swap the items
+		// Swap if no items could be moved
 		if (nothing_added && swap_if_needed) {
 			// Tell that we swapped
 			if (did_swap != NULL) {
@@ -802,9 +800,10 @@ u32 InventoryList::moveItem(u32 i, InventoryList *dest, u32 dest_i,
 			ItemStack item2 = dest->changeItem(dest_i, item1);
 			// Put item from destination list to the source list
 			changeItem(i, item2);
+			item1.clear(); // no leftover
 		}
 	}
-	return (oldcount - item1.count);
+	return (count - item1.count);
 }
 
 void InventoryList::checkResizeLock()

src/unittest/helpers/helper_moveaction.lua

@@ -1,4 +1,8 @@
 minetest.register_allow_player_inventory_action(function(player, action, inventory, info)
+	if action == "move" then
+		return info.count
+	end
+
 	if info.stack:get_name() == "default:water" then
 		return 0
 	end

src/unittest/test_moveaction.cpp

@@ -36,6 +36,7 @@ public:
 	void runTests(IGameDef *gamedef);
 
 	void testMove(ServerActiveObject *obj, IGameDef *gamedef);
+	void testMoveFillStack(ServerActiveObject *obj, IGameDef *gamedef);
 	void testMoveSomewhere(ServerActiveObject *obj, IGameDef *gamedef);
 	void testMoveUnallowed(ServerActiveObject *obj, IGameDef *gamedef);
 	void testMovePartial(ServerActiveObject *obj, IGameDef *gamedef);
@@ -51,14 +52,24 @@ void TestMoveAction::runTests(IGameDef *gamedef)
 	MockServer server;
 
 	ServerScripting server_scripting(&server);
-	server_scripting.loadMod(Server::getBuiltinLuaPath() + DIR_DELIM "init.lua", BUILTIN_MOD_NAME);
-	server_scripting.loadMod(std::string(HELPERS_PATH) + DIR_DELIM "helper_moveaction.lua",	BUILTIN_MOD_NAME);
+	try {
+		server_scripting.loadMod(Server::getBuiltinLuaPath() + DIR_DELIM "init.lua", BUILTIN_MOD_NAME);
+		server_scripting.loadMod(
+			std::string(HELPERS_PATH) + DIR_DELIM "helper_moveaction.lua", BUILTIN_MOD_NAME
+		);
+	} catch (ModError &e) {
+		// Print backtrace in case of syntax errors
+		rawstream << e.what() << std::endl;
+		num_tests_failed = 1;
+		return;
+	}
 
 	MetricsBackend mb;
 	ServerEnvironment server_env(nullptr, &server_scripting, &server, "", &mb);
 	MockServerActiveObject obj(&server_env);
 
 	TEST(testMove, &obj, gamedef);
+	TEST(testMoveFillStack, &obj, gamedef);
 	TEST(testMoveSomewhere, &obj, gamedef);
 	TEST(testMoveUnallowed, &obj, gamedef);
 	TEST(testMovePartial, &obj, gamedef);
@@ -95,6 +106,26 @@ void TestMoveAction::testMove(ServerActiveObject *obj, IGameDef *gamedef)
 	UASSERT(inv.p2.getList("main")->getItem(0).getItemString() == "default:stone 20");
 }
 
+void TestMoveAction::testMoveFillStack(ServerActiveObject *obj, IGameDef *gamedef)
+{
+	MockInventoryManager inv(gamedef);
+
+	auto list = inv.p1.addList("main", 10);
+	list->changeItem(0, parse_itemstack("default:stone 209"));
+	list->changeItem(1, parse_itemstack("default:stone 90")); // 9 free slots
+
+	apply_action("Move 209 player:p1 main 0 player:p1 main 1", &inv, obj, gamedef);
+
+	UASSERT(list->getItem(0).getItemString() == "default:stone 200");
+	UASSERT(list->getItem(1).getItemString() == "default:stone 99");
+
+	// Trigger stack swap
+	apply_action("Move 200 player:p1 main 0 player:p1 main 1", &inv, obj, gamedef);
+
+	UASSERT(list->getItem(0).getItemString() == "default:stone 99");
+	UASSERT(list->getItem(1).getItemString() == "default:stone 200");
+}
+
 void TestMoveAction::testMoveSomewhere(ServerActiveObject *obj, IGameDef *gamedef)
 {
 	MockInventoryManager inv(gamedef);