Fix validation logic of php:function() callbacks in dom and xsl #12593
Conversation
Two issues: - Assumed that at least 1 argument (function name) was provided. - Incorrect error path for the non-callable case.
| if (obj->stringval == NULL) { | ||
| zend_type_error("Handler name must be a string"); | ||
| xmlXPathFreeObject(obj); | ||
| goto cleanup; | ||
| goto cleanup_no_callable; | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
| zend_throw_error(NULL, "Function name must be passed as the first argument"); | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
Would the for loop below be more clear as:
for (i = fci.param_count; i > 0; i--) {?
There was a problem hiding this comment.
Agreed, I'll change it soon
| @@ -132,7 +137,7 @@ static void dom_xpath_ext_function_php(xmlXPathParserContextPtr ctxt, int nargs, | |||
| if (obj->stringval == NULL) { | |||
There was a problem hiding this comment.
Maybe add a comment that this obj is the last element of the FILO stack and must correspond to a PHP function name?
| zend_throw_error(NULL, "Function name must be passed as the first argument"); | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
Agreed, I'll change it soon
| @@ -132,7 +137,7 @@ static void dom_xpath_ext_function_php(xmlXPathParserContextPtr ctxt, int nargs, | |||
| if (obj->stringval == NULL) { | |||
Two issues: