Skip to content

ext/dom: Fix Dom\DtdNamedNodeMap index overflow in dimension access#22630

Merged
LamentXU123 merged 3 commits into
php:masterfrom
LamentXU123:fix-dom-overflow
Jul 8, 2026
Merged

ext/dom: Fix Dom\DtdNamedNodeMap index overflow in dimension access#22630
LamentXU123 merged 3 commits into
php:masterfrom
LamentXU123:fix-dom-overflow

Conversation

@LamentXU123

Copy link
Copy Markdown
Member

Dom\DtdNamedNodeMap dimension access could overflow the index when reading DTD entities or notations.

<?php
$doc = Dom\XMLDocument::createFromString("<!DOCTYPE root [<!ENTITY e 'x'>]><root/>");

var_dump($doc->doctype->entities[4294967296]?->nodeName);
string(1) "e"

}
}

dump_access(fn() => $doc->doctype->entities[$overflow]);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: let s test the negative values too

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Given other modern DOM collection dimension returns None in negative indexes and ValueError in out-of-range indexes, let's align with them.

$element->attributes[-1]; // NULL
$doctype->entities->item(4294967296); // ValueError: Argument #1 ($index) must be between 0 and 2147483647

@LamentXU123
LamentXU123 merged commit ff480bc into php:master Jul 8, 2026
18 checks passed
@LamentXU123
LamentXU123 deleted the fix-dom-overflow branch July 8, 2026 12:10
@LamentXU123

Copy link
Copy Markdown
Member Author

Does this need a UPGRADING entry? @devnexen

@devnexen

devnexen commented Jul 8, 2026

Copy link
Copy Markdown
Member

yes

LamentXU123 added a commit that referenced this pull request Jul 8, 2026
Dom\DtdNamedNodeMap array access negative index behavior: PR #22630

Dom\DtdNamedNodeMap array access INT_MAX index validation: PR #22630

sleep() maximum seconds validation: PR #22619

usleep() maximum microseconds validation: PR #22619
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.

2 participants