Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Two small xml fixes #405

Merged
merged 4 commits into from
May 15, 2024
Merged

Two small xml fixes #405

merged 4 commits into from
May 15, 2024

Conversation

sensetalkdoug
Copy link
Contributor

Fix execute_xpath() to return an NSError when expression can't be evaluated; Remove previous attributes when setting attributes; Add related tests

…luated; Remove previous attributes when setting attributes; Add related tests
@sensetalkdoug sensetalkdoug requested a review from rfm as a code owner May 14, 2024 21:28
@sensetalkdoug
Copy link
Contributor Author

While working with these classes, I also noticed this problem, which I did not try to fix:
The [NSXMLElement setNamespaces:] method is broken. It calls xmlFreeNsList() which frees the memory associated with all of the old namespaces. But there can be many pointers to those namespaces still in the ns entries of various nodes in the tree, which can lead to memory access crashes. Finding all of those references and setting them to NULL would prevent the crashing, but I believe is the wrong answer since the replacement namespaces being added by the call to setNamespaces: may include new (or even identical) entries for the same prefixes, so ideally you’d like to reset each node to point to the new namespace for that prefix.

Copy link
Contributor

@rfm rfm left a comment

Choose a reason for hiding this comment

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

Both changes looks good, and nice to see testcases. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants