Skip to content

Handle libxml2 OOM more consistently #11927

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

Merged
merged 3 commits into from
Dec 4, 2023
Merged

Handle libxml2 OOM more consistently #11927

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
044ed2c
Handle libxml2 OOM more consistently
nielsdos Aug 9, 2023
7f1c77f
Fixes/Updates
nielsdos Dec 4, 2023
bfe909a
[ci skip] NEWS
nielsdos Dec 4, 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
1 change: 1 addition & 0 deletions NEWS
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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;
Comment on lines +868 to 870
Copy link
Member

Choose a reason for hiding this comment

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

Probably makes sense to rewrite this code prior to the if(errorcode == 0) and use this as a guard as the only other case that seems to fail as far as I can see is on line 862.

Copy link
Member Author

Choose a reason for hiding this comment

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

There's also nsptr = dom_get_ns(nodep, uri, &errorcode, prefix); that has an out-parameter that can set the errorcode. And on line 844 the error code can also be set initially. I think I'm going to keep it as-is because it has a single easy-to-spot error handling block for the errorcode this way.

Copy link
Member

Choose a reason for hiding this comment

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

Right 👍

}

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;
}
Comment on lines 932 to 936
Copy link
Member

Choose a reason for hiding this comment

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

Same comment

Copy link
Member Author

Choose a reason for hiding this comment

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

Same reply as above ^^ :)


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
View file
Open in desktop
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);
Copy link
Member

Choose a reason for hiding this comment

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

This makes it possible to add the tentative DOMDocument return type :)

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
View file
Open in desktop
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)) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Note to self: recheck if OOM is the only way in this path that this can return 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
View file
Open in desktop
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
View file
Open in desktop

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
View file
Open in desktop
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