Fix validation logic of php:function() callbacks in dom and xsl

Two issues:
- Assumed that at least 1 argument (function name) was provided.
- Incorrect error path for the non-callable case.

Closes GH-12593.

Author: nielsdos

NEWS

@@ -11,6 +11,7 @@ PHP                                                                        NEWS
 - DOM:
   . Fix registerNodeClass with abstract class crashing. (nielsdos)
   . Add missing NULL pointer error check. (icy17)
+  . Fix validation logic of php:function() callbacks. (nielsdos)
 
 - Fiber:
   . Fixed bug GH-11121 (ReflectionFiber segfault). (danog, trowski, bwoebi)
@@ -60,6 +61,7 @@ PHP                                                                        NEWS
 
 - XSL:
   . Add missing module dependency. (nielsdos)
+  . Fix validation logic of php:function() callbacks. (nielsdos)
 
 26 Oct 2023, PHP 8.1.25
 

ext/dom/tests/php_function_edge_cases.phpt

@@ -0,0 +1,27 @@
+--TEST--
+php:function() edge cases
+--EXTENSIONS--
+dom
+--FILE--
+<?php
+
+$doc = new DOMDocument();
+$doc->loadHTML('<a href="https://php.net">hello</a>');
+$xpath = new DOMXpath($doc);
+$xpath->registerNamespace("php", "http://php.net/xpath");
+$xpath->registerPHPFunctions();
+try {
+    $xpath->query("//a[php:function(3)]");
+} catch (TypeError $e) {
+    echo $e->getMessage(), "\n";
+}
+try {
+    $xpath->query("//a[php:function()]");
+} catch (Throwable $e) {
+    echo $e->getMessage(), "\n";
+}
+
+?>
+--EXPECT--
+Handler name must be a string
+Function name must be passed as the first argument

ext/dom/xpath.c

@@ -70,12 +70,17 @@ static void dom_xpath_ext_function_php(xmlXPathParserContextPtr ctxt, int nargs,
 		return;
 	}
 
+	if (UNEXPECTED(nargs == 0)) {
+		zend_throw_error(NULL, "Function name must be passed as the first argument");
+		return;
+	}
+
 	fci.param_count = nargs - 1;
 	if (fci.param_count > 0) {
 		fci.params = safe_emalloc(fci.param_count, sizeof(zval), 0);
 	}
 	/* Reverse order to pop values off ctxt stack */
-	for (i = nargs - 2; i >= 0; i--) {
+	for (i = fci.param_count - 1; i >= 0; i--) {
 		obj = valuePop(ctxt);
 		switch (obj->type) {
 			case XPATH_STRING:
@@ -128,11 +133,12 @@ static void dom_xpath_ext_function_php(xmlXPathParserContextPtr ctxt, int nargs,
 
 	fci.size = sizeof(fci);
 
+	/* Last element of the stack is the function name */
 	obj = valuePop(ctxt);
 	if (obj->stringval == NULL) {
 		zend_type_error("Handler name must be a string");
 		xmlXPathFreeObject(obj);
-		goto cleanup;
+		goto cleanup_no_callable;
 	}
 	ZVAL_STRING(&fci.function_name, (char *) obj->stringval);
 	xmlXPathFreeObject(obj);
@@ -177,6 +183,7 @@ static void dom_xpath_ext_function_php(xmlXPathParserContextPtr ctxt, int nargs,
 cleanup:
 	zend_string_release_ex(callable, 0);
 	zval_ptr_dtor_nogc(&fci.function_name);
+cleanup_no_callable:
 	if (fci.param_count > 0) {
 		for (i = 0; i < nargs - 1; i++) {
 			zval_ptr_dtor(&fci.params[i]);

ext/xsl/tests/php_function_edge_cases.phpt

@@ -0,0 +1,45 @@
+--TEST--
+php:function() edge cases
+--EXTENSIONS--
+xsl
+--FILE--
+<?php
+
+function test($input) {
+    $xsl = new DomDocument();
+    $xsl->loadXML('<?xml version="1.0" encoding="iso-8859-1" ?>
+    <xsl:stylesheet version="1.0"
+    xmlns:xsl="http://www.w3.org/1999/XSL/Transform"
+    xmlns:php="http://php.net/xsl">
+    <xsl:template match="/">
+    <xsl:value-of select="php:function(' . $input . ')" />
+    </xsl:template>
+    </xsl:stylesheet>');
+
+    $inputdom = new DomDocument();
+    $inputdom->loadXML('<?xml version="1.0" encoding="iso-8859-1" ?>
+    <today></today>');
+
+    $proc = new XsltProcessor();
+    $proc->registerPhpFunctions();
+    $xsl = $proc->importStylesheet($xsl);
+    try {
+        $proc->transformToDoc($inputdom);
+    } catch (Exception $e) {
+        echo $e->getMessage(), "\n";
+    }
+}
+
+try {
+    test("");
+} catch (Throwable $e) {
+    echo $e->getMessage(), "\n";
+}
+
+test("3");
+
+?>
+--EXPECTF--
+Function name must be passed as the first argument
+
+Warning: XSLTProcessor::transformToDoc(): Handler name must be a string in %s on line %d

ext/xsl/xsltprocessor.c

@@ -141,12 +141,17 @@ static void xsl_ext_function_php(xmlXPathParserContextPtr ctxt, int nargs, int t
 		return;
 	}
 
+	if (UNEXPECTED(nargs == 0)) {
+		zend_throw_error(NULL, "Function name must be passed as the first argument");
+		return;
+	}
+
 	fci.param_count = nargs - 1;
 	if (fci.param_count > 0) {
 		args = safe_emalloc(fci.param_count, sizeof(zval), 0);
 	}
 	/* Reverse order to pop values off ctxt stack */
-	for (i = nargs - 2; i >= 0; i--) {
+	for (i = fci.param_count - 1; i >= 0; i--) {
 		obj = valuePop(ctxt);
 		if (obj == NULL) {
 			ZVAL_NULL(&args[i]);
@@ -221,7 +226,7 @@ static void xsl_ext_function_php(xmlXPathParserContextPtr ctxt, int nargs, int t
 		fci.params = NULL;
 	}
 
-
+	/* Last element of the stack is the function name */
 	obj = valuePop(ctxt);
 	if (obj == NULL || obj->stringval == NULL) {
 		php_error_docref(NULL, E_WARNING, "Handler name must be a string");