Fix bug #77686: Removed elements are still returned by getElementById

nielsdos · nielsdos · commit 0e34ac864a20 · 2023-06-04T16:20:34.000+02:00
From the moment an ID is created, libxml2's behaviour is to cache that element, even if that element is not yet attached to the document. Similarly, only upon destruction of the element the ID is actually removed by libxml2. Since libxml2 has such behaviour deeply ingrained in the library, and uses the cache for various purposes, it seems like a bad idea and lost cause to fight it. Instead, we'll simply walk the tree upwards to check if the node is attached to the document. Closes GH-11369.
diff --git a/NEWS b/NEWS
@@ -25,6 +25,8 @@ PHP                                                                        NEWS
     namespaces). (nielsdos)
   . Fixed bug #81642 (DOMChildNode::replaceWith() bug when replacing a node
     with itself). (nielsdos)
+  . Fixed bug #77686 (Removed elements are still returned by getElementById).
+    (nielsdos)
 
 - Opcache:
   . Fix allocation loop in zend_shared_alloc_startup(). (nielsdos)
diff --git a/ext/dom/document.c b/ext/dom/document.c
@@ -1008,6 +1008,19 @@ PHP_METHOD(DOMDocument, getElementsByTagNameNS)
 }
 /* }}} end dom_document_get_elements_by_tag_name_ns */
 
+static bool php_dom_is_node_attached(const xmlNode *node)
+{
+	ZEND_ASSERT(node != NULL);
+	node = node->parent;
+	while (node != NULL) {
+		if (node->type == XML_DOCUMENT_NODE || node->type == XML_HTML_DOCUMENT_NODE) {
+			return true;
+		}
+		node = node->parent;
+	}
+	return false;
+}
+
 /* {{{ URL: http://www.w3.org/TR/2003/WD-DOM-Level-3-Core-20030226/DOM3-Core.html#core-ID-getElBId
 Since: DOM Level 2
 */
@@ -1030,7 +1043,13 @@ PHP_METHOD(DOMDocument, getElementById)
 
 	attrp = xmlGetID(docp, (xmlChar *) idname);
 
-	if (attrp && attrp->parent) {
+	/* From the moment an ID is created, libxml2's behaviour is to cache that element, even
+	 * if that element is not yet attached to the document. Similarly, only upon destruction of
+	 * the element the ID is actually removed by libxml2. Since libxml2 has such behaviour deeply
+	 * ingrained in the library, and uses the cache for various purposes, it seems like a bad
+	 * idea and lost cause to fight it. Instead, we'll simply walk the tree upwards to check
+	 * if the node is attached to the document. */
+	if (attrp && attrp->parent && php_dom_is_node_attached(attrp->parent)) {
 		DOM_RET_OBJ((xmlNodePtr) attrp->parent, &ret, intern);
 	} else {
 		RETVAL_NULL();
diff --git a/ext/dom/tests/bug77686.phpt b/ext/dom/tests/bug77686.phpt
@@ -0,0 +1,40 @@
+--TEST--
+Bug #77686 (Removed elements are still returned by getElementById)
+--EXTENSIONS--
+dom
+--FILE--
+<?php
+
+$doc = new DOMDocument;
+$doc->loadHTML('<html id="htmlelement"><body id="x">before<div id="y">hello</div>after</body></html>');
+$body = $doc->getElementById('x');
+$div = $doc->getElementById('y');
+var_dump($doc->getElementById('y')->textContent);
+
+// Detached from document, should not find it anymore
+$body->removeChild($div);
+var_dump($doc->getElementById('y'));
+
+// Added again, should find it
+$body->appendChild($div);
+var_dump($doc->getElementById('y')->textContent);
+
+// Should find root element without a problem
+var_dump($doc->getElementById('htmlelement')->textContent);
+
+// Created element but not yet attached, should not find it before it is added
+$new_element = $doc->createElement('p');
+$new_element->textContent = 'my new text';
+$new_element->setAttribute('id', 'myp');
+var_dump($doc->getElementById('myp'));
+$body->appendChild($new_element);
+var_dump($doc->getElementById('myp')->textContent);
+
+?>
+--EXPECT--
+string(5) "hello"
+NULL
+string(5) "hello"
+string(16) "beforeafterhello"
+NULL
+string(11) "my new text"
