Skip to content

Commit 9c30647

Browse files
authored
Handle libxml2 OOM more consistently (php#11927)
This is a continuation of commit c2a58ab, in which several OOM error handling was converted to throwing an INVALID_STATE_ERR DOMException. Some places were missed and they still returned false without an exception, or threw a PHP_ERR DOMException. Convert all of these to INVALID_STATE_ERR DOMExceptions. This also reduces confusion of users going through documentation [1]. Unfortunately, not all node creations are checked for a NULL pointer. Some places therefore will not do anything if an OOM occurs (well, except crash). On the one hand it's nice to handle these OOM cases. On the other hand, this adds some complexity and it's very unlikely to happen in the real world. But then again, "unlikely" situations have caused trouble before. Ideally all cases should be checked. [1] php/doc-en#1741
1 parent 2cd8f3e commit 9c30647

File tree

8 files changed

+40
-31
lines changed

8 files changed

+40
-31
lines changed

NEWS

+1
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ DOM:
1515
. Fix cloning attribute with namespace disappearing namespace. (nielsdos)
1616
. Implement DOM HTML5 parsing and serialization RFC. (nielsdos)
1717
. Fix DOMElement->prefix with empty string creates bogus prefix. (nielsdos)
18+
. Handle OOM more consistently. (nielsdos)
1819

1920
FPM:
2021
. Implement GH-12385 (flush headers without body when calling flush()).

UPGRADING

+5
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,11 @@ PHP 8.4 UPGRADE NOTES
2525
you might encounter errors if the declaration is incompatible.
2626
Consult sections 2. New Features and 6. New Functions for a list of
2727
newly implemented methods and constants.
28+
. Some DOM methods previously returned false or a PHP_ERR DOMException if a new
29+
node could not be allocated. They consistently throw an INVALID_STATE_ERR
30+
DOMException now. This situation is extremely unlikely though and probably
31+
will not affect you. As a result DOMImplementation::createDocument() now has
32+
a tentative return type of DOMDocument instead of DOMDocument|false.
2833

2934
- PDO_DBLIB:
3035
. setAttribute, DBLIB_ATTR_STRINGIFY_UNIQUEIDENTIFIER and DBLIB_ATTR_DATETIME_CONVERT

ext/dom/document.c

+14-16
Original file line numberDiff line numberDiff line change
@@ -842,7 +842,12 @@ PHP_METHOD(DOM_Document, createElementNS)
842842
if (errorcode == 0) {
843843
if (xmlValidateName((xmlChar *) localname, 0) == 0) {
844844
nodep = xmlNewDocNode(docp, NULL, (xmlChar *) localname, (xmlChar *) value);
845-
if (nodep != NULL && uri != NULL) {
845+
if (UNEXPECTED(nodep == NULL)) {
846+
php_dom_throw_error(INVALID_STATE_ERR, /* strict */ true);
847+
RETURN_THROWS();
848+
}
849+
850+
if (uri != NULL) {
846851
xmlNsPtr nsptr = xmlSearchNsByHref(nodep->doc, nodep, (xmlChar *) uri);
847852
if (nsptr == NULL) {
848853
nsptr = dom_get_ns(nodep, uri, &errorcode, prefix);
@@ -860,17 +865,11 @@ PHP_METHOD(DOM_Document, createElementNS)
860865
}
861866

862867
if (errorcode != 0) {
863-
if (nodep != NULL) {
864-
xmlFreeNode(nodep);
865-
}
868+
xmlFreeNode(nodep);
866869
php_dom_throw_error(errorcode, dom_get_strict_error(intern->document));
867870
RETURN_FALSE;
868871
}
869872

870-
if (nodep == NULL) {
871-
RETURN_FALSE;
872-
}
873-
874873
DOM_RET_OBJ(nodep, &ret, intern);
875874
}
876875
/* }}} end dom_document_create_element_ns */
@@ -904,7 +903,12 @@ PHP_METHOD(DOM_Document, createAttributeNS)
904903
if (errorcode == 0) {
905904
if (xmlValidateName((xmlChar *) localname, 0) == 0) {
906905
nodep = (xmlNodePtr) xmlNewDocProp(docp, (xmlChar *) localname, NULL);
907-
if (nodep != NULL && uri_len > 0) {
906+
if (UNEXPECTED(nodep == NULL)) {
907+
php_dom_throw_error(INVALID_STATE_ERR, /* strict */ true);
908+
RETURN_THROWS();
909+
}
910+
911+
if (uri_len > 0) {
908912
nsptr = xmlSearchNsByHref(nodep->doc, root, (xmlChar *) uri);
909913
if (nsptr == NULL || nsptr->prefix == NULL) {
910914
nsptr = dom_get_ns(root, uri, &errorcode, prefix ? prefix : "default");
@@ -926,17 +930,11 @@ PHP_METHOD(DOM_Document, createAttributeNS)
926930
}
927931

928932
if (errorcode != 0) {
929-
if (nodep != NULL) {
930-
xmlFreeProp((xmlAttrPtr) nodep);
931-
}
933+
xmlFreeProp((xmlAttrPtr) nodep);
932934
php_dom_throw_error(errorcode, dom_get_strict_error(intern->document));
933935
RETURN_FALSE;
934936
}
935937

936-
if (nodep == NULL) {
937-
RETURN_FALSE;
938-
}
939-
940938
DOM_RET_OBJ(nodep, &ret, intern);
941939
}
942940
/* }}} end dom_document_create_attribute_ns */

ext/dom/domimplementation.c

+6-3
Original file line numberDiff line numberDiff line change
@@ -138,6 +138,8 @@ PHP_METHOD(DOMImplementation, createDocument)
138138
RETURN_THROWS();
139139
}
140140
if (doctype->doc != NULL) {
141+
/* As the new document is the context node, and the default for strict error checking
142+
* is true, this will always throw. */
141143
php_dom_throw_error(WRONG_DOCUMENT_ERR, 1);
142144
RETURN_THROWS();
143145
}
@@ -172,7 +174,9 @@ PHP_METHOD(DOMImplementation, createDocument)
172174
if (localname != NULL) {
173175
xmlFree(localname);
174176
}
175-
RETURN_FALSE;
177+
/* See above for strict error checking argument. */
178+
php_dom_throw_error(INVALID_STATE_ERR, /* strict */ true);
179+
RETURN_THROWS();
176180
}
177181

178182
if (doctype != NULL) {
@@ -195,8 +199,7 @@ PHP_METHOD(DOMImplementation, createDocument)
195199
}
196200
xmlFreeDoc(docp);
197201
xmlFree(localname);
198-
/* Need some better type of error here */
199-
php_dom_throw_error(PHP_ERR, 1);
202+
php_dom_throw_error(INVALID_STATE_ERR, /* strict */ true);
200203
RETURN_THROWS();
201204
}
202205

ext/dom/node.c

+8-6
Original file line numberDiff line numberDiff line change
@@ -687,7 +687,8 @@ zend_result dom_node_prefix_write(dom_object *obj, zval *newval)
687687
(nodep->type == XML_ATTRIBUTE_NODE && zend_string_equals_literal(prefix_str, "xmlns") &&
688688
strcmp(strURI, (char *) DOM_XMLNS_NAMESPACE)) ||
689689
(nodep->type == XML_ATTRIBUTE_NODE && !strcmp((char *) nodep->name, "xmlns"))) {
690-
ns = NULL;
690+
php_dom_throw_error(NAMESPACE_ERR, dom_get_strict_error(obj->document));
691+
return FAILURE;
691692
} else {
692693
curns = nsnode->nsDef;
693694
while (curns != NULL) {
@@ -699,14 +700,15 @@ zend_result dom_node_prefix_write(dom_object *obj, zval *newval)
699700
}
700701
if (ns == NULL) {
701702
ns = xmlNewNs(nsnode, nodep->ns->href, (xmlChar *)prefix);
703+
/* Sadly, we cannot distinguish between OOM and namespace conflict.
704+
* But OOM will almost never happen. */
705+
if (UNEXPECTED(ns == NULL)) {
706+
php_dom_throw_error(NAMESPACE_ERR, /* strict */ true);
707+
return FAILURE;
708+
}
702709
}
703710
}
704711

705-
if (ns == NULL) {
706-
php_dom_throw_error(NAMESPACE_ERR, dom_get_strict_error(obj->document));
707-
return FAILURE;
708-
}
709-
710712
xmlSetNs(nodep, ns);
711713
}
712714
break;

ext/dom/php_dom.stub.php

+2-2
Original file line numberDiff line numberDiff line change
@@ -477,8 +477,8 @@ public function hasFeature(string $feature, string $version): bool {}
477477
/** @return DOMDocumentType|false */
478478
public function createDocumentType(string $qualifiedName, string $publicId = "", string $systemId = "") {}
479479

480-
/** @return DOMDocument|false */
481-
public function createDocument(?string $namespace = null, string $qualifiedName = "", ?DOMDocumentType $doctype = null) {}
480+
/** @tentative-return-type */
481+
public function createDocument(?string $namespace = null, string $qualifiedName = "", ?DOMDocumentType $doctype = null): DOMDocument {}
482482
}
483483

484484
/** @alias DOM\DocumentFragment */

ext/dom/php_dom_arginfo.h

+2-2
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

ext/dom/text.c

+2-2
Original file line numberDiff line numberDiff line change
@@ -152,8 +152,8 @@ PHP_METHOD(DOMText, splitText)
152152
xmlFree(second);
153153

154154
if (nnode == NULL) {
155-
/* TODO Add warning? */
156-
RETURN_FALSE;
155+
php_dom_throw_error(INVALID_STATE_ERR, /* strict */ true);
156+
RETURN_THROWS();
157157
}
158158

159159
if (node->parent != NULL) {

0 commit comments

Comments
 (0)