set_list(..., {}) incorrectly forbidden in certain cases ("cannot be deleted or resized")

[grorp]

Minetest version

Minetest 5.8.0-dev-d197ff0f9 (Linux)
Using Irrlicht 1.9.0mt11
Using LuaJIT 2.1.0-beta3
BUILD_TYPE=Release
RUN_IN_PLACE=1
USE_CURL=1
USE_GETTEXT=1
USE_SOUND=1
STATIC_SHAREDIR="."
STATIC_LOCALEDIR="locale"

minetest/minetest_game@1e237b8

OS / Hardware

Operating system: Fedora Linux 37 (Workstation Edition)
CPU: Intel® Core™ i7-4790K × 8

Summary & Steps to reproduce

You can put an itemstack into MTG's creative inventory trash field (thus "deleting" the itemstack) by shift-clicking it.

Since 0fb6dba, if I try to put any itemstack into the trash by shift-click, Minetest crashes. This does not happen if I put items into the trash field "by hand".

AsyncErr: Lua: Runtime error from mod 'creative' in callback detached_inventory_OnPut(): InventoryList 'main' is currently in use and cannot be deleted or resized.
stack traceback:
	[C]: in function 'set_list'
	...n/bin/../games/minetest_game/mods/creative/inventory.lua:135: in function <...n/bin/../games/minetest_game/mods/creative/inventory.lua:134>

That this only happens since 0fb6dba (PR #13360) is why I'm opening the issue here rather than in the MTG repo.

[Desour]

Related: #13466

[SmallJoker]

This diff fixed the issue for me. Might it be possible for you to check whether there are any follow-up issues in your workflow?

diff --git a/src/inventorymanager.cpp b/src/inventorymanager.cpp
index 1c6b3ffc9..c3cf8d5bf 100644
--- a/src/inventorymanager.cpp
+++ b/src/inventorymanager.cpp
@@ -279,9 +279,6 @@ void IMoveAction::apply(InventoryManager *mgr, ServerActiveObject *player, IGame
 		return;
 	}
 
-	auto list_from_lock = list_from->resizeLock();
-	auto list_to_lock = list_to->resizeLock();
-
 	if (move_somewhere) {
 		s16 old_to_i = to_i;
 		u16 old_count = count;
@@ -326,6 +323,9 @@ void IMoveAction::apply(InventoryManager *mgr, ServerActiveObject *player, IGame
 		return;
 	}
 
+	auto list_from_lock = list_from->resizeLock();
+	auto list_to_lock = list_to->resizeLock();
+
 	if (from_i < 0 || list_from->getSize() <= (u32) from_i) {
 		infostream << "IMoveAction::apply(): FAIL: source index out of bounds: "
 			<< "size of from_list=\"" << list_from->getSize() << "\""

PS: To be technically correct, Minetest does not crash. It throws a Lua error. Crashes are unclean application terminations caused by e.g. segmentation faults.

[Desour]

diff

That would just disable the mitigation for this case, which is not good.
The correct way would be to re-look-up the invlists.

[SmallJoker]

@Desour The diff is valid because the function is called recursive. All script callbacks are still protected by the locks. The difference is that list_to_lock.release() now works correctly because there are no simultaneous double-locks.

[Desour]

All script callbacks are still protected by the locks.

The script callbacks don't need to be protected, but the borrowed references (i.e. list_to). Otherwise e.g. the list_to in list_to->getItem(dest_i) can be invalid.