Skip to content

Commit

Permalink
Handle libxml2 OOM more consistently (#11927)
Browse files Browse the repository at this point in the history
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
  • Loading branch information
nielsdos authored Dec 4, 2023
1 parent 2cd8f3e commit 9c30647
Show file tree
Hide file tree
Showing 8 changed files with 40 additions and 31 deletions.
1 change: 1 addition & 0 deletions NEWS
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ DOM:
. Fix cloning attribute with namespace disappearing namespace. (nielsdos)
. Implement DOM HTML5 parsing and serialization RFC. (nielsdos)
. Fix DOMElement->prefix with empty string creates bogus prefix. (nielsdos)
. Handle OOM more consistently. (nielsdos)

FPM:
. Implement GH-12385 (flush headers without body when calling flush()).
Expand Down
5 changes: 5 additions & 0 deletions UPGRADING
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,11 @@ PHP 8.4 UPGRADE NOTES
you might encounter errors if the declaration is incompatible.
Consult sections 2. New Features and 6. New Functions for a list of
newly implemented methods and constants.
. Some DOM methods previously returned false or a PHP_ERR DOMException if a new
node could not be allocated. They consistently throw an INVALID_STATE_ERR
DOMException now. This situation is extremely unlikely though and probably
will not affect you. As a result DOMImplementation::createDocument() now has
a tentative return type of DOMDocument instead of DOMDocument|false.

- PDO_DBLIB:
. setAttribute, DBLIB_ATTR_STRINGIFY_UNIQUEIDENTIFIER and DBLIB_ATTR_DATETIME_CONVERT
Expand Down
30 changes: 14 additions & 16 deletions ext/dom/document.c
Original file line number Diff line number Diff line change
Expand Up @@ -842,7 +842,12 @@ PHP_METHOD(DOM_Document, createElementNS)
if (errorcode == 0) {
if (xmlValidateName((xmlChar *) localname, 0) == 0) {
nodep = xmlNewDocNode(docp, NULL, (xmlChar *) localname, (xmlChar *) value);
if (nodep != NULL && uri != NULL) {
if (UNEXPECTED(nodep == NULL)) {
php_dom_throw_error(INVALID_STATE_ERR, /* strict */ true);
RETURN_THROWS();
}

if (uri != NULL) {
xmlNsPtr nsptr = xmlSearchNsByHref(nodep->doc, nodep, (xmlChar *) uri);
if (nsptr == NULL) {
nsptr = dom_get_ns(nodep, uri, &errorcode, prefix);
Expand All @@ -860,17 +865,11 @@ PHP_METHOD(DOM_Document, createElementNS)
}

if (errorcode != 0) {
if (nodep != NULL) {
xmlFreeNode(nodep);
}
xmlFreeNode(nodep);
php_dom_throw_error(errorcode, dom_get_strict_error(intern->document));
RETURN_FALSE;
}

if (nodep == NULL) {
RETURN_FALSE;
}

DOM_RET_OBJ(nodep, &ret, intern);
}
/* }}} end dom_document_create_element_ns */
Expand Down Expand Up @@ -904,7 +903,12 @@ PHP_METHOD(DOM_Document, createAttributeNS)
if (errorcode == 0) {
if (xmlValidateName((xmlChar *) localname, 0) == 0) {
nodep = (xmlNodePtr) xmlNewDocProp(docp, (xmlChar *) localname, NULL);
if (nodep != NULL && uri_len > 0) {
if (UNEXPECTED(nodep == NULL)) {
php_dom_throw_error(INVALID_STATE_ERR, /* strict */ true);
RETURN_THROWS();
}

if (uri_len > 0) {
nsptr = xmlSearchNsByHref(nodep->doc, root, (xmlChar *) uri);
if (nsptr == NULL || nsptr->prefix == NULL) {
nsptr = dom_get_ns(root, uri, &errorcode, prefix ? prefix : "default");
Expand All @@ -926,17 +930,11 @@ PHP_METHOD(DOM_Document, createAttributeNS)
}

if (errorcode != 0) {
if (nodep != NULL) {
xmlFreeProp((xmlAttrPtr) nodep);
}
xmlFreeProp((xmlAttrPtr) nodep);
php_dom_throw_error(errorcode, dom_get_strict_error(intern->document));
RETURN_FALSE;
}

if (nodep == NULL) {
RETURN_FALSE;
}

DOM_RET_OBJ(nodep, &ret, intern);
}
/* }}} end dom_document_create_attribute_ns */
Expand Down
9 changes: 6 additions & 3 deletions ext/dom/domimplementation.c
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,8 @@ PHP_METHOD(DOMImplementation, createDocument)
RETURN_THROWS();
}
if (doctype->doc != NULL) {
/* As the new document is the context node, and the default for strict error checking
* is true, this will always throw. */
php_dom_throw_error(WRONG_DOCUMENT_ERR, 1);
RETURN_THROWS();
}
Expand Down Expand Up @@ -172,7 +174,9 @@ PHP_METHOD(DOMImplementation, createDocument)
if (localname != NULL) {
xmlFree(localname);
}
RETURN_FALSE;
/* See above for strict error checking argument. */
php_dom_throw_error(INVALID_STATE_ERR, /* strict */ true);
RETURN_THROWS();
}

if (doctype != NULL) {
Expand All @@ -195,8 +199,7 @@ PHP_METHOD(DOMImplementation, createDocument)
}
xmlFreeDoc(docp);
xmlFree(localname);
/* Need some better type of error here */
php_dom_throw_error(PHP_ERR, 1);
php_dom_throw_error(INVALID_STATE_ERR, /* strict */ true);
RETURN_THROWS();
}

Expand Down
14 changes: 8 additions & 6 deletions ext/dom/node.c
Original file line number Diff line number Diff line change
Expand Up @@ -687,7 +687,8 @@ zend_result dom_node_prefix_write(dom_object *obj, zval *newval)
(nodep->type == XML_ATTRIBUTE_NODE && zend_string_equals_literal(prefix_str, "xmlns") &&
strcmp(strURI, (char *) DOM_XMLNS_NAMESPACE)) ||
(nodep->type == XML_ATTRIBUTE_NODE && !strcmp((char *) nodep->name, "xmlns"))) {
ns = NULL;
php_dom_throw_error(NAMESPACE_ERR, dom_get_strict_error(obj->document));
return FAILURE;
} else {
curns = nsnode->nsDef;
while (curns != NULL) {
Expand All @@ -699,14 +700,15 @@ zend_result dom_node_prefix_write(dom_object *obj, zval *newval)
}
if (ns == NULL) {
ns = xmlNewNs(nsnode, nodep->ns->href, (xmlChar *)prefix);
/* Sadly, we cannot distinguish between OOM and namespace conflict.
* But OOM will almost never happen. */
if (UNEXPECTED(ns == NULL)) {
php_dom_throw_error(NAMESPACE_ERR, /* strict */ true);
return FAILURE;
}
}
}

if (ns == NULL) {
php_dom_throw_error(NAMESPACE_ERR, dom_get_strict_error(obj->document));
return FAILURE;
}

xmlSetNs(nodep, ns);
}
break;
Expand Down
4 changes: 2 additions & 2 deletions ext/dom/php_dom.stub.php
Original file line number Diff line number Diff line change
Expand Up @@ -477,8 +477,8 @@ public function hasFeature(string $feature, string $version): bool {}
/** @return DOMDocumentType|false */
public function createDocumentType(string $qualifiedName, string $publicId = "", string $systemId = "") {}

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

/** @alias DOM\DocumentFragment */
Expand Down
4 changes: 2 additions & 2 deletions ext/dom/php_dom_arginfo.h

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions ext/dom/text.c
Original file line number Diff line number Diff line change
Expand Up @@ -152,8 +152,8 @@ PHP_METHOD(DOMText, splitText)
xmlFree(second);

if (nnode == NULL) {
/* TODO Add warning? */
RETURN_FALSE;
php_dom_throw_error(INVALID_STATE_ERR, /* strict */ true);
RETURN_THROWS();
}

if (node->parent != NULL) {
Expand Down

0 comments on commit 9c30647

Please sign in to comment.