Skip to content

Fix use after unlink in XML::Node#16432

Merged
straight-shoota merged 1 commit intocrystal-lang:masterfrom
toddsundsted:use-after-free
Dec 12, 2025
Merged

Fix use after unlink in XML::Node#16432
straight-shoota merged 1 commit intocrystal-lang:masterfrom
toddsundsted:use-after-free

Conversation

@toddsundsted
Copy link
Contributor

This code works because attributes only have a single text node in practice, but the use after unlink is troublesome. The code also checks (obj = cached?(node)) the parent when iterating over the children. There is no memory leak here, but I am including the PR since I think it brings the implementation in line with the name of the method.

@straight-shoota straight-shoota added kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:stdlib:serialization labels Nov 25, 2025
@ysbaddaden
Copy link
Collaborator

ysbaddaden commented Nov 26, 2025

The iteration and call the XML::Node#cached? are indeed wrong 🤦

I believe XML::Attributes#[]= must also be fixed: the bug causes the node to be unlinked while children aren't, and the patch will do the opposite: unlink the children but keep the node.

Maybe it should behave like XML::Attributes#delete and always unlink the prop, then libxml would always add the prop (never replacing it) 🤔

@straight-shoota
Copy link
Member

What's wrong about keeping the node? xmlSetProp reuses it, so I don't see why we would need to remove it?

Following #delete sounds like a good plan though if there's a good reason to remove the prop node.

@ysbaddaden
Copy link
Collaborator

I wrongly thought that xmlSetProp would free the libxml node while we might still have live references, but reading the libxml code again, it only unlinks the children the node might have, so this is fine 👍

@straight-shoota straight-shoota added this to the 1.19.0 milestone Dec 11, 2025
@straight-shoota straight-shoota changed the title Fix Use After Unlink Fix use after unlink in XML::Node Dec 11, 2025
@straight-shoota straight-shoota merged commit a8e85a6 into crystal-lang:master Dec 12, 2025
46 checks passed
@toddsundsted
Copy link
Contributor Author

thanks!

toddsundsted added a commit to toddsundsted/ktistec that referenced this pull request Feb 24, 2026
toddsundsted added a commit to toddsundsted/ktistec that referenced this pull request Mar 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:stdlib:serialization

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants