Skip to content

Fix error paths in dom_xml_serializing_a_document_node() - #24564

Closed
ndossche wants to merge 2 commits into
php:PHP-8.4from
ndossche:fix-error-paths-dom
Closed

ndossche wants to merge 2 commits into
php:PHP-8.4from
ndossche:fix-error-paths-dom

Conversation

@ndossche

@ndossche ndossche commented Oct 22, 2026 •

Copy link
Copy Markdown
Member

They should restore the children pointer even in case of a write failure.

They should restore the child pointers even in case of a write failure.
Comment thread ext/dom/xml_serializer.c Outdated
return 0;

cleanup:
node->children = child;

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.

question: do we need to set node->last throughout the whole workflow too ? I think not but ...

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.

No, the loops that use the node only care about the next and children fields, not last.

@iliaal iliaal 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.

LGTM.

Unrelated, but nearby: a userland stream_write() that runs during the xmlSaveFlush() here sees an empty document and can insert into it, which leaves children/last inconsistent and segv on a later append. I'll make a PR, unless you want to tackle it or fold-it-in.

@ndossche

Copy link
Copy Markdown
Member Author

Yeah you're correct unfortunately. I pushed out a fix for that into this PR. It now makes a shallow copy of the document just for the declaration.

@iliaal iliaal 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, thanks for making the changes

@ndossche ndossche closed this in 6454e10 Oct 24, 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