Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix freeze in core.check_for_falling #13745

Merged
merged 2 commits into from Aug 27, 2023

Conversation

savilli
Copy link
Contributor

@savilli savilli commented Aug 21, 2023

core.add_entity(pos, "__builtin:falling_node") can fail if there are too many entities already. In this case, convert_to_falling_node

local obj = core.add_entity(pos, "__builtin:falling_node")
if not obj then
return false
returns false, and the falling node is not removed. But core.check_single_for_falling didn't forward the error
convert_to_falling_node(p, n)
return true
. Therefore core.check_for_falling tries to remove this node again
-- If we did need to walk the neighbor, then
-- start walking it from the walk order start (1),
-- and not the order we just pushed up the stack.
v = 1
and the server freezes in an infinite loop.

To do

This PR is Ready for Review.

How to test

  • Replace local obj = core.add_entity(pos, "__builtin:falling_node") with local obj = nil
  • Place a falling node, for example, a sand
  • Check that the server is not frozen

@wsor4035 wsor4035 added Bugfix 🐛 PRs that fix a bug @ Builtin labels Aug 21, 2023
@sfan5 sfan5 added the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Aug 27, 2023
@SmallJoker SmallJoker removed the Action / change needed Code still needs changes (PR) / more information requested (Issues) label Aug 27, 2023
Copy link
Member

@sfan5 sfan5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Copy link
Member

@SmallJoker SmallJoker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested briefly; seems to work.

@sfan5 sfan5 merged commit 852d6a7 into minetest:master Aug 27, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants