Skip to content

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

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

Closed
wants to merge 2 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
423320d
Fix validation logic of php:function() callbacks in dom and xsl
nielsdos Nov 2, 2023
8bee075
Fix misplaced label
nielsdos Nov 2, 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
27 changes: 27 additions & 0 deletions ext/dom/tests/php_function_edge_cases.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
8 changes: 7 additions & 1 deletion ext/dom/xpath.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,11 @@ 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;
}

Copy link
Member

Choose a reason for hiding this comment

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

Would the for loop below be more clear as:

for (i = fci.param_count; i > 0; i--) {

?

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed, I'll change it soon

fci.param_count = nargs - 1;
if (fci.param_count > 0) {
fci.params = safe_emalloc(fci.param_count, sizeof(zval), 0);
Expand Down Expand Up @@ -132,7 +137,7 @@ static void dom_xpath_ext_function_php(xmlXPathParserContextPtr ctxt, int nargs,
if (obj->stringval == NULL) {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add a comment that this obj is the last element of the FILO stack and must correspond to a PHP function name?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ack

zend_type_error("Handler name must be a string");
xmlXPathFreeObject(obj);
goto cleanup;
goto cleanup_no_callable;
}
Comment on lines 137 to 141
Copy link
Member

Choose a reason for hiding this comment

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

You should be able to check that the string is indeed a valid callable before allocating the arguments of it.

Moreover, (for master) you could use the named_params field instead of allocating each argument individually, should make clean-up easier as you would just need to destroy the HashTable.

Copy link
Member Author

Choose a reason for hiding this comment

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

You should be able to check that the string is indeed a valid callable before allocating the arguments of it.

Not easy to do, it's a stack machine so the function name comes last. I'd have to store the past arguments anyway so I can't avoid the allocation.

Moreover, (for master) you could use the named_params field instead of allocating each argument individually, should make clean-up easier as you would just need to destroy the HashTable.

TIL. But that will also be slower because of the hashtable management code.

Copy link
Member

Choose a reason for hiding this comment

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

You should be able to check that the string is indeed a valid callable before allocating the arguments of it.

Not easy to do, it's a stack machine so the function name comes last. I'd have to store the past arguments anyway so I can't avoid the allocation.

Okay, that code makes way more sense

ZVAL_STRING(&fci.function_name, (char *) obj->stringval);
xmlXPathFreeObject(obj);
Expand Down Expand Up @@ -177,6 +182,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]);
Expand Down
45 changes: 45 additions & 0 deletions ext/xsl/tests/php_function_edge_cases.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
5 changes: 5 additions & 0 deletions ext/xsl/xsltprocessor.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,11 @@ 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);
Expand Down