Fix crashes with unproper cleaning of repeated yield from #6130
Conversation
bwoebi
force-pushed
the
fix-yield-from-reuse
branch
from
9986dc4 to
6c157dc
Compare
Zend/zend_generators.c
Outdated
| if (node->ptr.leaf == from_leaf) { | ||
| node->ptr.leaf = to_leaf; | ||
| } | ||
| if (node->children <= 1) { |
There was a problem hiding this comment.
ZEND_ASSERT(node->child.single.leaf == from_leaf) here?
There was a problem hiding this comment.
No, as said in other comment, nodes with only a single child have any leaf there.
| } | ||
|
|
||
| static void zend_generator_remove_leaf_child(zend_generator_node *node, zend_generator *leaf, zend_generator *replace_leaf) { | ||
| if (node->children > 1) { |
There was a problem hiding this comment.
Shouldn't we have ZEND_ASSERT(node->children > 1) here? We only get here if parent.node->children > 1, which also implies that root.node->children > 1, I believe, as we store all the leaves.
Or else this is missing test coverage for the case where node->children <= 1 here...
There was a problem hiding this comment.
No, it does not imply that, we only store all the leaves if the node has multiple direct children (that's why this multi_children_node searching needs to exist, winding down the leafs until the first node with multiple children is found),
There was a problem hiding this comment.
Okay, I see. It would be great to have a test case that hits the else part, as none of the existing ones do.
| @@ -465,10 +540,13 @@ static void zend_generator_add_single_child(zend_generator_node *node, zend_gene | |||
| node->child.ht = ht; | |||
| } | |||
|
|
|||
| zend_hash_index_add_ptr(node->child.ht, (zend_ulong) leaf, child); | |||
| if (zend_hash_index_add_ptr(node->child.ht, (zend_ulong) leaf, child) == NULL) { | |||
| ZEND_ASSERT(node->children > 1); | |||
There was a problem hiding this comment.
Hm, I don't quite get under what circumstances the leaf is already in the table?
There was a problem hiding this comment.
It may be added a second time while merging child nodes, when the parent is already a multi_children_node. Could be probably also prevented by tweaking the conditions in the final while loop of zend_generator_add_child(), but I didn't bother figuring that out with respect to all possible cases we have there.
There was a problem hiding this comment.
If you want to have a look, check out Zend/tests/generators/yield_from_multi_tree.phpt
There was a problem hiding this comment.
Ah, I remember the exact reason: each node with just one direct child does not have all the eventual leafs … now, when we add a child to a non-leaf node we now have two children - and in that case we need to add all the children of this nodes next multi_children_node, which also contains the current leaf.
We need to skip that one at least.
| @@ -507,7 +585,6 @@ static void zend_generator_add_child(zend_generator *generator, zend_generator * | |||
| } else if (generator->node.children == 1) { | |||
| multi_children_node = zend_generator_search_multi_children_node(&generator->node); | |||
| if (multi_children_node) { | |||
| generator->node.children = 0; | |||
There was a problem hiding this comment.
Why did this have to be dropped?
There was a problem hiding this comment.
Due to the change to zend_generator_add_single_child: the already existing child is already there, and thanks to the change, it is not being counted again; before it was and thus the count had to be reset.
bwoebi
force-pushed
the
fix-yield-from-reuse
branch
from
6c157dc to
508cf0c
Compare
bwoebi
force-pushed
the
fix-yield-from-reuse
branch
from
508cf0c to
2d217f7
Compare
| } | ||
| } else if (generator->node.parent) { |
There was a problem hiding this comment.
I think you should be able to move this whole if() block before the children == 0 check and thus avoid the need for goto.
@nikic there it is :-)
Basically: children were sometimes counted doubly and the destructor cleaning was just way to primitive, the general logic of yield from handling seems to have proven sound.