Handle libxml2 OOM more consistently #11927

nielsdos · 2023-08-09T20:48:47Z

See php/doc-en#1741 for some background

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

NOTE: since we don't handle all OOM cases, maybe the wording in UPGRADING should be relaxed.

nielsdos · 2023-08-09T20:50:04Z

Looks like this also got rid of the last use of PHP_ERR (which is non-standard, and vague).

nielsdos · 2023-08-09T20:56:24Z

More weirdness: this got rid of the last way to return false from PHP_METHOD(DOMImplementation, createDocument). However, it looks like it should be able to return false because there's still another error condition, but that ignores the strict error property:

php-src/ext/dom/domimplementation.c

Lines 161 to 167 in 620b622

    
           if (errorcode != 0) { 
        
           	if (localname != NULL) { 
        
           		xmlFree(localname); 
        
           	} 
        
           	php_dom_throw_error(errorcode, 1); 
        
           	RETURN_THROWS(); 
        
           }

Notice the hardcoded 1 in php_dom_throw_error. I guess that should be changed, no?

Girgias

LGTM, I suppose the PHP_ERR constant can be added to the deprecation chop board.

Girgias · 2023-08-10T17:31:55Z

ext/dom/document.c

+		xmlFreeNode(nodep);
 		php_dom_throw_error(errorcode, dom_get_strict_error(intern->document));
 		RETURN_FALSE;


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.

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.

Girgias · 2023-08-10T17:32:24Z

ext/dom/document.c

 	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;
 	}


Same comment

Same reply as above ^^ :)

Girgias · 2023-08-10T18:17:27Z

More weirdness: this got rid of the last way to return false from PHP_METHOD(DOMImplementation, createDocument). However, it looks like it should be able to return false because there's still another error condition, but that ignores the strict error property:

php-src/ext/dom/domimplementation.c

Lines 161 to 167 in 620b622

if (errorcode != 0) {

if (localname != NULL) {

xmlFree(localname);

}

php_dom_throw_error(errorcode, 1);

RETURN_THROWS();

}

Notice the hardcoded 1 in php_dom_throw_error. I guess that should be changed, no?

Probably? I need to go I'll have another think

Girgias · 2023-08-10T22:35:56Z

Okay, I think the throw is correct for createDocument().

Looking at the spec, the interesting part is step 3, which says:

If qualifiedName is not the empty string, then set element to the result of running the internal createElementNS steps, given document, namespace, qualifiedName, and an empty dictionary.

And considering the implementation is simply:

The createElementNS(namespace, qualifiedName, options) method steps are to return the result of running the internal createElementNS steps, given this, namespace, qualifiedName, and options.

However, this can only return false if the DOM document sets strictErrorChecking to false, but by default it is true and the empty document is created during the call, so it is impossible to toggle this off.

It seems the strictErrorChecking property is a DOM Core Level 3 feature that has been deprecated and removed from browsers, and I think we should put this on the chopping board ASAP as it would clean up the API quite a lot.

nielsdos · 2023-08-10T23:05:57Z

However, this can only return false if the DOM document sets strictErrorChecking to false, but by default it is true and the empty document is created during the call, so it is impossible to toggle this off.

Hmm. The first step of createElementNS is https://dom.spec.whatwg.org/#validate-and-extract . This specifies things like the conditions for the NAMESPACE_ERR that we throw. It is executed before the creation of the new document in step 4, seemingly within the context of the current document (because we don't have a new one yet, so which other context could it be?). It's a bit unclear though and I'm not convinced my reasoning is right.
Of course, the strict error thing doesn't exist in the live spec. And looking at the DOM3 spec: https://www.w3.org/TR/2003/WD-DOM-Level-3-Core-20030226/DOM3-Core.html#core-Level-2-Core-DOM-createDocument is even more vague. So this doesn't clear things up either.

It seems the strictErrorChecking property is a DOM Core Level 3 feature that has been deprecated and removed from browsers, and I think we should put this on the chopping board ASAP as it would clean up the API quite a lot.

Yeah it would be a lot easier for sure. I'm not sure this is an easy deprecation though, I have no idea about how widely people use this.

As a side note, we must be careful deprecating things that were removed for the live spec. The current live spec basically only takes into account HTML. So XML properties such as xmlVersion and xmlStandalone for example don't exist in the live spec anymore. However, ext/dom supports both HTML and XML documents. Therefore we must keep these things alive such that XML functionality is not lost.

nielsdos · 2023-08-10T23:10:52Z

Sorry, I clearly am not awake anymore. True facepalm moment here.
Your reasoning is right, the context is the new document. Therefore, it must throw. So discard the reasoning above please.
The part I wrote about deprecations still holds though :)

staabm · 2023-08-11T08:12:06Z

Sorry for asking this question in between: reading all these comments I get the feeling DOM could run out-of-memory but PHP would still work (and handle this error) without running out of memory itself?

is PHP Userland code expected to handle a possible DOM out-of-memory error?

Girgias · 2023-08-11T13:30:14Z

Sorry for asking this question in between: reading all these comments I get the feeling DOM could run out-of-memory but PHP would still work (and handle this error) without running out of memory itself?

is PHP Userland code expected to handle a possible DOM out-of-memory error?

In theory yes, as the ext/dom memory usage does not count against the PHP memory limit (due to limitations with libxml not allowing to pass a custom allocator). So an allocation could fail, return false/throw an exception and pass back to PHP if it hasn't reached its own memory limit.

nielsdos · 2023-08-11T15:42:55Z

In theory they should be handled. In practice, if you're OOM in libxml2, then you're probably screwed anyway because the system allocator ran out of memory. Similarly for other libraries that PHP uses under the hood.

The goal is unifying all of the OOM conditions into exceptions is to make the return value semantics easier, and more correct w.r.t. the DOM spec. Exceptions will also make sure you can more easily gracefully shut down the request, which should probably be what happens when you run out of memory.

staabm · 2023-08-11T15:45:50Z

I see thx. the same is true for preg_* then, right?

refs phpstan/phpstan-src#2560 (comment)

nielsdos · 2023-08-11T15:47:36Z

@Girgias do I change the return type of createDocument in this PR? It's with a @return, so IIRC this is a docs-only annotation that does not actually enforce a return type?
The thing is though that I didn't check for other potential spec-compliance issues. Perhaps I should leave the return type as-is and only change it when I (hopefully) do my spec fix work in 8.4?

nielsdos · 2023-08-11T15:49:59Z

I see thx. the same is true for preg_* then, right?

refs phpstan/phpstan-src#2560 (comment)

That memory is also allocated via the system allocator, so you're probably screwed there too if you run OOM.
The reason we can change it in ext/dom to an exception is because we follow a specification in ext/dom, whereas we don't have a specification of how the methods should look like in ext/pcre.

Girgias · 2023-08-11T16:48:34Z

@Girgias do I change the return type of createDocument in this PR? It's with a @return, so IIRC this is a docs-only annotation that does not actually enforce a return type? The thing is though that I didn't check for other potential spec-compliance issues. Perhaps I should leave the return type as-is and only change it when I (hopefully) do my spec fix work in 8.4?

Would not make this change and leave it for later as currently it's not even a tentative type. But maybe @kocsismate can clarify?

nielsdos · 2023-10-13T17:16:41Z

ext/dom/node.c

@@ -693,14 +694,13 @@ int dom_node_prefix_write(dom_object *obj, zval *newval)
 					}
 					if (ns == NULL) {
 						ns = xmlNewNs(nsnode, nodep->ns->href, (xmlChar *)prefix);
+						if (UNEXPECTED(ns == NULL)) {


Note to self: recheck if OOM is the only way in this path that this can return NULL.

kocsismate · 2023-10-15T17:01:21Z

ext/dom/domimplementation.c

@@ -172,7 +172,8 @@ PHP_METHOD(DOMImplementation, createDocument)
 		if (localname != NULL) {
 			xmlFree(localname);
 		}
-		RETURN_FALSE;
+		php_dom_throw_error(INVALID_STATE_ERR, /* strict */ true);


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

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

github-actions bot added the Extension: dom label Aug 9, 2023

Girgias approved these changes Aug 10, 2023

View reviewed changes

staabm mentioned this pull request Aug 12, 2023

Implement PregGrepDynamicReturnTypeExtension phpstan/phpstan-src#2560

Closed

nielsdos commented Oct 13, 2023

View reviewed changes

kocsismate reviewed Oct 15, 2023

View reviewed changes

nielsdos added 2 commits December 4, 2023 22:46

Fixes/Updates

7f1c77f

nielsdos force-pushed the dom-oom branch from 2d008ae to 7f1c77f Compare December 4, 2023 22:10

[ci skip] NEWS

bfe909a

nielsdos merged commit 9c30647 into php:master Dec 4, 2023

xabbuh mentioned this pull request Jul 23, 2024

[ErrorHandler][VarDumper] Remove PHP 8.4 deprecations symfony/symfony#57799

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle libxml2 OOM more consistently #11927

Handle libxml2 OOM more consistently #11927

nielsdos commented Aug 9, 2023

nielsdos commented Aug 9, 2023

nielsdos commented Aug 9, 2023 •

edited

Loading

Girgias left a comment

Girgias Aug 10, 2023

nielsdos Aug 10, 2023

Girgias Aug 10, 2023

Girgias Aug 10, 2023

nielsdos Aug 10, 2023

Girgias commented Aug 10, 2023

Girgias commented Aug 10, 2023

nielsdos commented Aug 10, 2023

nielsdos commented Aug 10, 2023

staabm commented Aug 11, 2023

Girgias commented Aug 11, 2023

nielsdos commented Aug 11, 2023

staabm commented Aug 11, 2023

nielsdos commented Aug 11, 2023

nielsdos commented Aug 11, 2023

Girgias commented Aug 11, 2023

nielsdos Oct 13, 2023

kocsismate Oct 15, 2023

Handle libxml2 OOM more consistently #11927

Handle libxml2 OOM more consistently #11927

Conversation

nielsdos commented Aug 9, 2023

nielsdos commented Aug 9, 2023

nielsdos commented Aug 9, 2023 • edited Loading

Girgias left a comment

Choose a reason for hiding this comment

Girgias Aug 10, 2023

Choose a reason for hiding this comment

nielsdos Aug 10, 2023

Choose a reason for hiding this comment

Girgias Aug 10, 2023

Choose a reason for hiding this comment

Girgias Aug 10, 2023

Choose a reason for hiding this comment

nielsdos Aug 10, 2023

Choose a reason for hiding this comment

Girgias commented Aug 10, 2023

Girgias commented Aug 10, 2023

nielsdos commented Aug 10, 2023

nielsdos commented Aug 10, 2023

staabm commented Aug 11, 2023

Girgias commented Aug 11, 2023

nielsdos commented Aug 11, 2023

staabm commented Aug 11, 2023

nielsdos commented Aug 11, 2023

nielsdos commented Aug 11, 2023

Girgias commented Aug 11, 2023

nielsdos Oct 13, 2023

Choose a reason for hiding this comment

kocsismate Oct 15, 2023

Choose a reason for hiding this comment

nielsdos commented Aug 9, 2023 •

edited

Loading