your-land/bugtracker
your-land
/
bugtracker
44
20
Code Issues 2.4k Projects 2 Wiki

nil check on get_luaentity() #3601

New Issue
Open
opened 2023-01-23 17:46:48 +00:00 by AliasAlreadyTaken · 22 comments
AliasAlreadyTaken commented 2023-01-23 17:46:48 +00:00
Owner
Copy Link

According to flux and wsor, get_luaentity() can return nil. This
is neither documented in https://github.com/minetest/minetest/blob/master/doc/lua_api.txt#L7154 nor do a lot of mods check for nil at this place.

  • Question devs under which circumstances get_luaentity()
  • Go through all mentions of get_luaentity() and nil-proof it
  • PR our findings to the API doc, since get_luaentity() is very poorly documented
  • Maybe mention in modder forum of minetest?
  • PR to all upstream mods when we found their non nil-proofed get_luaentity()
According to flux and wsor, get_luaentity() can return nil. This is neither documented in https://github.com/minetest/minetest/blob/master/doc/lua_api.txt#L7154 nor do a lot of mods check for nil at this place. * [ ] Question devs under which circumstances get_luaentity() * [ ] Go through all mentions of get_luaentity() and nil-proof it * [ ] PR our findings to the API doc, since get_luaentity() is very poorly documented * [ ] Maybe mention in modder forum of minetest? * [ ] PR to all upstream mods when we found their non nil-proofed get_luaentity()
AliasAlreadyTaken commented 2023-01-23 17:48:20 +00:00
Author
Owner
Copy Link

Followup of #3597

Followup of #3597
AliasAlreadyTaken added the
1. kind/bug
2. prio/low
2. prio/good first issue
 labels 2023-01-23 17:48:38 +00:00
flux commented 2023-01-23 20:21:43 +00:00
Member
Copy Link

it returns nil generally when either

  1. the object is a player
  2. the object is a stale reference to something which was unloaded

i'm not certain under which conditions minetest.get_objects_inside_radius would return stale references - looking at the code, it should be filtering them out.

after a cursory review of the code, most of the places where get_luaentity() is used seem to be immediately after creating an object (minetest.add_entity()) - which can fail if the object failed to be created, which is actually a rare occurrence.

it returns `nil` generally when either 1. the object is a player 2. the object is a stale reference to something which was unloaded i'm not certain under which conditions `minetest.get_objects_inside_radius` would return stale references - looking at the code, [it should be filtering them out](https://github.com/minetest/minetest/blob/b8aaad4f1ef2e4b22cb7d47de9cb54068ccbbe18/src/script/lua_api/l_env.cpp#L711). after a cursory review of the code, most of the places where `get_luaentity()` is used seem to be immediately after creating an object (`minetest.add_entity()`) - which can fail if the object failed to be created, which is actually a rare occurrence.
AliasAlreadyTaken commented 2023-01-24 15:12:17 +00:00
Author
Owner
Copy Link

Happened again in a different piece of code: #3606

The servers it happened on were 5.7.0-dev. The live server which is 5.6.1 runs this piece of code flawlessly. Is it possible this is a 5.7.0+ problem?

The code at the specific location flux mentioned didn't change for 2 years, but maybe something else somewhere else did, that may have affected it?

Also I was told the maintainer of Mineclone experienced a similar problem here: https://git.minetest.land/MineClone2/MineClone2/pulls/3324/files

Happened again in a different piece of code: #3606 The servers it happened on were 5.7.0-dev. The live server which is 5.6.1 runs this piece of code flawlessly. Is it possible this is a 5.7.0+ problem? The code at the specific location flux mentioned didn't change for 2 years, but maybe something else somewhere else did, that may have affected it? Also I was told the maintainer of Mineclone experienced a similar problem here: https://git.minetest.land/MineClone2/MineClone2/pulls/3324/files
👀 1
flux commented 2023-01-24 17:05:12 +00:00
Member
Copy Link

The servers it happened on were 5.7.0-dev. The live server which is 5.6.1 runs this piece of code flawlessly. Is it possible this is a 5.7.0+ problem?

seems quite possible. which commit of 5.7.0-dev is the server?

> The servers it happened on were 5.7.0-dev. The live server which is 5.6.1 runs this piece of code flawlessly. Is it possible this is a 5.7.0+ problem? seems quite possible. which commit of 5.7.0-dev is the server?
AliasAlreadyTaken commented 2023-01-24 17:37:26 +00:00
Author
Owner
Copy Link

The servers it happened on were 5.7.0-dev. The live server which is 5.6.1 runs this piece of code flawlessly. Is it possible this is a 5.7.0+ problem?

seems quite possible. which commit of 5.7.0-dev is the server?

Test Server says it's on b8aaad4f1, but git says it can't find this commit?

Maybe it was reverted?

However since the Mineclone maintainer also found a similar issue, I'd guess the problem is in more than one commit.

> > The servers it happened on were 5.7.0-dev. The live server which is 5.6.1 runs this piece of code flawlessly. Is it possible this is a 5.7.0+ problem? > > seems quite possible. which commit of 5.7.0-dev is the server? Test Server says it's on b8aaad4f1, but git says it can't find this commit? Maybe it was reverted? However since the Mineclone maintainer also found a similar issue, I'd guess the problem is in more than one commit.
Bla commented 2023-01-24 17:40:47 +00:00
Copy Link

b8aaad4f1e

https://github.com/minetest/minetest/commit/b8aaad4f1ef2e4b22cb7d47de9cb54068ccbbe18
👍 1
flux commented 2023-01-24 20:33:49 +00:00
Member
Copy Link

the immediate common denominator seems to be use of get_objects_inside_radius and get_objects_in_area. assuming that anything which doesn't check whether the objects returned by those is a valid object, i've found the following issues:

the immediate common denominator seems to be use of `get_objects_inside_radius` and `get_objects_in_area`. assuming that anything which doesn't check whether the objects returned by those is a *valid* object, i've found the following issues: * [ ] [meseportals](https://gitea.your-land.de/your-land/minetest-meseportals/src/commit/49853f81e25f584f55fb08728636ce6a98b1afea/node_behaviors.lua#L163) * [ ] [mobs_redo](https://gitea.your-land.de/your-land/mobs_redo/src/commit/bbcdc4b67d0e431d6045614d53adb85266973456/api.lua#L1316) * [ ] [moremesecons](https://gitea.your-land.de/your-land/MoreMesecons/src/commit/13645134a6015c7a38330f8850dde20b8909bece/moremesecons_entity_detector/init.lua#L46) * [ ] [petz](https://gitea.your-land.de/your-land/petz/src/commit/1770d622be0a10e1ae926f2cb4554f43e7b50c7c/kitz/api/api_clear_mobs.lua#L3) * [x] [smartshop](https://github.com/fluxionary/minetest-smartshop/commit/23000ff87fe0f97b62721c2ac8d4bc585e0d9612) * [ ] [spears](https://gitea.your-land.de/your-land/spears/src/commit/01e2bb97213381b47c7633523d923a77f3a7c1f8/functions.lua#L96) * [ ] [tnt](https://gitea.your-land.de/your-land/tnt/src/commit/9dc575d534cb2b72405c64b14090c81341de08f1/init.lua#L159) * [ ] [water_life](https://gitea.your-land.de/your-land/water_life/src/commit/d20a8b071fbea360fda8acff32ec1859e24ddeca/api.lua#L375) (herd members include anything that doesn't have a luaentity, like players or invalid objects...) * [ ] [yl_arena](https://gitea.your-land.de/your-land/yl_arena/src/commit/8afe3e7f2ba5003a02db654405ee389cd064f2ca/functions.lua#L277) * [ ] [yl_event](https://gitea.your-land.de/your-land/yl_events/src/commit/6e8f34ffa404f077a60b505b4825f225485edefb/temp.lua#L24) * [ ] [yl_monsterrace](https://gitea.your-land.de/your-land/yl_monsterrace/src/commit/72797da6d14d53d98ebb2262a56c5a26284f21b9/functions.lua#L157) (and several other uses) * [ ] [yl_nether_mobs](https://gitea.your-land.de/your-land/yl_nether_mobs/src/commit/8e2a4dce5f6b8540926d8f2c476e9507b132fe1f/wither.lua#L99)
flux commented 2023-01-24 20:36:29 +00:00
Member
Copy Link

b8aaad4f1e

i'll rebuild my local server and see if i can't shake anything out

> https://github.com/minetest/minetest/commit/b8aaad4f1ef2e4b22cb7d47de9cb54068ccbbe18 i'll rebuild my local server and see if i can't shake anything out
Bla commented 2023-01-24 20:58:43 +00:00
Copy Link

b8aaad4f1e

i'll rebuild my local server and see if i can't shake anything out

Not saying thats the culprit, just linked to the commit which is probably the one Alias git couldn´t find

> > https://github.com/minetest/minetest/commit/b8aaad4f1ef2e4b22cb7d47de9cb54068ccbbe18 > > i'll rebuild my local server and see if i can't shake anything out Not saying thats the culprit, just linked to the commit which is probably the one Alias git couldn´t find
flux commented 2023-01-24 22:08:10 +00:00
Member
Copy Link
local function override(get_objects)
    return function(...)
        local objects = get_objects(...)
        for i = 1, #objects do
            local obj = objects[i]
            if not (obj:is_player() or obj:get_luaentity()) then
                minetest.log("error", "invalid object?" .. dump({
                    pos = dump(obj:get_pos()),
                }))
            end
         end
         return objects
     end
 end
 
 minetest.get_objects_in_area = override(minetest.get_objects_in_area)
 minetest.get_objects_inside_radius = override(minetest.get_objects_inside_radius)
```lua local function override(get_objects) return function(...) local objects = get_objects(...) for i = 1, #objects do local obj = objects[i] if not (obj:is_player() or obj:get_luaentity()) then minetest.log("error", "invalid object?" .. dump({ pos = dump(obj:get_pos()), })) end end return objects end end minetest.get_objects_in_area = override(minetest.get_objects_in_area) minetest.get_objects_inside_radius = override(minetest.get_objects_inside_radius) ```
flux commented 2023-08-14 19:27:37 +00:00
Member
Copy Link

i added a fix to moremesecons in a PR for something else: https://github.com/minetest-mods/MoreMesecons/pull/32

i guess we never put that logging code in the actual yl codebase

i added a fix to moremesecons in a PR for something else: https://github.com/minetest-mods/MoreMesecons/pull/32 i guess we never put that logging code in the actual yl codebase
AliasAlreadyTaken commented 2023-08-14 22:23:17 +00:00
Author
Owner
Copy Link

i guess we never put that logging code in the actual yl codebase

Should we?

> i guess we never put that logging code in the actual yl codebase Should we?
flux commented 2023-08-15 16:25:43 +00:00
Member
Copy Link

i guess we never put that logging code in the actual yl codebase

Should we?

this issue hasn't actually occurred in quite a while, has it? maybe if it resurfaces.

> > i guess we never put that logging code in the actual yl codebase > > Should we? this issue hasn't actually occurred in quite a while, has it? maybe if it resurfaces.
👍 1
flux commented 2023-08-18 01:13:50 +00:00
Member
Copy Link

the MoreMesecons PR was accepted

the MoreMesecons PR was accepted
flux added the
4. step/partially fixed
 label 2023-08-18 01:14:04 +00:00
flux commented 2023-08-18 16:39:49 +00:00
Member
Copy Link

i guess we never put that logging code in the actual yl codebase

Should we?

this issue hasn't actually occurred in quite a while, has it? maybe if it resurfaces.

well, i guess i summoned the bug by mentioning it: #5155

i added a modified version of that code to yl_commons: a82eabdfb6

this version actually removes the "bad" objects from the return value so that other unsafe code won't crash, but note that it makes two of our most expensive functions even more expensive (see #3723). i should aim to patch all the other bad uses and then disable the "bugfix", but that's going to be a pain.

> > > i guess we never put that logging code in the actual yl codebase > > > > Should we? > > this issue hasn't actually occurred in quite a while, has it? maybe if it resurfaces. well, i guess i summoned the bug by mentioning it: #5155 i added a modified version of that code to yl_commons: https://gitea.your-land.de/your-land/yl_commons/commit/a82eabdfb6ff745546dca50fb9e18168373bef30 this version actually removes the "bad" objects from the return value so that other unsafe code won't crash, but note that it makes two of our most expensive functions even more expensive (see #3723). i should aim to patch all the other bad uses and then disable the "bugfix", but that's going to be a pain.
flux commented 2023-10-31 20:43:30 +00:00
Member
Copy Link

alias writes in #5253:

We have a couple of those in the log:

2023-09-09 11:12:17: ERROR[Server]: INVALID OBJECT?! {
	properties = {
		eye_height = 1.625,
		breath_max = 0,
		wield_item = "",
		infotext = "yl_speak_up:human",
		colors = {
			{
				a = 255,
				r = 255,
				g = 255,
				b = 255
			}
		},
		spritediv = {
			x = 1,
			y = 1
		},
		initial_sprite_basepos = {
			x = 0,
			y = 0
		},
		automatic_face_movement_dir = false,
		automatic_face_movement_max_rotation_per_sec = -1,
		backface_culling = true,
		damage_texture_modifier = "^[brighten",
		shaded = true,
		zoom_fov = 0,
		makes_footstep_sound = false,
		stepheight = 0,
		nametag_color = {
			a = 255,
			r = 255,
			g = 255,
			b = 255
		},
		nametag_bgcolor = false,
		static_save = true,
		show_on_minimap = false,
		mesh = "",
		physical = false,
		collide_with_objects = true,
		collisionbox = {
			-0.5,
			-0.5,
			-0.5,
			0.5,
			0.5,
			0.5
		},
		visual = "sprite",
		visual_size = {
			x = 1,
			y = 1,
			z = 1
		},
		use_texture_alpha = false,
		is_visible = true,
		selectionbox = {
			-0.5,
			-0.5,
			-0.5,
			0.5,
			0.5,
			0.5,
			rotate = false
		},
		pointable = true,
		hp_max = 1,
		automatic_rotate = 0,
		glow = 0,
		nametag = "",
		textures = {
			"unknown_object.png"
		}
	},
	hp = 100,
	is_player = false,
	pos = "(2003,14.5,1172)",
	entity_name = "yl_speak_up:human",
	nametag_attributes = {
		text = "",
		color = {
			a = 255,
			r = 255,
			g = 255,
			b = 255
		},
		bgcolor = false
	}
}
alias writes in #5253: We have a couple of those in the log: ``` 2023-09-09 11:12:17: ERROR[Server]: INVALID OBJECT?! { properties = { eye_height = 1.625, breath_max = 0, wield_item = "", infotext = "yl_speak_up:human", colors = { { a = 255, r = 255, g = 255, b = 255 } }, spritediv = { x = 1, y = 1 }, initial_sprite_basepos = { x = 0, y = 0 }, automatic_face_movement_dir = false, automatic_face_movement_max_rotation_per_sec = -1, backface_culling = true, damage_texture_modifier = "^[brighten", shaded = true, zoom_fov = 0, makes_footstep_sound = false, stepheight = 0, nametag_color = { a = 255, r = 255, g = 255, b = 255 }, nametag_bgcolor = false, static_save = true, show_on_minimap = false, mesh = "", physical = false, collide_with_objects = true, collisionbox = { -0.5, -0.5, -0.5, 0.5, 0.5, 0.5 }, visual = "sprite", visual_size = { x = 1, y = 1, z = 1 }, use_texture_alpha = false, is_visible = true, selectionbox = { -0.5, -0.5, -0.5, 0.5, 0.5, 0.5, rotate = false }, pointable = true, hp_max = 1, automatic_rotate = 0, glow = 0, nametag = "", textures = { "unknown_object.png" } }, hp = 100, is_player = false, pos = "(2003,14.5,1172)", entity_name = "yl_speak_up:human", nametag_attributes = { text = "", color = { a = 255, r = 255, g = 255, b = 255 }, bgcolor = false } } ```
flux commented 2023-10-31 20:54:59 +00:00
Member
Copy Link
	pos = "(2003,14.5,1172)",
	entity_name = "yl_speak_up:human",

this is Amanda.

i added some code to add a stack trace to see if there's any pattern b27043c302

``` pos = "(2003,14.5,1172)", entity_name = "yl_speak_up:human", ``` this is Amanda. i added some code to add a stack trace to see if there's any pattern https://gitea.your-land.de/your-land/yl_commons/commit/b27043c302147f05d31dc06979270dbb5d9c8c67
AliasAlreadyTaken commented 2023-12-16 01:33:01 +00:00
Author
Owner
Copy Link

2023-12-14 23:47:58: ERROR[Server]: LuaEntity name "water_life:crocodile" not defined
2023-12-14 23:48:00: ERROR[Server]: INVALID OBJECT?! {
2023-12-14 23:48:00: ERROR[Server]: INVALID OBJECT?! {
2023-12-14 23:48:01: ERROR[Server]: INVALID OBJECT?! {

There occurred 90 incidents lately, unfortunately the debug code was not yet ingame.

2023-12-14 23:47:58: ERROR[Server]: LuaEntity name "water_life:crocodile" not defined 2023-12-14 23:48:00: ERROR[Server]: INVALID OBJECT?! { 2023-12-14 23:48:00: ERROR[Server]: INVALID OBJECT?! { 2023-12-14 23:48:01: ERROR[Server]: INVALID OBJECT?! { There occurred 90 incidents lately, unfortunately the debug code was not yet ingame.
flux commented 2023-12-16 23:40:40 +00:00
Member
Copy Link

2023-12-14 23:47:58: ERROR[Server]: LuaEntity name "water_life:crocodile" not defined

i'm pretty sure that was me trying to spawn one in to test something, not realizing that they name is "water_life:croc"

> 2023-12-14 23:47:58: ERROR[Server]: LuaEntity name "water_life:crocodile" not defined i'm pretty sure that was me trying to spawn one in to test something, not realizing that they name is "water_life:croc"
flux commented 2024-03-28 01:38:45 +00:00
Member
Copy Link

per #6575, the cause if this is probably almost always unknown objects.

i'm not sure why Amanda was implicated though, unless we started up the server w/out NPCs once, or in that intermediate era where they weren't aliased properly?

per #6575, the cause if this is probably almost always unknown objects. i'm not sure why Amanda was implicated though, unless we started up the server w/out NPCs once, or in that intermediate era where they weren't aliased properly?
AliasAlreadyTaken commented 2024-03-29 04:25:52 +00:00
Author
Owner
Copy Link

So the 100% repro is "unknown object" ? I'm fairly certain there was a brief time on main where we switched from one type of NPC to another, but they should never have been on for more than a minute or so, just in case I started the server by mistake. If NPCs were unknown objects, they're fairly easy to spot

So the 100% repro is "unknown object" ? I'm fairly certain there was a brief time on main where we switched from one type of NPC to another, but they should never have been on for more than a minute or so, just in case I started the server by mistake. If NPCs were unknown objects, they're fairly easy to spot
flux commented 2024-03-29 04:35:45 +00:00
Member
Copy Link

the 100% repro is "unknown object" ?

yes, not sure about the rest of it though. there might be another cause.

> the 100% repro is "unknown object" ? yes, not sure about the rest of it though. there might be another cause.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels   
1. kind/balancing

Depends on balancing

  
1. kind/breaking

something that alters or disables an existing game mechanic

  
1. kind/bug

Something is not working

  
1. kind/construction

A build needs fixing

  
1. kind/documentation

Needs proper documentation

  
1. kind/enhancement

New feature

  
1. kind/griefing

players despoiling builds or nature

  
1. kind/invalid

Not a valid report, e.g. created by mistake with no content.

  
1. kind/meme

issue is mostly a joke

  
1. kind/node limit

something which works for or against the node limit

  
1. kind/other

Something not within the other kinds

  
1. kind/protocol

something concerning how staff should react to player requests, or how players should treat each other

  
2. prio/controversial

not likely to happen w/out a lot of discussion

  
2. prio/critical

Very important

  
2. prio/elevated

something players are eager to have fixed

  
2. prio/good first issue

the resolution to this is could be done by a new minetest contributor, who doesn't yet understand the mod ecosystem

  
2. prio/interesting

one of us has real interest in getting this implemented or fixed. however, more research might be needed.

  
2. prio/low

Not so important

  
3. source/art

art needs to be created or found for this to exist. includes sound, 3d modelling, and in-game building.

  
3. source/client

Issue is with the minetest client, or with player client configuration or the technical details of their setup

  
3. source/engine

Issue is due to a bug or feature of the minetest engine

  
3. source/ingame

Problem and solution exist in-game, e.g. builds or player moderation

  
3. source/integration

Two or more nonbuggy mechanics conflict, or our attempts to rectify conflicting mechanics have created a new bug

  
3. source/lag

A problem which is caused or made worse by lag, due to server processing issues, client processing issues, or network issues

  
3. source/license

A licensing problem is involved

  
3. source/mod upstream

Problem is upstream (not engine/client)

  
3. source/unknown

   
3. source/website

Affects the website

  
4. step/approved

Alias has approved this feature, someone should implement it

  
4. step/at work

Being worked on

  
4. step/blocked

we plan to do this, but other work has to be finished first

  
4. step/discussion

issue is under discussion, a solution has not been dictated or agreed upon

  
4. step/help wanted

Need some help

  
4. step/needs confirmation

   
4. step/partially fixed

this is mitigated or partially solved, but more work needs to be done

  
4. step/question

More information is needed

  
4. step/ready to deploy

has been QA tested by another person. has no outstanding issues. might be installed on prod, but needs final testing

  
4. step/ready to QA test

the code is supposedly fixed, but needs to be tested.

  
4. step/want approval

Someone can quickly implement this if Alias approves the change/feature

  
5. result/cannot reproduce

   
5. result/duplicate

This issue or pull request already exists

  
5. result/fixed

problem is fixed

  
5. result/maybe

not planned currently, but maybe some day

  
5. result/wontfix

the reported issue won't be changed, whether it's intentional or not.

  
ugh/petz

yet another petz bugz.

  
ugh/QA main

That's something we can only wait and see whether it hapens again: On the main server

  
ugh/QA NOK

Fix didn't yield the expected results

  
ugh/QA OK

tested

No Label
1. kind/balancing
1. kind/breaking
1. kind/bug
1. kind/construction
1. kind/documentation
1. kind/enhancement
1. kind/griefing
1. kind/invalid
1. kind/meme
1. kind/node limit
1. kind/other
1. kind/protocol
2. prio/controversial
2. prio/critical
2. prio/elevated
2. prio/good first issue
2. prio/interesting
2. prio/low
3. source/art
3. source/client
3. source/engine
3. source/ingame
3. source/integration
3. source/lag
3. source/license
3. source/mod upstream
3. source/unknown
3. source/website
4. step/approved
4. step/at work
4. step/blocked
4. step/discussion
4. step/help wanted
4. step/needs confirmation
4. step/partially fixed
4. step/question
4. step/ready to deploy
4. step/ready to QA test
4. step/want approval
5. result/cannot reproduce
5. result/duplicate
5. result/fixed
5. result/maybe
5. result/wontfix
ugh/petz
ugh/QA main
ugh/QA NOK
ugh/QA OK
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
3 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: your-land/bugtracker#3601
No description provided.
Delete Branch "%!s(<nil>)"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?