Fix GH-23120: DOMNode::isEqualNode stack overflow on deeply nested trees - #23140
Conversation
|
I would rather prefer you control the stack limit ; also, for once, the CI arm issue is very related (timeout?). |
| ZEND_ASSERT(other != NULL); | ||
|
|
||
| if (UNEXPECTED(php_dom_node_is_equal_node_check_stack_limit())) { | ||
| zend_throw_error(NULL, "Maximum call stack size reached. Infinite recursion?"); |
There was a problem hiding this comment.
it needs to be gated with the exception global.
|
|
||
| if (UNEXPECTED(php_dom_node_is_equal_node_check_stack_limit())) { | ||
| if (!EG(exception)) { | ||
| zend_throw_error(NULL, "Maximum call stack size reached. Infinite recursion?"); |
There was a problem hiding this comment.
This can't be infinite recursion because a tree is an acyclic graph
| zend_throw_error(NULL, "Maximum call stack size reached. Infinite recursion?"); | |
| zend_throw_error(NULL, "Maximum call stack size reached."); |
There was a problem hiding this comment.
This is correct! I copy-paste this message from the last fix #23127 and I overlook this. Thx
* PHP-8.4: Fix phpGH-23120: DOMNode::isEqualNode stack overflow on deeply nested trees (php#23140)
|
This fails in CI, see https://github.com/php/php-src/actions/runs/31288567288/job/93181842068. |
|
ffs, I didn't expect stack depth on every platform to be that huge. |
|
@shivammathur had fixed later on here I believe. |
|
@devnexen Ah thanks, I missed that there was another commit. |
|
I see, but I think this can still be improved..? I suggest to use Fibers on this because we want our test to pass on extremely extreme platforms. something like and call isEqualSize in Fibers. |
|
That seems overengineered to me. |
|
Understandable :P |
4 participants
DOMNode::isEqualNode() recursively compared ordered child lists.
Compare each node without descending into its children first to solve this and we walk both subtrees in tree order to compare descendants iteratively.
Fix GH-23120