Skip to content

Align DOMChildNode parent checks with spec #11905

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

Closed
wants to merge 7 commits into from
Closed

Align DOMChildNode parent checks with spec #11905

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
163e16d
Align DOMChildNode parent checks with spec
nielsdos Aug 7, 2023
99908d5
Fix mistake where it did not output a hierarchy error because the par…
nielsdos Aug 7, 2023
68fa8d4
Extend regression test
nielsdos Aug 7, 2023
be1867e
Remove document check
nielsdos Aug 8, 2023
a521284
Add additional test
nielsdos Aug 8, 2023
02a3dbb
[ci skip] UPGRADING
nielsdos Aug 8, 2023
1cb6dbd
Fix null pointer dereference by always using the node instead of the …
nielsdos Aug 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions UPGRADING
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,11 @@ PHP 8.3 UPGRADE NOTES
iterating over the WeakMap (reachability via iteration is considered weak).
Previously, such entries would never be automatically removed.

- DOM:
. DOMChildNode::after(), DOMChildNode::before(), DOMChildNode::replaceWith()
on a node that has no parent is now a no-op instead of a hierarchy exception,
which is the behaviour spec demands.

- FFI:
. C functions that have a return type of void now return null instead of
returning the following object object(FFI\CData:void) { }
Expand Down
42 changes: 16 additions & 26 deletions ext/dom/parentnode.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,8 +240,13 @@ static void dom_fragment_assign_parent_node(xmlNodePtr parentNode, xmlNodePtr fr

static zend_result dom_sanity_check_node_list_for_insertion(php_libxml_ref_obj *document, xmlNodePtr parentNode, zval *nodes, int nodesc)
{
if (document == NULL) {
php_dom_throw_error(HIERARCHY_REQUEST_ERR, 1);
if (UNEXPECTED(document == NULL)) {
php_dom_throw_error(HIERARCHY_REQUEST_ERR, 1 /* no document, so be strict */);
Copy link
Member

Choose a reason for hiding this comment

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

I'm confused, is it really a hierarchy error if we don't have a document?

Copy link
Member Author

@nielsdos nielsdos Aug 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Couldn't find this condition in spec, it was added in #5990 to fix https://bugs.php.net/bug.php?id=79968.
However, testing it with a custom element (*) in JS seems to work without an exception:

class Foo extends HTMLElement{}
customElements.define("my-foo", Foo)
foo = new Foo;
foo.after(new Foo);
(new XMLSerializer).serializeToString(foo);

(*) custom element, because in JS you can't instantiate an element directly without a document unless you create a custom one.

And you know what's weird? When I remove this check I expected the original bug I linked above to appear again. But nope, no crash 🤔
I guess the actual segfault back then happened not because of the missing document, but because of the context node being NULL. However, in current day's code the context node is NULL-checked.
Also calling dom_get_strict_error with a NULL argument is fine, because dom_get_strict_error will return the default values in that case.

I have pushed a commit to remove that condition and amend the test (again).

Copy link
Member Author

Choose a reason for hiding this comment

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

I've added an additional test for this with elements.

Copy link
Member Author

@nielsdos nielsdos Aug 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Ah so there was a possible null-pointer crash after all, but I fixed it by reading the document property from the node instead of from the context. The context->document pointer may be NULL, but the node can't be.

return FAILURE;
}

if (UNEXPECTED(parentNode == NULL)) {
/* No error required, this must be a no-op per spec */
return FAILURE;
}

Expand Down Expand Up @@ -394,10 +399,9 @@ void dom_parent_node_after(dom_object *context, zval *nodes, uint32_t nodesc)

/* Spec step 1 */
parentNode = prevsib->parent;
/* Spec step 2 */
if (!parentNode) {
int stricterror = dom_get_strict_error(context->document);
php_dom_throw_error(HIERARCHY_REQUEST_ERR, stricterror);

/* Sanity check for fragment, includes spec step 2 */
if (UNEXPECTED(dom_sanity_check_node_list_for_insertion(context->document, parentNode, nodes, nodesc) != SUCCESS)) {
return;
}

Expand All @@ -412,10 +416,6 @@ void dom_parent_node_after(dom_object *context, zval *nodes, uint32_t nodesc)

doc = prevsib->doc;

if (UNEXPECTED(dom_sanity_check_node_list_for_insertion(context->document, parentNode, nodes, nodesc) != SUCCESS)) {
return;
}

php_libxml_invalidate_node_list_cache_from_doc(doc);

/* Spec step 4: convert nodes into fragment */
Expand Down Expand Up @@ -451,10 +451,9 @@ void dom_parent_node_before(dom_object *context, zval *nodes, uint32_t nodesc)

/* Spec step 1 */
parentNode = nextsib->parent;
/* Spec step 2 */
if (!parentNode) {
int stricterror = dom_get_strict_error(context->document);
php_dom_throw_error(HIERARCHY_REQUEST_ERR, stricterror);

/* Sanity check for fragment, includes spec step 2 */
if (UNEXPECTED(dom_sanity_check_node_list_for_insertion(context->document, parentNode, nodes, nodesc) != SUCCESS)) {
return;
}

Expand All @@ -469,10 +468,6 @@ void dom_parent_node_before(dom_object *context, zval *nodes, uint32_t nodesc)

doc = nextsib->doc;

if (UNEXPECTED(dom_sanity_check_node_list_for_insertion(context->document, parentNode, nodes, nodesc) != SUCCESS)) {
return;
}

php_libxml_invalidate_node_list_cache_from_doc(doc);

/* Spec step 4: convert nodes into fragment */
Expand Down Expand Up @@ -553,10 +548,9 @@ void dom_child_replace_with(dom_object *context, zval *nodes, uint32_t nodesc)

/* Spec step 1 */
xmlNodePtr parentNode = child->parent;
/* Spec step 2 */
if (!parentNode) {
int stricterror = dom_get_strict_error(context->document);
php_dom_throw_error(HIERARCHY_REQUEST_ERR, stricterror);

/* Sanity check for fragment, includes spec step 2 */
if (UNEXPECTED(dom_sanity_check_node_list_for_insertion(context->document, parentNode, nodes, nodesc) != SUCCESS)) {
return;
}

Expand All @@ -574,10 +568,6 @@ void dom_child_replace_with(dom_object *context, zval *nodes, uint32_t nodesc)
viable_next_sibling = viable_next_sibling->next;
}

if (UNEXPECTED(dom_sanity_check_node_list_for_insertion(context->document, parentNode, nodes, nodesc) != SUCCESS)) {
return;
}

php_libxml_invalidate_node_list_cache_from_doc(context->document->ptr);

/* Spec step 4: convert nodes into fragment */
Expand Down
44 changes: 44 additions & 0 deletions ext/dom/tests/DOMChildNode_methods_without_parent.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
--TEST--
DOMChildNode methods without a parent
--EXTENSIONS--
dom
--FILE--
<?php
$doc = new DOMDocument;
$doc->loadXML(<<<XML
<?xml version="1.0"?>
<container>
<child/>
</container>
XML);

$container = $doc->documentElement;
$child = $container->firstElementChild;

$test = $doc->createElement('foo');
foreach (['before', 'after', 'replaceWith'] as $method) {
echo "--- $method ---\n";
$test->$method($child);
echo $doc->saveXML();
echo $doc->saveXML($test), "\n";
}
?>
--EXPECT--
--- before ---
<?xml version="1.0"?>
<container>
<child/>
</container>
<foo/>
--- after ---
<?xml version="1.0"?>
<container>
<child/>
</container>
<foo/>
--- replaceWith ---
<?xml version="1.0"?>
<container>
<child/>
</container>
<foo/>
17 changes: 16 additions & 1 deletion ext/dom/tests/bug79968.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,23 @@ $cdata = new DOMText;
try {
$cdata->before("string");
} catch (DOMException $e) {
echo $e->getMessage();
echo $e->getMessage(), "\n";
}

try {
$cdata->after("string");
} catch (DOMException $e) {
echo $e->getMessage(), "\n";
}

try {
$cdata->replaceWith("string");
} catch (DOMException $e) {
echo $e->getMessage(), "\n";
}

?>
--EXPECT--
Hierarchy Request Error
Hierarchy Request Error
Hierarchy Request Error