Skip to content

Fixed bug #78577 #4731

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

Closed
wants to merge 2 commits into from
Closed

Fixed bug #78577 #4731

wants to merge 2 commits into from

Conversation

beberlei
Copy link
Contributor

@beberlei beberlei commented Sep 20, 2019

The nsparent should actually be the nodep already from the DOMElement#getAttributeNode() call.

Fixes https://bugs.php.net/bug.php?id=78577

@beberlei
Copy link
Contributor Author

Related to https://bugs.php.net/bug.php?id=70359

@nikic
Copy link
Member

nikic commented Sep 23, 2019

There's some similar code doing nsparent = node->_private in xpath.c.

@beberlei
Copy link
Contributor Author

@nikic You are right, multiple times actually. Going to try to get it to segfault as well using XPath in the test-case.

@beberlei
Copy link
Contributor Author

Triggering the copy pasted code in xpath.c does not lead to a segfault, so I think that @cmb69 assertion on the linked ticket is right, that the culprit is the invalid cast in dom_get_dom1_attribute.

@robrichards
Copy link

@beberlei I sent you a patch to try as the type case in that method was intentional. It was used incorrectly tho in the dom_element_get_attribute_node method

@krakjoe krakjoe added the Bug label Sep 30, 2019
@krakjoe
Copy link
Member

krakjoe commented Sep 30, 2019

Can we get a status update here please ?

@beberlei
Copy link
Contributor Author

beberlei commented Oct 4, 2019

So I wrote with @robrichards on this, and incorporated parts of a patch that he sent me. One problem is still that the previous code using _private doesn't work even though an explicit cast ((xmlNsPtr)attr)->_private; addresses the problem that the field is in different order compared to xmlNodePtr.

So i kept my part of the patch that uses nodep instead of attrp->_private for the parent node, because they are the same by the nature of the current DOM operation.

@beberlei
Copy link
Contributor Author

beberlei commented Oct 5, 2019

The fix is still not good, accessing $attr->parentNode will point to invalid memory when the parent node is removed from the DOM and garbage collected.

@github-actions
Copy link

There has not been any recent activity in this PR. It will automatically be closed in 7 days if no further action is taken.

@github-actions github-actions bot added the Stale label Feb 15, 2022
@github-actions github-actions bot closed this Feb 22, 2022
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.

4 participants