ParticleSpawner (and maybe sound) IDs Reused

[Warr1024]

Minetest version

Minetest 5.8.0-dev-70261b8e9 (Linux)
Using Irrlicht 1.9.0mt13
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"

Active renderer

OpenGL 4.2

Irrlicht device

X11

Operating system and version

Debian 12

CPU model

Intel i5-3320M (4) @ 3.300GHz

GPU model

No response

OpenGL version

No response

Summary

Particle spawner IDs are aggressively reused, which can lead to deleting the wrong particle spawner.

It looks like the "ID" that MT returns for a particle spawner is not actually a unique identifier for that spawner, but an address (i.e. index of an array). So if I create particle spawner X and it gets assigned ID 3, then when I later call delete_particlespawner(3) I am not actually saying "delete particle spawner X", I'm unwittingly saying "delete whatever particle spawner currently occupies index 3". If X had already been deleted for some other reason (expiry, deletion by some other mod) then some other arbitrary particle spawner may now occupy that slot and be deleted instead.

The correct thing to do would be to use a unique value for each resource. A mapping of resource unique ID to address could be maintained in the engine.

Ideally the resource identifiers would be opaque to Lua (e.g. userdata) to prevent users from attempting to rely on the content/structure of the values, or to try to serialize and store them, or try to fabricate them.

Steps to reproduce

WARNING: Do NOT attempt to test this against NodeCore version 02673309-161dcfb0 or later, as there is already a workaround present that makes the bug unreproducible.

Install this mod code into a worldmod:

do
	local theta = 0
	local function fx()
		minetest.after(0.25, fx)

		-- singleplayer only
		local players = minetest.get_connected_players()
		local player = players[next(players)]
		if not player then return end

		local pos = player:get_pos()
		pos.y = pos.y + player:get_properties().eye_height
		local look = player:get_look_dir()
		local vert = vector.new(0, 1, 0)
		local horz = vector.cross(look, vert)
		vert = vector.cross(look, horz)
		theta = theta + math.pi / 8
		local targ = vector.add(pos, vector.multiply(look, 5))
		targ = vector.add(targ, vector.multiply(horz, math.sin(theta) * 3))
		targ = vector.add(targ, vector.multiply(vert, math.cos(theta) * 3))

		minetest.add_particlespawner({
				time = 1,
				amount = 50,
				minexptime = 0.5,
				maxexptime = 0.5,
				minpos = vector.offset(targ, -0.5, -0.5, -0.5),
				maxpos = vector.offset(targ, 0.5, 0.5, 0.5),
				minsize = 1,
				maxsize = 1,
				texture = "[combine:1x1^[noalpha",
			})
	end
	minetest.after(0, fx)
end

Stand still while looking out into open air, and observe that now a cloud of black particles will revolve around the center of the player's view. This cloud is "smooth" and has no obvious gaps.

Now, add this code to a worldmod (it doesn't matter if it's in the same or different mod):

do
	local oldid
	local function fx()
		minetest.after(math.random() * 2, fx)

		-- singleplayer only
		local players = minetest.get_connected_players()
		local player = players[next(players)]
		if not player then return end

		local pos = player:get_pos()
		pos.y = pos.y + player:get_properties().eye_height
		pos = vector.offset(pos, math.random() * 10 - 5, math.random() * 10 - 5,
			math.random() * 10 - 5)
		if oldid then minetest.delete_particlespawner(oldid) end
		oldid = minetest.add_particlespawner({
				time = 1,
				amount = 50,
				minexptime = 0.5,
				maxexptime = 0.5,
				minpos = vector.offset(pos, -0.5, -0.5, -0.5),
				maxpos = vector.offset(pos, 0.5, 0.5, 0.5),
				minsize = 1,
				maxsize = 1,
				texture = "[combine:1x1^[noalpha^[invert:rgb",
			})
	end
	minetest.after(0, fx)
end

This will add a cloud of white particles that randomly teleports around the player.

Note that the black particle cloud "clock" will now have visible gaps in it, even though:

• The black particle effect was never modified, nothing it created was publicly accessible, and it never used delete_particlespawner
• The white particle effect code never deleted any particlespawner ID that it wasn't given by the API.

Note that the gaps may be subtle and difficult to differentiate against just the randomness of particle distribution in the cloud, but you an definitely see the difference in smoothness if you watch for a while and compare against the case without the teleporting white clouds. Some of the black cloud particlespawners are getting deleted too early.

[Desour]

For sound ids, there was this: #12780 (Not merged.)

[sfan5]

Note that not reusing IDs doesn't get rid of the race condition, it just makes the window so big that it practically doesn't matter.
The actual issue is that these IDs "expire" without the Lua side having any way to tell.

[Warr1024]

Also if the IDs are expired, even if some part of the Lua side is told, there's no way to effectively guarantee that everywhere the ID ended up in the Lua side is clear about the reuse.

It's also somewhat unnecessary to require that the Lua side be responsible for dealing with this when it's easily dealt with centrally by either engine or builtin. If I attempt to delete a handle that's expired, even if I reasonably should have known that it did, there's no reason I need to be punished by having some other arbitrary resource deleted instead; it would be perfectly reasonable to just do nothing.

[Warr1024]

FYI the workaround for this I have in NodeCore is:

https://gitlab.com/sztest/nodecore/-/blob/dev/mods/nc_api/compat_issue14044.lua

I am posting this because:

• It demonstrates that (at least as far as I've seen so far) changing the data type of resource handles does not seem to break downstream mods.
• It could be dropped into builtin in very close to the form it's already in, as a quick and cheap option, or at least sets a minimum benchmark for what a "proper" fix should achieve.
• If we eventually DO change the engine behavior to use unique IDs for each resource, I'll need to know, in order to prevent the workaround from memory leaking, but I also can't remove the workaround because of the old versions I'll need to support, so I'll need some way to tell which situation I'm in.

I am probably going to have to put this into a couple of other games too (even if the fix gets implemented and merged very soon, it will still be months before it's officially released, and many more before I can EOS the old release) so knowing the shape that an engine-side fix would take would help. If we were going to encapsulate resource IDs as e.g. userdata types, that would make it a lot simpler for me to detect the fixed version and bypass the workaround cache.

[Wuzzy2]

[...] that would make it a lot simpler for me to detect the fixed version and bypass the workaround cache.

The easiest method to "detect" the fixed version is when the bugfix also adds a field to the minetest.features table.