Details
Description
Calculated question formula validation think that formulas like
{a}?
{b}is valid, which then results in parse errors in eval'ed code.
Easy workaround for this would be this method of syntax validation ($code is full code you need to execute, not just formula):
function check_syntax($code)
The function will return true if code is syntactical correct and false if it is not.
To get more accurate error reporting you'll need to use tools like http://netevil.org/blog/2006/nov/parserandlexergeneratorsforphp
This may not be easy at first, thought it'll provide you with an ability to create any possible features in you formulas and (eventually) avoid eval at all (this is good, as passing users input to eval may lead to very serious security vulnerabilities). Actually tools will made most complex job for you.
Gliffy Diagrams
Attachments
Issue Links
 has a nonspecific relationship to

MDL7198 New grammar analyzer for Calculated question with pluginable packages of functions.
 Closed
Activity
 All
 Comments
 Work Log
 History
 Activity
 Links Hierarchy
You may replace
{a}with 1 for syntax check, it doesn't matter. Full code means that you'll need to add at least "return " or "$something = " before formula and a semicolon after, to receive valid PHP operator.
This check should find you any sintactical incorrect things, but doesn't report what they are.
Thanks for the cue.
I will see how I can use it.
From Oleg ( MDL18034)
"P.S. I managed to get php code in eval using import, but first time failed to do something malicious with it. Will need to study calculated code more thorought to do it. I think either save_question_options should check formula validity, or it should be done before any eval to be sure (right now there is one eval before it isn't done)."
Effectively there is no checking there.
I will look at this.
There is already $question>import_process parameter that could be used when saving options to do the validate process on formulas that is done in edit_calculated_form,
as it is used to import datassets
if( isset($question>import_process)&&$question>import_process)
I will work on this, so get back to your main project...
Once the filtering is done I don't think that there are problems with "malignant" code because only math functions are allowed.
function qtype_calculated_find_formula_errors($formula) {
/// Validates the formula submitted from the question edit page.
/// Returns false if everything is alright.
/// Otherwise it constructs an error message
// Strip away dataset names
while (ereg('
<
{"\']*\\}', $formula, $regs))
{ $formula = str_replace($regs[0], '1', $formula); } // Strip away empty space and lowercase it
$formula = strtolower(str_replace(' ', '', $formula));
$safeoperatorchar = '+/%>:^~<?=&!'; / */
$operatorornumber = "[$safeoperatorchar.09eE]";
while (ereg("(^[$safeoperatorchar,(])([az09_]*)\\(($operatorornumber+(,$operatorornumber+((,$operatorornumber+)+)?)?)?
)",
$formula, $regs)) {
switch ($regs[2]) {
// Simple parenthesis
case '':
if ($regs[4]  strlen($regs[3])==0)
break;
// Zero argument functions
case 'pi':
if ($regs[3])
break;
// Single argument functions (the most common case)
case 'abs': case 'acos': case 'acosh': case 'asin': case 'asinh':
case 'atan': case 'atanh': case 'bindec': case 'ceil': case 'cos':
case 'cosh': case 'decbin': case 'decoct': case 'deg2rad':
case 'exp': case 'expm1': case 'floor': case 'is_finite':
case 'is_infinite': case 'is_nan': case 'log10': case 'log1p':
case 'octdec': case 'rad2deg': case 'sin': case 'sinh': case 'sqrt':
case 'tan': case 'tanh':
if ($regs[4]  empty($regs[3]))
break;
// Functions that take one or two arguments
case 'log': case 'round':
if ($regs[5]  empty($regs[3]))
break;
// Functions that must have two arguments
case 'atan2': case 'fmod': case 'pow':
if ($regs[5]  empty($regs[4]))
break;
// Functions that take two or more arguments
case 'min': case 'max':
if (empty($regs[4]))
break;
default:
return get_string('unsupportedformulafunction','quiz',$regs[2]);
}
// Exchange the function call with '1' and then chack for
// another function call...
if ($regs[1])
else
{ // The function call starts the formula $formula = ereg_replace("^$regs[2]\\([^)]*\\)", '1', $formula); }}
if (ereg("[^$safeoperatorchar.09eE]+", $formula, $regs))
{ return get_string('illegalformulasyntax', 'quiz', $regs[0]); }else
{ // Formula just might be valid return false; }}
However the maths results can be bad but this is detected elsewhere when creating the datasets.
"Once the filtering is done I don't think that there are problems with "malignant" code because only math functions are allowed. "  are you sure that not malignant things can be done via modifying values of variables?
I suggests adding syntactical check mentioned in description (not necessary as separate function) to qtype_calculated_find_formula_errors. It'll catch any additional errors, that are not identifyed by now.
On import however, additional checking is needed as students could be allowed to import questions.
I put this on my todo list...
See http://moodle.org/mod/forum/discuss.php?d=122222 for a related problem.
Thanks for reporting this issue.
We have detected that this issue has been inactive for over a year has been recorded as affecting versions that are no longer supported.
If you believe that this issue is still relevant to current versions (2.1 and beyond), please comment on the issue. Issues left inactive for a further month will be closed.
Michael d;
lqjjLKA0p6
Pierre  did you do anything about this problem in new Moodle version to have it fixed?
I don't beleive it is fixed, but have no time to retest for now...
This has been fixed in calculated/question.php i.e. 2.1 when Tim build this file

/**

* Evaluate an expression after the variable values have been substituted.

* @param string $expression the expression. A PHP expression with placeholders

* like {a} for where the variables need to go.

* @return float the computed result.

*/

protected function calculate_raw($expression) {

// This validation trick from http://php.net/manual/en/function.eval.php

if (!@eval('return true; $result = ' . $expression . ';')) {

throw new moodle_exception('illegalformulasyntax', 'qtype_calculated', '', $expression);

}

return eval('return ' . $expression . ';');

}

However I have to check if this is applied when editing the questions.
Micheal,
I will get through all my "not apparently active" bugs in the next 2 weeks.
Thanks for the remainder.
This was on my todo list for the semester so its promote at an higher level
Testing on master, this has not been solved completely.
i.e the error is not detected when editing the question.
the
?
{b}:
{c}structure is allowed in formulas but not correctly tested.
Tim has commented somewhere that he plans to implement for calculated the evalmath.class.php library set
for the new OU numericals questiontypes.
So this bug should remain active
Thanks for reporting this issue.
We have detected that this issue has been inactive for over a year has been recorded as affecting versions that are no longer supported.
If you believe that this issue is still relevant to current versions (2.3 and beyond), please comment on the issue. Issues left inactive for a further month will be closed.
Michael d;
4d6f6f646c6521
Reproduced on qa.moodle.net
{a}?
{b}as an answer gives parse errors when generating sets and saving the question.
Parse error: syntax error, unexpected ';' in /html/question/type/calculated/questiontype.php(1259) : eval()'d code on line 1
Parse error: syntax error, unexpected ';' in /html/question/type/calculated/questiontype.php(1073) : eval()'d code on line 1
Notice: Undefined variable: ansvalue in /html/question/type/calculated/questiontype.php on line 1074
Exploring again this problem and other related that are not detected
 if the user do not put an operator
 between two params {a} {b}
 two (...)(...)
 or the combination could result in either an error .
This could be detected byfunction qtype_calculated_find_formula_errors($formula) {
// Validates the formula submitted from the question edit page.
// Returns false if everything is alright
// otherwise it constructs an error message.
// Strip away dataset names.
$regs = array();
preg_match('~[)\}][ ]*[\{(]~',$formula, $regs);
if (isset($regs[0])) {
return " consecutive param or (...) ";
}
The (...)? true : false syntax can be tested by the following
$regs = array();

preg_match_all('~[\\?:]~',$formula, $regs);

$count = 0;

if ( $regs[0] != null ){

foreach ($regs[0] as $ri => $rs) {

// echo "<p>ri $ri rs $rs <p>";

if ($ri == 0 & $rs == ":"){

// echo "<p>ri $ri rs $rs";

return " : preceding ? ERROR in the formula<p>";

}

if ($rs == ":"){

$count += 1;

}

if ($rs == "?"){

$count += 1;

}

}

if ($count >= 0) return " ERROR in the formula ?: unbalanced more ? in the formula";

if ($count <= 0) return " ERROR in the formula ?: unbalanced more : in the formula";

}

This "RAW" code seems to work.
Hi Pierre, this issue is reported over 1.9. Is it still valid?
Yes I am doing the tests on last week master ...
In reviewing the code of calculatedsimple I did some tests and effectively in one of them I forgot to put a + between
{a}{b}
So I created the first regex and
this led me to this "old" unsolved problem.
So I am waiting for Tim comments about implementing this solution or something similar.
Pierre. All this (syntactical analysis of the formula) could be done with much less programming effort using specialised tools like PHPParserGenerator (or LemonPHP). If you wish, I could show you expamples in Preg question type where we use this technique for regular expression analysis and validation.
We also writing similar code for programming language expressions as a part of Formal languages block project, but it still have several bugs that need fixing.
Thanks for your offer.
I am waiting for Tim comments about using some additional patches to the actual code or adding a more complete syntactical analysis as what he have done in varnumeric question type and as you propose.
I send the following email to Tim about using EvalMath as done in OU var_numeric types.
At first EvalMath(true, true) is safer and a lot more versatile than the combination of qtype_calculated_find_formula_errors() and eval.
I think that we could set EvalMath(true, true) so to reproduce the already available calculated functions and then adding the possibilities for a more flexible handling.
This should allow to keep the existing calculated questions.
Is there some fundamental problems to do this ?
Here his comment :
If making this changes does not break existing calculated questions, the there is no problem. It would be a good change.
EvalMath is used by the Moodle gradebook (as well as varnumeric). I don't think there is any problem adding more functions. If you do then everyone benefits.
So I will explore how to use EvalMath
EvalMath first results are positive and let place to a more flexible equation handling ( here implicit * ).
Set 20 ({x})({y})

(4.7)(5.5) = 25.85

Correct answer : 25.85 inside limits of true value

Min: 25.5915  Max: 26.1085

There are few failures of the actual calculated PHP Unit tests mostly related ot how EvalMath reacts to extreme case
like qtype_calculated_find_formula_errors(1) test false.
This is not a good test as you could have one of the answer where the formula is 1 as long as there is a least one param
{x..}in one of the other answers.
So "If making this changes does not break existing calculated questions" should test true
So first let's explore what features EvalMath can add to calculated ...
Among the first things to do is to create a local class and fillin the function list to include all the functions of calculated
( https://docs.moodle.org/28/en/Calculated_question_type) as 'octdec','decoct'
that are not already in EvalMath
class CalculatedEvalMath extends EvalMath {


var $fb = array( // builtin functions

'abs','acos','acosh','asin','arcsin','arcsinh','asinh',

...'octdec','decoct' ... 'tan','tanh' );

}

Having a local class should give us additional flexibility
"Having a local class should give us additional flexibility"
EvalMath design does not allow a conditional structure as
(numeric result) ?(value to return if numeric result is true) : (value to return if numeric result is false)
so we need a prefunction that will split the formula in the 3 parts, use EvalMath to calculate each segment and return the result. (or any error).
Anyway this is safer than to use eval() as in actual code
eval('$ansvalue = '.$formula.';');
we need prefunction that will split the formula in the 3 parts, use EvalMath to calculate each segment and return the result. (or any error).
Done with something like
require_once($CFG>libdir . '/evalmath/evalmath.class.php');


class CalculatedEvalMath extends EvalMath {

var $fb = array( // builtin functions will contain all those already in calculated

'abs','acos','acosh','asin','asinh','sin','sinh','arcsin','arcsinh',

'cos','cosh','arccos','arccosh',

'tan','tanh','arctan','atan','arctanh','atanh',

'sqrt','ln','log','log10','exp','floor','ceil',

'octdec','decoct' );


function evaluatecalculated($expr) {

$regs = array();

preg_match_all('~[\\?:]~',$expr, $regs);

$count = 0;

if ( $regs[0] != null ){

foreach ($regs[0] as $ri => $rs) {

echo "<p>ri $ri rs $rs <p>"; // for testing

if ($rs == ":"){

$count += 1;

if ($count < 0 ) return $this>trigger(get_string('preceding:informula', 'qtype_calculated', $rs));

}

if ($rs == "?"){

$count += 1;

}

}

if ($count > 0) return $this>trigger(get_string('unbalanced?informula', 'qtype_calculated',$expr));

$res = preg_split('~[\\?:]~',$expr, 1);

// Convert each part of valuetotest ? valueiftrue : valueiffalse using evaluate.

foreach ($res as $ri => $resa) {

echo "<p>before evaluate $ri rs rs".$res[$ri]."<p>";// for testing

$res[$ri] = $this>evaluate($resa);

if (is_null($res[$ri])) return $this>trigger(get_string('unbalanced?informula', 'qtype_calculated',$expr));

echo "<p>after evaluate $ri rs rs".$res[$ri]."<p>";// for testing

}

// Start at 0.

$rebuilt="";

$ires = 0;

if(count($res) > count($regs[0]) ){ //there should be value?value:value

$rebuilt .= $res[$ires];

foreach ($regs[0] as $ri => $resa) {

$rebuilt .= $resa;

$ires++;

$rebuilt .= $res[$ires];

}

}

echo "<p>refait $refait<p>";// for testing

$str = null;

// We can use eval without problems as $rebuilt = number ? number : number ;

eval('$str = '.$rebuilt.';');

return $str;

}

return $this>evaluate($expr);

}

}

CalculateEvalMath 2 worlds side by side in some cases
(1.4)(1.9) = 2.66

Correct answer : 2.66 inside limits of true value

Min: 2.6334  Max: 2.6866

(1.4)*(1.9) = 2.66

Correct answer : 2.66 inside limits of true value

Min: 2.6334  Max: 2.6866


or


pow(6.6,2) = 43.56

Correct answer : 43.56 inside limits of true value

Min: 43.1244  Max: 43.9956

power(6.6,2) = 43.56

Correct answer : 43.56 inside limits of true value

Min: 43.1244  Max: 43.9956

So we could use the preceeding moodle versions calculateds and add to the next i.e. 2,9 version at least the functions that are already defined in EvalMath or other ones.
There are choices also about what additional features(response format, partial grading etc) that could be added without loosing the user ....
or losing the main advantage of calculated ( and complexity) that the variants responses are checked before being submitted to the student.
So Tim, how do we handle this opportunity (forum ?).
So let use the request a peer review feature
As I wrote in a preceeding comment
What do we plan now and how ?
Fails against automated checks.
Error: the repository field is empty. Nothing was checked.
 Testing instructions are missing.
_{More information about this report}
A technical dificulty in handling the additional functions and parameters.
To simplify I add all additional PHP math functions by rebuilding $fb from evalmath in calculated questiontype.php
As I want to control all the buitlin functions I just redefine
class CalculatedEvalMath extends EvalMath {


var $fb = array( // builtin functions

'abs','acos','acosh','asin','asinh','sin','sinh','arcsin','arcsinh',

'cos','cosh','arccos','arccosh',

'tan','tanh','arctan','atan','arctanh','atanh',

'sqrt','ln','log','log10','exp','floor','ceil',

'octdec','decoct' );

To add the pow I add them in evalmath file as $fc is called as
line 386 of evalmath.class.php file
call_user_func_array(array('EvalMathFuncs', $fnn), array_reverse($args));
class EvalMath {

var $fc = array( // calc functions emulation

'average'=>array(1), 'max'=>array(1), 'min'=>array(1),

'mod'=>array(2), 'pi'=>array(0), 'power'=>array(2), 'pow' = array(2),

'round'=>array(1, 2), 'sum'=>array(1), 'rand_int'=>array(2),

'rand_float'=>array(0));

and
// spreadsheet functions emulation
class EvalMathFuncs {

static function pow($op1, $op2) {

return pow($op1, $op2);

}

...

Do we need to copy all the evalmath.class.php file in a calculated file so to have a complete control ?
I just put the code on github (just a working issue)
Tim,
There is no hurry about how set the final code. The main thing is that we can improve calculated keeping the old questions active.
I will first prepare some docs in the Development section to set a more complete proposal on what could be the features of the "new and improved" calculated questions.
Then we could start an issue in the quiz forum somewhere in january.(Although my 3 childrens are 4550 years old, I watch TV and take christmas holidays )
I just push to github a "crude" solution where I create local variant
class CalculatedEvalMath extends EvalMath where I copy parts of the variables and code so I can handle all the functions I we will need and use a copy the class EvalMathFuncs as class CalcEvalMathFuncs .
This is working as shown in editing a calculated simple question.
Set 1 pow(
,2)+pow...
pow(9.8,2)+power(9.8,2) = 192.08
Correct answer : 192.08 inside limits of true value
Min: 190.1592 — Max: 194.0008
Apart putting this in the questiontype.php file
Is it the best way to do it ???
Learning is a continuous process.
There is a better solution which is simply something like
class CalcEvalMathFuncs extends EvalMathFuncs {


static function pow($op1, $op2) {

return pow($op1, $op2);

}


}

I did not see that EvalMathFuncs is an array of functions until this morning and that can it be manipulated as so
So I updated github and I go back to PHP manuals as a student
One of my concerns is about code stability and testing. If we define
class CalcEvalMathFuncs extends EvalMathFuncs {


static function pow($op1, $op2) {

return pow($op1, $op2);

}

we also inherit of all the functions defined in EvalMathFuncs . If there modification of that class i.e. new functions they will be available for the user of calculated but not necessarily tested for calculated requirements.
One way to limit is to define in calculated
var $fb = array( // builtin functions
'abs','acos','acosh','asin','asinh','sin','sinh','arcsin','arcsinh',
'cos','cosh','arccos','arccosh',
'tan','tanh','arctan','atan','arctanh','atanh',
'sqrt','ln','log','log10','exp','floor','ceil',
'octdec','decoct' );
var $fc = array( // calc functions emulation
'average'=>array(1), 'max'=>array(1), 'min'=>array(1),
'mod'=>array(2), 'pi'=>array(0), 'power'=>array(2), 'pow'=>array(2),
'round'=>array(1, 2), 'sum'=>array(1), 'rand_int'=>array(2),
'rand_float'=>array(0));
so the preceeding
class CalcEvalMathFuncs {

static function average() {

$args = func_get_args();

return (call_user_func_array(array('self', 'sum'), $args) / count($args));

}

...

static function max() {

$args = func_get_args();


static function pow($op1, $op2) {

return pow($op1, $op2);

...

}

}

is another way to control which functions are allowed in calculated.
Sorry for the slow reply. It has been a busy week.
We really don't want to copy anything from the EvalMathFuncs class.
I am thinking that several of those functions would be useful additions to the gradebook. Therefore, my suggestion is to make a new MDL issue in order to add the new functions to the core EvalMathFuncs class. Then we submit that for integation as a separate change.
Once that has been integrated, then we can change qtype_calculated to use it.
There is 2 lists of functions useable in EvalMath : primary function with 1 parameter that are listed in $fb
var $fb = array( // builtin functions

'sin','sinh','arcsin','asin','arcsinh','asinh',

'cos','cosh','arccos','acos','arccosh','acosh',

'tan','tanh','arctan','atan','arctanh','atanh',

'sqrt','abs','ln','log','exp','floor','ceil');

There is also the list of primary functions with 0 or more than 1 paramer that are listed in $fc
var $fc = array( // calc functions emulation

'average'=>array(1), 'max'=>array(1), 'min'=>array(1),

'mod'=>array(2), 'pi'=>array(0), 'power'=>array(2),

'round'=>array(1, 2), 'sum'=>array(1), 'rand_int'=>array(2),

'rand_float'=>array(0));

My objective is to be able to used all these functions already available in EvalMath
+
primary Math functions that are in actual calculated but NOT in EvalMath (like pow)
+ other functions that we will want to use now or later in calculated that would be added either in $fb or $fc folowing their parameters.
This why I declare in $fb or $fc in class CalculatedEvalMath extends EvalMath
and the code of new functions NOT already set in class EvalMathFuncs { are declared in local as
class CalcEvalMathFuncs extends EvalMathFuncs {
So the expansion of calculated available functions can be done without any interference from EvalMath addtional new functions. If the use of some new calculated function can be generalized in EvalMath then Evalmath code could be improved.
But I will say that your proposal to improve EvalMath should be done once we will have created and tested a new efficient function in calculated question.
For this issue I think that we should first merge EvalMath in calculated so that all actual old calculated remain valid and could benefit from the new functions as newly created calculated.
My example is that an old calculated question with
pow({x},2) can use side by side power({x},2)

and obtains the same result.
I still think I would extend core EvalMath. Adding new functions there will not break anything, and might be useful in the gradebook or variablenumeric question types. It also avoids having to copy a whole function just to change the call_user_func_array(array('CalcEvalMathFuncs', $fnn) bit.
However, you are doing the work, so I am happy for you to do it the way you think is best.
Just a suggestion, but rather than copying the var $fb and var $fc arrays, instead you can do:
function __construct() { 
$fc['pow'] = 2; 
$fc['average'] = 1; 
$fc['max'] = 1; 
}

That makes it easier to see which functions are being added.
I have added a page in docs to describe the things to do for each functions.
https://docs.moodle.org/dev/Calculated_question_:_adding_new_math_functions#Available_functions
Although it is not completed for all function, it illustrates which functions already exist in either Calculated or EvalMath so they should already be usable when the issue is completed.
If they already exist in EvalMath then they should already work in the actual MDL18141 code.
Some need to be created in EvalMath , i.e. new .
However they could be created only in CalculatedEvalMath. i.e this is the case of pow() .
*I still think I would extend core EvalMath. Adding new functions there will not break anything, and might be useful in the gradebook or variablenumeric question types. *
So as you agree, I will extend core EvalMath .
However in order to test for bad values the actual calculated allow syntax involving
 is_finite
 is_infinite
 is_nan
So in actual 2.8 Moodle demo syntax like these are working OK.
2.0*4(2.0==5?0.8:0)+5.4 = 13.40

Correct answer : 13.40 inside limits of true value

Min: 13.27  Max: 13.53

is_infinite((9.8+6.8)/1)?4:5 = 5.00

Correct answer : 5.00 inside limits of true value

Min: 4.95  Max: 5.05


but not in the actual issue even if I add is_infinite in the available functions.
I understand that this is quite tricky but most probably OK given the way EvalMath works.
So let's put this on january todo list
In calculated question
 is_finite
 is_infinite
 is_nan
could ( should) only be used in the ?: structure and can ( I tested) be filtered in calculated when handling the ?: structure.
I don't think that they should be allowed in EvalMath as they are part of the error code in EvalMath.
Just a little comment that life has modified todo january list to todo february list
Pierre Pichet, if you unassign issues from yourself please do not forget to click "Stop development" button. TIA
$code is full code you need to execute, not just formula):
{a}I.e. with the
parameters replace by their real values .
this is not so simple. the actual code replace everything by 1 as a first check.
Not perfect but better than nothing.
Working on this could include
MDL7198proposal as a complete solution.