Skip to content

Fix GH-23116 and GH-23117: stack overflow when normalizing a deeply nested document - #23127

Closed
lazerg wants to merge 2 commits into
php:PHP-8.4from
lazerg:fix/gh-23116-23117-dom-normalize-stack-limit
Closed

Fix GH-23116 and GH-23117: stack overflow when normalizing a deeply nested document#23127
lazerg wants to merge 2 commits into
php:PHP-8.4from
lazerg:fix/gh-23116-23117-dom-normalize-stack-limit

Conversation

@lazerg

@lazerg lazerg commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

DOMNode::normalize() and Dom\Node::normalize() walk the tree by recursing once per element child, so a deeply nested document exhausts the C stack and segfaults. Both now check the stack limit on entry and throw an Error, the same way the XML serializer has since GH-22570.

The throw is gated on EG(exception) because these functions return void: without it, unwinding through a tree that is wide at the overflow depth throws once per node and chains the Errors, which is quadratic.

Fixes #23116
Fixes #23117

@lazerg
lazerg requested a review from devnexen as a code owner August 8, 2026 04:58
@lazerg
lazerg force-pushed the fix/gh-23116-23117-dom-normalize-stack-limit branch from ad54d80 to 8be21fc Compare August 8, 2026 05:23
@devnexen

devnexen commented Aug 8, 2026

Copy link
Copy Markdown
Member

nit: can we exercise this test somewhere ?

$doc = new DOMDocument();
$root = $doc->createElement('root');
for ($s = 0; $s < 2; $s++) {
    $node = $doc->createElement('a');
    for ($i = 0; $i < 100000; $i++) {
        $parent = $doc->createElement('a');
        $parent->appendChild($node);
        $node = $parent;
    }
    $root->appendChild($node);
}
$doc->appendChild($root);

try {
    $doc->normalize();
} catch (\Error $e) {
    echo "wide: ", $e::class, ": ", $e->getMessage(), "\n";
    **var_dump($e->getPrevious());**
}

expecting

wide: Error: Maximum call stack size reached. Infinite recursion?
NULL

@lazerg
lazerg force-pushed the fix/gh-23116-23117-dom-normalize-stack-limit branch from 8be21fc to 86d9b4a Compare August 8, 2026 05:39
@lazerg
lazerg force-pushed the fix/gh-23116-23117-dom-normalize-stack-limit branch from c03b193 to 8fb1d7c Compare August 8, 2026 05:42
@lazerg

lazerg commented Aug 8, 2026

Copy link
Copy Markdown
Contributor Author

Added. Both tests now build two deep branches under one root and assert that getPrevious() is NULL. I checked that this assertion does fail if I drop the !EG(exception) gate, so it exercises the case you meant.

I also moved the tests off loadXML() while I was in there. The FreeBSD job capped the parse depth at 2048 even with LIBXML_PARSEHUGE, so the tree never got deep enough to reach the guard. Building through the DOM API at depth 25000 with a 256K stack limit gives the same coverage and runs in about 2.5s.

LamentXU123 added a commit that referenced this pull request Aug 8, 2026
* PHP-8.5:
  Fix GH-23116 and GH-23117: stack overflow when normalizing a deeply nested document (#23127)
@LamentXU123

Copy link
Copy Markdown
Member

Thanks!

pull Bot pushed a commit to KornaPhp/php-src that referenced this pull request Aug 8, 2026
* PHP-8.4:
  Fix phpGH-23116 and phpGH-23117: stack overflow when normalizing a deeply nested document (php#23127)
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.

3 participants