Skip to content

Fix #70359 and #78577 #11402

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 6 commits into from
Closed

Fix #70359 and #78577 #11402

wants to merge 6 commits into from

Conversation

nielsdos
Copy link
Member

@nielsdos nielsdos commented Jun 8, 2023 •
edited
Loading

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

Both these issues have the same underlying 2 problems. Even though these issue reports use debugging functionality, it also segfaults if you access some fields directly.

The 2 underlying problems are:

  • There was some incorrect field setting, and weird code due to the casting and abuse of types. I solved this in one go by casting to the right type and using the correct fields.
  • We're creating a fake child, but adding the parent node to it. However, because the fake child is not part of the parent's node children list, it will not get destroyed together with the parent when the parent gets destroyed. The consequence is that if you remove the parent, instead of invalidating the attribute, it will cause a use-after-free. The only way I could think of to solve this is to get an additional reference to the parent. Implementation-wise this was simply a matter of adding a custom constructor and destructor handler for the DOMNamespaceNode.

I've broken up the PR into individual commits to make it easier to follow for reviews. Will squash upon merge.

nielsdos
nielsdos commented Jun 8, 2023
View reviewed changes
ext/dom/php_dom.c Outdated
attrp->ns = curns;

php_dom_create_object(attrp, return_value, parent_intern);
/* This object must exist, because we just created an object for it via DOM_RET_OBJ. */
Copy link
Member Author

@nielsdos nielsdos Jun 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Right, should've said php_dom_create_object() in the comment instead of DOM_RET_OBJ... But CI is running now so I'll fix it in the merge..

Girgias
Girgias approved these changes Jun 9, 2023
View reviewed changes
Copy link
Member

@Girgias Girgias left a comment

Choose a reason for hiding this comment

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

Thanks for tackling this, going through the individual commits was definitely helpful.

ext/dom/php_dom.c Outdated
@@ -1001,10 +1009,8 @@ void dom_namednode_iter(dom_object *basenode, int ntype, dom_object *intern, xml
}
/* }}} */

static dom_object* dom_objects_set_class(zend_class_entry *class_type) /* {{{ */
static void dom_objects_set_class_ex(zend_class_entry *class_type, dom_object *intern) /* {{{ */
Copy link
Member

Choose a reason for hiding this comment

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

Nit you probably want to remove the trailing /* {{{ */ comment

ext/dom/php_dom.c Outdated
@@ -1028,6 +1038,26 @@ zend_object *dom_objects_new(zend_class_entry *class_type)
}
/* }}} */

static zend_object *dom_objects_namespace_node_new(zend_class_entry *class_type)
{
ZEND_ASSERT(class_type == dom_namespace_node_class_entry);
Copy link
Member

Choose a reason for hiding this comment

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

Would this be an issue if someone sets a custom class for namespace nodes?

Otherwise, just asserting it is an instance of the namespace node class should be fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right now it's not possible to register a custom class for DOMNameSpaceNode, likely because of an oversight.
I'll get rid of the assert anyway though, because this might be a problem when support is added in the future. The assert only had the purpose of sanity checking that I was using the right object handlers, so it's safe to remove.

@nielsdos nielsdos closed this in f2d673f Jun 9, 2023
nielsdos added a commit to nielsdos/php-src that referenced this pull request Jun 10, 2023
@nielsdos
Fix lifetime issue with getAttributeNodeNS()
ba2f9f4 
It's the same issue that I fixed previously in phpGH-11402, but in a
different place.
@nielsdos nielsdos mentioned this pull request Jun 10, 2023
Closed
nielsdos added a commit that referenced this pull request Jun 13, 2023
@nielsdos
Fix lifetime issue with getAttributeNodeNS()
e309fd8 
It's the same issue that I fixed previously in GH-11402, but in a
different place.

Closes GH-11422.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Girgias Girgias Girgias approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@nielsdos @Girgias