Skip to content

Commit 2d008ae

Browse files
committed
Handle libxml2 OOM more consistently
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 620b622 commit 2d008ae

File tree

5 files changed

+28
-27
lines changed

5 files changed

+28
-27
lines changed

UPGRADING

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,9 @@ PHP 8.3 UPGRADE NOTES
5353
. Using the DOMParentNode and DOMChildNode methods without a document now works
5454
instead of throwing a HIERARCHY_REQUEST_ERR DOMException. This is in line with
5555
the behaviour spec demands.
56+
. Some DOM methods previously returned false or a PHP_ERR DOMException if a new
57+
node could not be allocated.
58+
They consistently throw an INVALID_STATE_ERR DOMException now.
5659

5760
- FFI:
5861
. C functions that have a return type of void now return null instead of

ext/dom/document.c

Lines changed: 14 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -846,7 +846,12 @@ PHP_METHOD(DOMDocument, createElementNS)
846846
if (errorcode == 0) {
847847
if (xmlValidateName((xmlChar *) localname, 0) == 0) {
848848
nodep = xmlNewDocNode(docp, NULL, (xmlChar *) localname, (xmlChar *) value);
849-
if (nodep != NULL && uri != NULL) {
849+
if (UNEXPECTED(nodep == NULL)) {
850+
php_dom_throw_error(INVALID_STATE_ERR, /* strict */ true);
851+
RETURN_THROWS();
852+
}
853+
854+
if (uri != NULL) {
850855
xmlNsPtr nsptr = xmlSearchNsByHref(nodep->doc, nodep, (xmlChar *) uri);
851856
if (nsptr == NULL) {
852857
nsptr = dom_get_ns(nodep, uri, &errorcode, prefix);
@@ -864,17 +869,11 @@ PHP_METHOD(DOMDocument, createElementNS)
864869
}
865870

866871
if (errorcode != 0) {
867-
if (nodep != NULL) {
868-
xmlFreeNode(nodep);
869-
}
872+
xmlFreeNode(nodep);
870873
php_dom_throw_error(errorcode, dom_get_strict_error(intern->document));
871874
RETURN_FALSE;
872875
}
873876

874-
if (nodep == NULL) {
875-
RETURN_FALSE;
876-
}
877-
878877
DOM_RET_OBJ(nodep, &ret, intern);
879878
}
880879
/* }}} end dom_document_create_element_ns */
@@ -908,7 +907,12 @@ PHP_METHOD(DOMDocument, createAttributeNS)
908907
if (errorcode == 0) {
909908
if (xmlValidateName((xmlChar *) localname, 0) == 0) {
910909
nodep = (xmlNodePtr) xmlNewDocProp(docp, (xmlChar *) localname, NULL);
911-
if (nodep != NULL && uri_len > 0) {
910+
if (UNEXPECTED(nodep == NULL)) {
911+
php_dom_throw_error(INVALID_STATE_ERR, /* strict */ true);
912+
RETURN_THROWS();
913+
}
914+
915+
if (uri_len > 0) {
912916
nsptr = xmlSearchNsByHref(nodep->doc, root, (xmlChar *) uri);
913917
if (nsptr == NULL) {
914918
nsptr = dom_get_ns(root, uri, &errorcode, prefix);
@@ -930,17 +934,11 @@ PHP_METHOD(DOMDocument, createAttributeNS)
930934
}
931935

932936
if (errorcode != 0) {
933-
if (nodep != NULL) {
934-
xmlFreeProp((xmlAttrPtr) nodep);
935-
}
937+
xmlFreeProp((xmlAttrPtr) nodep);
936938
php_dom_throw_error(errorcode, dom_get_strict_error(intern->document));
937939
RETURN_FALSE;
938940
}
939941

940-
if (nodep == NULL) {
941-
RETURN_FALSE;
942-
}
943-
944942
DOM_RET_OBJ(nodep, &ret, intern);
945943
}
946944
/* }}} end dom_document_create_attribute_ns */

ext/dom/domimplementation.c

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -172,7 +172,8 @@ PHP_METHOD(DOMImplementation, createDocument)
172172
if (localname != NULL) {
173173
xmlFree(localname);
174174
}
175-
RETURN_FALSE;
175+
php_dom_throw_error(INVALID_STATE_ERR, /* strict */ true);
176+
RETURN_THROWS();
176177
}
177178

178179
if (doctype != NULL) {
@@ -195,8 +196,7 @@ PHP_METHOD(DOMImplementation, createDocument)
195196
}
196197
xmlFreeDoc(docp);
197198
xmlFree(localname);
198-
/* Need some better type of error here */
199-
php_dom_throw_error(PHP_ERR, 1);
199+
php_dom_throw_error(INVALID_STATE_ERR, /* strict */ true);
200200
RETURN_THROWS();
201201
}
202202

ext/dom/node.c

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -681,7 +681,8 @@ int dom_node_prefix_write(dom_object *obj, zval *newval)
681681
(nodep->type == XML_ATTRIBUTE_NODE && zend_string_equals_literal(prefix_str, "xmlns") &&
682682
strcmp(strURI, (char *) DOM_XMLNS_NAMESPACE)) ||
683683
(nodep->type == XML_ATTRIBUTE_NODE && !strcmp((char *) nodep->name, "xmlns"))) {
684-
ns = NULL;
684+
php_dom_throw_error(NAMESPACE_ERR, dom_get_strict_error(obj->document));
685+
return FAILURE;
685686
} else {
686687
curns = nsnode->nsDef;
687688
while (curns != NULL) {
@@ -693,14 +694,13 @@ int dom_node_prefix_write(dom_object *obj, zval *newval)
693694
}
694695
if (ns == NULL) {
695696
ns = xmlNewNs(nsnode, nodep->ns->href, (xmlChar *)prefix);
697+
if (UNEXPECTED(ns == NULL)) {
698+
php_dom_throw_error(INVALID_STATE_ERR, /* strict */ true);
699+
return FAILURE;
700+
}
696701
}
697702
}
698703

699-
if (ns == NULL) {
700-
php_dom_throw_error(NAMESPACE_ERR, dom_get_strict_error(obj->document));
701-
return FAILURE;
702-
}
703-
704704
xmlSetNs(nodep, ns);
705705
}
706706
break;

ext/dom/text.c

Lines changed: 2 additions & 2 deletions
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)