Skip to content

Implement request #71571: XSLT processor should provide option to change maxDepth #13731

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
Mar 31, 2024
Merged

Implement request #71571: XSLT processor should provide option to change maxDepth #13731

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a958f8e
Implement request #71571: XSLT processor should provide option to cha…
nielsdos Mar 16, 2024
ce10587
Improve error message
nielsdos Mar 18, 2024
b18b1b3
Block indirect modifications of validated properties
nielsdos Mar 18, 2024
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
2 changes: 2 additions & 0 deletions NEWS
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,5 +224,7 @@ PHP NEWS
. Implement request #64137 (XSLTProcessor::setParameter() should allow both
quotes to be used). (nielsdos)
. Implemented "Improve callbacks in ext/dom and ext/xsl" RFC. (nielsdos)
. Added XSLTProcessor::$maxTemplateDepth and XSLTProcessor::$maxTemplateVars.
(nielsdos)

<<< NOTE: Insert NEWS from last stable release here prior to actual release! >>>
2 changes: 2 additions & 0 deletions UPGRADING
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -259,6 +259,8 @@ PHP 8.4 UPGRADE NOTES
quotes.
. It is now possible to pass any callable to registerPhpFunctions().
RFC: https://wiki.php.net/rfc/improve_callbacks_dom_and_xsl
. Added XSLTProcessor::$maxTemplateDepth and XSLTProcessor::$maxTemplateVars
to control the recursion depth of XSL template evaluation.

========================================
3. Changes in SAPI modules
Expand Down
5 changes: 3 additions & 2 deletions ext/dom/xpath_callbacks.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,8 +67,9 @@ PHP_DOM_EXPORT void php_dom_xpath_callbacks_clean_argument_stack(xmlXPathParserC
xmlXPathFreeObject(obj);
}

/* Push sentinel value */
valuePush(ctxt, xmlXPathNewString((const xmlChar *) ""));
/* Don't push a sentinel value here. If this is called from an error situation, then by *not* pushing a sentinel
* the execution will halt. If this is called from a regular situation, then it is the caller's responsibility
* to ensure the stack remains balanced. */
}

PHP_DOM_EXPORT void php_dom_xpath_callbacks_dtor(php_dom_xpath_callbacks *registry)
Expand Down
60 changes: 60 additions & 0 deletions ext/xsl/php_xsl.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,10 +118,69 @@ zend_object *xsl_objects_new(zend_class_entry *class_type)
intern->parameter = zend_new_array(0);
php_dom_xpath_callbacks_ctor(&intern->xpath_callbacks);

/* Default initialize properties that could not be default initialized at the stub because they depend on library
* configuration parameters. */
ZVAL_LONG(xsl_prop_max_template_depth(&intern->std), xsltMaxDepth);
ZVAL_LONG(xsl_prop_max_template_vars(&intern->std), xsltMaxVars);

return &intern->std;
}
/* }}} */

#if ZEND_DEBUG
# define XSL_DEFINE_PROP_ACCESSOR(c_name, php_name, prop_index) \
zval *xsl_prop_##c_name(zend_object *object) \
{ \
zend_string *prop_name = ZSTR_INIT_LITERAL(php_name, false); \
const zend_property_info *prop_info = zend_get_property_info(xsl_xsltprocessor_class_entry, prop_name, 0); \
zend_string_release_ex(prop_name, false); \
ZEND_ASSERT(OBJ_PROP_TO_NUM(prop_info->offset) == prop_index); \
return OBJ_PROP_NUM(object, prop_index); \
}
#else
# define XSL_DEFINE_PROP_ACCESSOR(c_name, php_name, prop_index) \
zval *xsl_prop_##c_name(zend_object *object) \
{ \
return OBJ_PROP_NUM(object, prop_index); \
}
#endif

XSL_DEFINE_PROP_ACCESSOR(max_template_depth, "maxTemplateDepth", 2)
XSL_DEFINE_PROP_ACCESSOR(max_template_vars, "maxTemplateVars", 3)

static zval *xsl_objects_write_property_with_validation(zend_object *object, zend_string *member, zval *value, void **cache_slot, zval *property)
{
/* Read old value so we can restore it if necessary. The value is not refcounted as its type is IS_LONG. */
ZEND_ASSERT(Z_TYPE_P(property) == IS_LONG);
zend_long old_property_value = Z_LVAL_P(property);

/* Write new property, which will also potentially perform coercions. */
zend_std_write_property(object, member, value, cache_slot);

/* Validate value *after* coercions have been performed, and restore the old value if necessary. */
if (UNEXPECTED(Z_LVAL_P(property) < 0)) {
Z_LVAL_P(property) = old_property_value;
zend_value_error("%s::$%s must be greater than or equal to 0", ZSTR_VAL(object->ce->name), ZSTR_VAL(member));
return &EG(error_zval);
}

return property;
}

static zval *xsl_objects_write_property(zend_object *object, zend_string *member, zval *value, void **cache_slot)
{
/* Extra validation for maxTemplateDepth and maxTemplateVars */
if (zend_string_equals_literal(member, "maxTemplateDepth")) {
zval *property = xsl_prop_max_template_depth(object);
return xsl_objects_write_property_with_validation(object, member, value, cache_slot, property);
} else if (zend_string_equals_literal(member, "maxTemplateVars")) {
zval *property = xsl_prop_max_template_vars(object);
return xsl_objects_write_property_with_validation(object, member, value, cache_slot, property);
} else {
return zend_std_write_property(object, member, value, cache_slot);
}
}

/* {{{ PHP_MINIT_FUNCTION */
PHP_MINIT_FUNCTION(xsl)
{
Expand All @@ -130,6 +189,7 @@ PHP_MINIT_FUNCTION(xsl)
xsl_object_handlers.clone_obj = NULL;
xsl_object_handlers.free_obj = xsl_objects_free_storage;
xsl_object_handlers.get_gc = xsl_objects_get_gc;
xsl_object_handlers.write_property = xsl_objects_write_property;
Copy link
Member

@arnaud-lb arnaud-lb Mar 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I love that you are using std properties, but unfortunately you need to override other handlers to really prevent non-validated mutations:

Override .unset_property, .get_property_ptr_ptr, and .read_property to prevent unset(), $obj->prop = &$ref, and $ref = &$obj->prop.
Something like this should work:

zval *xsl_std_get_property_ptr_ptr(zend_object *zobj, zend_string *name, int type, void **cache_slot)
{
	return NULL;
}

zval *xsl_std_read_property(zend_object *zobj, zend_string *name, int type, void **cache_slot, zval *rv)
{
	/* read handler is being called as a fallback after get_property_ptr_ptr returned NULL */
	if (type != BP_VAR_IS && type != BP_VAR_R && is maxTemplateDepth or maxTemplateVars) {
		zend_throw_error(NULL, "indirect modification");
		return &EG(uninitialized_zval);
	}

	return zend_std_read_property(zobj, name, type, cache_slot, rv);
}

void xsl_std_unset_property(zend_object *object, zend_string *member, void **cache_slot)
{
	if (is maxTemplateDepth or maxTemplateVars) {
		zend_throw_error(NULL, "unset");
		return;
	}

	zend_std_unset_property(object, member, cache_slot);
}

Also, the engine may skip handlers once cache_slot has been filled:

foreach ([1,-1] as $value) {
    $proc->maxTemplateDepth = $value; // second iteration skips the handler
}

so you need to clear it in xsl_objects_write_property_with_validation with CACHE_PTR_EX(cache_slot, NULL); after zend_std_write_property().

Copy link
Member Author

Choose a reason for hiding this comment

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

Right of course, can't believe I missed this. Thanks for pointing this out!
On the one hand, it does make me wonder if all the extra code is "worth it", or if we should just throw at evaluation time. On the other hand I like to get an error as soon as I do something wrong, so I still prefer this from a UX PoV.

Copy link
Member

Choose a reason for hiding this comment

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

I agree. That's a perfect use-case for hooks, and if they pass it may be possible to replace the custom handlers by a hook!


xsl_xsltprocessor_class_entry = register_class_XSLTProcessor();
xsl_xsltprocessor_class_entry->create_object = xsl_objects_new;
Expand Down
3 changes: 3 additions & 0 deletions ext/xsl/php_xsl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,9 @@ void xsl_objects_free_storage(zend_object *object);
void xsl_ext_function_string_php(xmlXPathParserContextPtr ctxt, int nargs);
void xsl_ext_function_object_php(xmlXPathParserContextPtr ctxt, int nargs);

zval *xsl_prop_max_template_depth(zend_object *object);
zval *xsl_prop_max_template_vars(zend_object *object);

PHP_MINIT_FUNCTION(xsl);
PHP_MSHUTDOWN_FUNCTION(xsl);
PHP_RINIT_FUNCTION(xsl);
Expand Down
4 changes: 4 additions & 0 deletions ext/xsl/php_xsl.stub.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,10 @@ class XSLTProcessor

public bool $cloneDocument = false;

public int $maxTemplateDepth;

public int $maxTemplateVars;

/**
* @param DOMDocument|DOM\Document|SimpleXMLElement $stylesheet
* @tentative-return-type
Expand Down
14 changes: 13 additions & 1 deletion ext/xsl/php_xsl_arginfo.h
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

48 changes: 48 additions & 0 deletions ext/xsl/tests/bug71571_a.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
--TEST--
Request #71571 (XSLT processor should provide option to change maxDepth) - variant A
--EXTENSIONS--
xsl
--INI--
error_reporting=E_ALL
--FILE--
<?php

$myxsl = <<<'EOF'
<?xml version="1.0" encoding="UTF-8"?>
<xsl:stylesheet version="1.0" xmlns:xsl="http://www.w3.org/1999/XSL/Transform">
<xsl:template match="/">
<xsl:call-template name="recurse"/>
</xsl:template>

<xsl:template name="recurse">
<xsl:call-template name="recurse"/>
</xsl:template>
</xsl:stylesheet>
EOF;

$xsl = new DOMDocument();
$xsl->loadXML($myxsl);

$doc = new DOMDocument();

$proc = new XSLTProcessor;
$proc->maxTemplateDepth = 2;
$proc->importStyleSheet($xsl);
$proc->transformToDoc($doc);

?>
--EXPECTF--
Warning: XSLTProcessor::transformToDoc(): runtime error: file %s line 8 element call-template in %s on line %d

Warning: XSLTProcessor::transformToDoc(): xsltApplySequenceConstructor: A potential infinite template recursion was detected.
You can adjust xsltMaxDepth (--maxdepth) in order to raise the maximum number of nested template calls and variables/params (currently set to 2). in %s on line %d

Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to change this error message? It would be nice to refer to maxTemplateDepth here

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, done that now, albeit a bit ugly code.

Warning: XSLTProcessor::transformToDoc(): Templates: in %s on line %d

Warning: XSLTProcessor::transformToDoc(): #0 name recurse in %s on line %d

Warning: XSLTProcessor::transformToDoc(): #1 name recurse in %s on line %d

Warning: XSLTProcessor::transformToDoc(): #2 name / in %s on line %d

Warning: XSLTProcessor::transformToDoc(): Variables: in %s on line %d
61 changes: 61 additions & 0 deletions ext/xsl/tests/bug71571_b.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
--TEST--
Request #71571 (XSLT processor should provide option to change maxDepth) - variant B
--EXTENSIONS--
xsl
--INI--
error_reporting=E_ALL
--FILE--
<?php

$myxsl = <<<'EOF'
<?xml version="1.0" encoding="UTF-8"?>
<xsl:stylesheet version="1.0" xmlns:xsl="http://www.w3.org/1999/XSL/Transform">
<xsl:template match="/">
<xsl:call-template name="recurse"/>
</xsl:template>

<xsl:template name="recurse">
<xsl:param name="COUNT">1</xsl:param>
<xsl:call-template name="recurse">
<xsl:with-param name="COUNT" select="$COUNT + 1"/>
</xsl:call-template>
</xsl:template>
</xsl:stylesheet>
EOF;

$xsl = new DOMDocument();
$xsl->loadXML($myxsl);

$doc = new DOMDocument();

$proc = new XSLTProcessor;
// Set the template depth limit so high that we will certainly hit the variable depth limit first.
$proc->maxTemplateDepth = 2**30;
$proc->maxTemplateVars = 2;
$proc->importStyleSheet($xsl);
$proc->transformToDoc($doc);

?>
--EXPECTF--
Warning: XSLTProcessor::transformToDoc(): runtime error: file %s line 8 element param in %s on line %d

Warning: XSLTProcessor::transformToDoc(): xsltApplyXSLTTemplate: A potential infinite template recursion was detected.
You can adjust maxTemplateVars (--maxvars) in order to raise the maximum number of variables/params (currently set to 2). in %s on line %d

Warning: XSLTProcessor::transformToDoc(): Templates: in %s on line %d

Warning: XSLTProcessor::transformToDoc(): #0 name recurse in %s on line %d

Warning: XSLTProcessor::transformToDoc(): #1 name recurse in %s on line %d

Warning: XSLTProcessor::transformToDoc(): #2 name / in %s on line %d

Warning: XSLTProcessor::transformToDoc(): Variables: in %s on line %d

Warning: XSLTProcessor::transformToDoc(): #0 in %s on line %d

Warning: XSLTProcessor::transformToDoc(): COUNT in %s on line %d

Warning: XSLTProcessor::transformToDoc(): #1 in %s on line %d

Warning: XSLTProcessor::transformToDoc(): param COUNT in %s on line %d
44 changes: 44 additions & 0 deletions ext/xsl/tests/maxTemplateDepth_errors.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
--TEST--
XSLTProcessor::$maxTemplateDepth errors
--EXTENSIONS--
xsl
--INI--
error_reporting=E_ALL & ~E_DEPRECATED
--FILE--
<?php

$processor = new XSLTProcessor;
$oldValue = $processor->maxTemplateDepth;

try {
$processor->maxTemplateDepth = -1;
} catch (ValueError $e) {
echo $e->getMessage(), "\n";
}

var_dump($processor->maxTemplateDepth === $oldValue);

try {
$processor->maxTemplateDepth = -32.1;
} catch (ValueError $e) {
echo $e->getMessage(), "\n";
}

var_dump($processor->maxTemplateDepth === $oldValue);

try {
$processor->maxTemplateDepth = "-1";
} catch (ValueError $e) {
echo $e->getMessage(), "\n";
}

var_dump($processor->maxTemplateDepth === $oldValue);

?>
--EXPECT--
XSLTProcessor::$maxTemplateDepth must be greater than or equal to 0
bool(true)
XSLTProcessor::$maxTemplateDepth must be greater than or equal to 0
bool(true)
XSLTProcessor::$maxTemplateDepth must be greater than or equal to 0
bool(true)
44 changes: 44 additions & 0 deletions ext/xsl/tests/maxTemplateVars_errors.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
--TEST--
XSLTProcessor::$maxTemplateVars errors
--EXTENSIONS--
xsl
--INI--
error_reporting=E_ALL & ~E_DEPRECATED
--FILE--
<?php

$processor = new XSLTProcessor;
$oldValue = $processor->maxTemplateVars;

try {
$processor->maxTemplateVars = -1;
} catch (ValueError $e) {
echo $e->getMessage(), "\n";
}

var_dump($processor->maxTemplateVars === $oldValue);

try {
$processor->maxTemplateVars = -32.1;
} catch (ValueError $e) {
echo $e->getMessage(), "\n";
}

var_dump($processor->maxTemplateVars === $oldValue);

try {
$processor->maxTemplateVars = "-1";
} catch (ValueError $e) {
echo $e->getMessage(), "\n";
}

var_dump($processor->maxTemplateVars === $oldValue);

?>
--EXPECT--
XSLTProcessor::$maxTemplateVars must be greater than or equal to 0
bool(true)
XSLTProcessor::$maxTemplateVars must be greater than or equal to 0
bool(true)
XSLTProcessor::$maxTemplateVars must be greater than or equal to 0
bool(true)
23 changes: 23 additions & 0 deletions ext/xsl/tests/new_without_constructor.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
--TEST--
XSLTProcessor: new instance without constructor
--EXTENSIONS--
xsl
--FILE--
<?php

$rc = new ReflectionClass('XSLTProcessor');
$processor = $rc->newInstanceWithoutConstructor();
var_dump($processor);

?>
--EXPECTF--
object(XSLTProcessor)#2 (4) {
["doXInclude"]=>
bool(false)
["cloneDocument"]=>
bool(false)
["maxTemplateDepth"]=>
int(%d)
["maxTemplateVars"]=>
int(%d)
}
8 changes: 8 additions & 0 deletions ext/xsl/xsltprocessor.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -360,6 +360,14 @@ static xmlDocPtr php_xsl_apply_stylesheet(zval *id, xsl_object *intern, xsltStyl
ctxt->xinclude = zend_is_true(doXInclude);
zend_string_release_ex(member, 0);

zval *max_template_depth = xsl_prop_max_template_depth(Z_OBJ_P(id));
ZEND_ASSERT(Z_TYPE_P(max_template_depth) == IS_LONG);
ctxt->maxTemplateDepth = Z_LVAL_P(max_template_depth);

zval *max_template_vars = xsl_prop_max_template_vars(Z_OBJ_P(id));
ZEND_ASSERT(Z_TYPE_P(max_template_vars) == IS_LONG);
ctxt->maxTemplateVars = Z_LVAL_P(max_template_vars);

secPrefsValue = intern->securityPrefs;

/* if securityPrefs is set to NONE, we don't have to do any checks, but otherwise... */
Expand Down