[3.x] Fix index being out of sync in Node::remove_child#77483
[3.x] Fix index being out of sync in Node::remove_child#77483lawnjelly wants to merge 1 commit intogodotengine:3.xfrom
Node::remove_child#77483Conversation
lawnjelly
force-pushed
the
fix_remove_child_idx
branch
from
04ab028 to
b0d0bba
Compare
scene/main/node.cpp
Outdated
There was a problem hiding this comment.
Is this safe in this context? I might be misunderstanding this change, it seems that if we mess this up we can't really be sure that p_child is still pointing to what we think it is?
There was a problem hiding this comment.
p_child is still pointing to the correct child, the issue is that the idx of the child (i.e. position in the childlist) can change during the pre-deletion, which is the mechanism of the bug in the issue.
The ERR_FAIL here should not happen unless something really bad is going on, like p_child is being removed as a child during the pre-delete. This is very likely not possible, but this ERR_FAIL is more a fail safe to prevent a crash in this scenario, and indicate where the problem is occurring. As this if clause should be pretty rare anyway, there's no cost to adding an ERR_FAIL here.
There was a problem hiding this comment.
Ah there is a potential bug here though, I forgot to reget child_count as it could have changed. Will fix. 👍
DONE.
There was a problem hiding this comment.
Just to reiterate, p_child will still point to the same child in the same area of memory, as children are non-relocatable - it is only the index that may change.
In rare circumstances, the index of the child to be removed can get out of sync during the routine. We detect this scenario and recalculate the index, preventing corruption of the children.
lawnjelly
force-pushed
the
fix_remove_child_idx
branch
from
b0d0bba to
5790513
Compare
YuriSizov
changed the title
Node::remove_child
akien-mga
changed the title
Node::remove_childNode::remove_child
| // Child count could have changed. | ||
| child_count = data.children.size(); | ||
|
|
||
| if ((p_child->data.pos != idx) || (idx >= child_count) || (data.children[idx] != p_child)) { |
There was a problem hiding this comment.
I didn't write a comment here, but just to make it clear for review, there are three possibilities here (for detecting a change to p_child's index):
- The child itself knows that its index has changed
- The previous index is now outside the range of the new child count
- The child of the parent given by the index is no longer
p_child(this can be checked providingidx < child_countin the previous)
(The first could possibly be omitted, but its cheap to check)
2 participants
In rare circumstances, the index of the child to be removed can get out of sync during the routine. We detect this scenario and recalculate the index, preventing corruption of the children.
Fixes #77480
Notes
_set_tree(nullptr), but just in case any of the other calls affects the index, I've moved immediately prior todata.children.remove(idx).ERR_FAILmessage is subtly different, to enable us to distinguish between the two cases.