From 9c306470fb8ccbfb7a35c83c9666cdf91cc64ea0 Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Mon, 4 Dec 2023 22:49:25 +0000 Subject: [PATCH] Handle libxml2 OOM more consistently (#11927) This is a continuation of commit c2a58ab07d, 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] https://github.com/php/doc-en/issues/1741 --- NEWS | 1 + UPGRADING | 5 +++++ ext/dom/document.c | 30 ++++++++++++++---------------- ext/dom/domimplementation.c | 9 ++++++--- ext/dom/node.c | 14 ++++++++------ ext/dom/php_dom.stub.php | 4 ++-- ext/dom/php_dom_arginfo.h | 4 ++-- ext/dom/text.c | 4 ++-- 8 files changed, 40 insertions(+), 31 deletions(-) diff --git a/NEWS b/NEWS index 06ccd818371..68128f21283 100644 --- a/NEWS +++ b/NEWS @@ -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()). diff --git a/UPGRADING b/UPGRADING index b35439e0c2e..2334b1abbf4 100644 --- a/UPGRADING +++ b/UPGRADING @@ -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 diff --git a/ext/dom/document.c b/ext/dom/document.c index 8cdf8ec19b0..e9dc173bb5b 100644 --- a/ext/dom/document.c +++ b/ext/dom/document.c @@ -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); @@ -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 */ @@ -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"); @@ -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 */ diff --git a/ext/dom/domimplementation.c b/ext/dom/domimplementation.c index 64432fc3da3..d771eccad86 100644 --- a/ext/dom/domimplementation.c +++ b/ext/dom/domimplementation.c @@ -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(); } @@ -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) { @@ -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(); } diff --git a/ext/dom/node.c b/ext/dom/node.c index fbf1f327e4d..6d026c69624 100644 --- a/ext/dom/node.c +++ b/ext/dom/node.c @@ -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) { @@ -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; diff --git a/ext/dom/php_dom.stub.php b/ext/dom/php_dom.stub.php index 2bd600e0c6e..dd52d5ca510 100644 --- a/ext/dom/php_dom.stub.php +++ b/ext/dom/php_dom.stub.php @@ -477,8 +477,8 @@ namespace /** @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 */ diff --git a/ext/dom/php_dom_arginfo.h b/ext/dom/php_dom_arginfo.h index 703db8fa818..43de4fee965 100644 --- a/ext/dom/php_dom_arginfo.h +++ b/ext/dom/php_dom_arginfo.h @@ -1,5 +1,5 @@ /* This is a generated file, edit the .stub.php file instead. - * Stub hash: 53f161ae504057211c907938819f6e7f1f4fbfa2 */ + * Stub hash: dc65484181a06009b064b650edbb9d831a56fd7c */ ZEND_BEGIN_ARG_WITH_RETURN_OBJ_INFO_EX(arginfo_dom_import_simplexml, 0, 1, DOMElement, 0) ZEND_ARG_TYPE_INFO(0, node, IS_OBJECT, 0) @@ -144,7 +144,7 @@ ZEND_BEGIN_ARG_INFO_EX(arginfo_class_DOMImplementation_createDocumentType, 0, 0, ZEND_ARG_TYPE_INFO_WITH_DEFAULT_VALUE(0, systemId, IS_STRING, 0, "\"\"") ZEND_END_ARG_INFO() -ZEND_BEGIN_ARG_INFO_EX(arginfo_class_DOMImplementation_createDocument, 0, 0, 0) +ZEND_BEGIN_ARG_WITH_TENTATIVE_RETURN_OBJ_INFO_EX(arginfo_class_DOMImplementation_createDocument, 0, 0, DOMDocument, 0) ZEND_ARG_TYPE_INFO_WITH_DEFAULT_VALUE(0, namespace, IS_STRING, 1, "null") ZEND_ARG_TYPE_INFO_WITH_DEFAULT_VALUE(0, qualifiedName, IS_STRING, 0, "\"\"") ZEND_ARG_OBJ_INFO_WITH_DEFAULT_VALUE(0, doctype, DOMDocumentType, 1, "null") diff --git a/ext/dom/text.c b/ext/dom/text.c index c51240273d8..1f2a6443ff4 100644 --- a/ext/dom/text.c +++ b/ext/dom/text.c @@ -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) {