Skip to content

Fix segfault comparing uninitialized SimpleXMLElement instances - #23067

Closed
iliaal wants to merge 1 commit into
php:PHP-8.4from
iliaal:fix/sxe-compare-uninitialized
Closed

Fix segfault comparing uninitialized SimpleXMLElement instances#23067
iliaal wants to merge 1 commit into
php:PHP-8.4from
iliaal:fix/sxe-compare-uninitialized

Conversation

@iliaal

@iliaal iliaal commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

sxe_objects_compare reaches sxe1->document->ptr whenever both operands have a NULL node, without checking that document is set. A subclass whose constructor skips parent::__construct leaves both fields NULL, so new MySXE == new MySXE dereferences NULL and segfaults. The documents are compared only when both are set, so two uninitialized instances are uncomparable, matching the node comparison directly above.

@iliaal
iliaal requested a review from devnexen as a code owner August 5, 2026 13:27
iliaal added a commit to iliaal/php-src that referenced this pull request Aug 5, 2026
sxe_objects_compare dereferenced document->ptr when both nodes were
NULL without checking document. A subclass that skips parent
__construct leaves document NULL, so $a == $b segfaulted. Treat two
NULL documents as equal and mixed NULL/non-NULL as uncomparable.

Closes phpGH-23067
@iliaal
iliaal force-pushed the fix/sxe-compare-uninitialized branch from 6701e73 to 0094ebf Compare August 5, 2026 13:28

@arnaud-lb arnaud-lb left a comment

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.

Looks good to me!

Comment thread ext/simplexml/simplexml.c Outdated
if (sxe1->node == NULL && sxe2->node == NULL) {
/* Both nodes not set: Only support equality comparison between documents. */
if (sxe1->document == NULL || sxe2->document == NULL) {
if (sxe1->document == sxe2->document) {

@devnexen devnexen Aug 10, 2026

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.

we should only compare documents (not referring to the field name).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Switched to comparing the documents, with a missing document resolving to NULL.

@iliaal
iliaal force-pushed the fix/sxe-compare-uninitialized branch from 0094ebf to 801195e Compare August 10, 2026 12:33
@iliaal
iliaal requested a review from devnexen August 10, 2026 12:35
Comment thread ext/simplexml/simplexml.c Outdated
if (sxe1->document->ptr == sxe2->document->ptr) {
xmlDocPtr doc1 = sxe1->document ? sxe1->document->ptr : NULL;
xmlDocPtr doc2 = sxe2->document ? sxe2->document->ptr : NULL;
if (doc1 == doc2) {

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.

I meant

if (sxe1->document != NULL && sxe2->document != NULL && sxe1->document->ptr == sxe2->document->ptr)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ah, misread the first time. Applied as written.

sxe_objects_compare dereferenced document->ptr when both nodes were
NULL without checking document. A subclass that skips parent
__construct leaves document NULL, so $a == $b segfaulted. Compare the
documents only when both are set; anything else is uncomparable.
@iliaal
iliaal force-pushed the fix/sxe-compare-uninitialized branch from 801195e to 4bf8c86 Compare August 10, 2026 13:02
@iliaal iliaal closed this in 7b1f563 Aug 10, 2026
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