Fix DOMElement::append() and DOMElement::prepend() hierarchy checks #11344
Conversation
We could end up in an invalid hierarchy, resulting in infinite loops and eventual crashes if we don't check for the DOM hierarchy validity.
| @@ -296,6 +319,11 @@ void dom_parent_node_prepend(dom_object *context, zval *nodes, int nodesc) | |||
| return; | |||
| } | |||
|
|
|||
| if (UNEXPECTED(dom_hierarchy_node_list(parentNode, nodes, nodesc) != SUCCESS)) { | |||
There was a problem hiding this comment.
Any reason for using != SUCCESS? Shouldn't really be an issue here but when I was converting a lot of int return values to zend_result this code pattern always made me triple check that I was not missing a case where something other than SUCCESS and FAILURE was returned.
| if (UNEXPECTED(dom_hierarchy_node_list(parentNode, nodes, nodesc) != SUCCESS)) { | |
| if (UNEXPECTED(dom_hierarchy_node_list(parentNode, nodes, nodesc) == FAILURE)) { |
There was a problem hiding this comment.
I have a habit of doing != SUCCESS because it's a comparison against 0, which encodes as a shorter instruction than a comparison with -1. I saw both != SUCCESS and == FAILURE in the code so I just picked the one I liked most.
I can change it if you want?
There was a problem hiding this comment.
Either is fine frankly, I think I was told about the shorter instruction once.
I believe that one day zend_result can be a true alias for bool... but that's a lot of code auditing.
| static zend_result dom_hierarchy_node_list(xmlNodePtr parentNode, zval *nodes, int nodesc) | ||
| { | ||
| for (int i = 0; i < nodesc; i++) { | ||
| if (Z_TYPE(nodes[i]) == IS_OBJECT) { |
There was a problem hiding this comment.
Wait, can a node not be an object?
There was a problem hiding this comment.
Spec says you may insert strings, so it can be IS_STRING too. In that case we create a text node, but since that's a new object it cannot violate the dom hierarchy.
There was a problem hiding this comment.
Right, I didn't think that the zval node list could contain anything other than objects, as I would have expected free strings to always be encoded as DOMText object. Just having wrong assumptions.
We could end up in an invalid hierarchy, resulting in infinite loops and eventual crashes if we don't check for the DOM hierarchy validity.
This is in accordance to the "pre-insertion validity" check from spec.
Requesting a review from @Girgias again (sorry 😅) since no one else seems to be able to review these atm.