CVE-2014-3545

@@ -419,6 +419,10 @@ public function get_values() {
      * @return float the computed result.
      */
     public function calculate($expression) {
+        // Make sure no malicious code is present in the expression. Refer MDL-46148 for details.
+        if ($error = qtype_calculated_find_formula_errors($expression)) {
+            throw new moodle_exception('illegalformulasyntax', 'qtype_calculated', '', $error);
+        }
         return $this->calculate_raw($this->substitute_values_for_eval($expression));
     }
 

@@ -372,6 +372,7 @@ public function importprocess($category) {
                         $this->category = $newcategory;
                     }
                 }
+                $transaction->allow_commit();
                 continue;
             }
             $question->context = $this->importcontext;

@@ -1061,10 +1061,14 @@ public function comment_on_datasetitems($qtypeobj, $questionid, $questiontext,
         }
 
         $answers = fullclone($answers);
-        $errors = '';
         $delimiter = ': ';
         $virtualqtype =  $qtypeobj->get_virtual_qtype();
         foreach ($answers as $key => $answer) {
+            $error = qtype_calculated_find_formula_errors($answer->answer);
+            if ($error) {
+                $comment->stranswers[$key] = $error;
+                continue;
+            }
             $formula = $this->substitute_variables($answer->answer, $data);
             $formattedanswer = qtype_calculated_calculate_answer(
                 $answer->answer, $data, $answer->tolerance,

@@ -357,6 +357,7 @@ public function importprocess($category) {
         $count = 0;
 
         foreach ($questions as $question) {   // Process and store each question
+            $transaction = $DB->start_delegated_transaction();
 
             // reset the php timeout
             core_php_time_limit::raise();
@@ -429,9 +430,14 @@ public function importprocess($category) {
 
             if (!empty($result->error)) {
                 echo $OUTPUT->notification($result->error);
+                // Can't use $transaction->rollback(); since it requires an exception,
+                // and I don't want to rewrite this code to change the error handling now.
+                $DB->force_transaction_rollback();
                 return false;
             }
 
+            $transaction->allow_commit();
+
             if (!empty($result->notice)) {
                 echo $OUTPUT->notification($result->notice);
                 return true;

@@ -606,12 +606,9 @@ public function readquestions ($lines) {
                 // Calculated Question.
                 $question = $this->defaultquestion();
                 $question->qtype = 'calculated';
-                $question->answers = array(); // No problem as they go as :FORMULA: from webct.
+                $question->answer = array(); // No problem as they go as :FORMULA: from webct.
                 $question->units = array();
                 $question->dataset = array();
-
-                // To make us pass the end-of-question sanity checks.
-                $question->answer = array('dummy');
                 $question->fraction = array('1.0');
                 $question->feedback = array();
 
@@ -739,7 +736,7 @@ public function readquestions ($lines) {
             if (preg_match('~^:FORMULA:(.*)~i', $line, $webctoptions)) {
                 // Answer for a calculated question.
                 ++$currentchoice;
-                $question->answers[$currentchoice] =
+                $question->answer[$currentchoice] =
                         qformat_webct_convert_formula($webctoptions[1]);
 
                 // Default settings.

@@ -779,7 +779,7 @@ public function import_calculated($question) {
 
         // Get answers array.
         $answers = $question['#']['answer'];
-        $qo->answers = array();
+        $qo->answer = array();
         $qo->feedback = array();
         $qo->fraction = array();
         $qo->tolerance = array();
@@ -793,7 +793,7 @@ public function import_calculated($question) {
             if (empty($ans->answer['text'])) {
                 $ans->answer['text'] = '*';
             }
-            $qo->answers[] = $ans->answer;
+            $qo->answer[] = $ans->answer['text'];
             $qo->feedback[] = $ans->feedback;
             $qo->tolerance[] = $answer['#']['tolerance'][0]['#'];
             // Fraction as a tag is deprecated.

@@ -42,6 +42,9 @@ public function save_question_options($question) {
         global $CFG, $DB;
         $context = $question->context;
 
+        // Make it impossible to save bad formulas anywhere.
+        $this->validate_question_data($question);
+
         // Calculated options.
         $update = true;
         $options = $DB->get_record('question_calculated_options',
@@ -72,10 +75,7 @@ public function save_question_options($question) {
         }
 
         // Insert all the new answers.
-        if (isset($question->answer) && !isset($question->answers)) {
-            $question->answers = $question->answer;
-        }
-        foreach ($question->answers as $key => $answerdata) {
+        foreach ($question->answer as $key => $answerdata) {
             if (is_array($answerdata)) {
                 $answerdata = $answerdata['text'];
             }
@@ -163,6 +163,13 @@ protected function validate_answer($answer) {
         }
     }
 
+    protected function validate_question_data($question) {
+        parent::validate_question_data($question);
+        $this->validate_text($question->correctfeedback['text']);
+        $this->validate_text($question->partiallycorrectfeedback['text']);
+        $this->validate_text($question->incorrectfeedback['text']);
+    }
+
     protected function make_question_instance($questiondata) {
         question_bank::load_question_definition_classes($this->name());
         if ($questiondata->options->single) {

@@ -131,11 +131,13 @@ public function get_datasets_for_export($question) {
 
     public function save_question_options($question) {
         global $CFG, $DB;
+
+        // Make it impossible to save bad formulas anywhere.
+        $this->validate_question_data($question);
+
         // The code is used for calculated, calculatedsimple and calculatedmulti qtypes.
         $context = $question->context;
-        if (isset($question->answer) && !isset($question->answers)) {
-            $question->answers = $question->answer;
-        }
+
         // Calculated options.
         $update = true;
         $options = $DB->get_record('question_calculated_options',
@@ -185,14 +187,7 @@ public function save_question_options($question) {
             $units = $result->units;
         }
 
-        // Insert all the new answers.
-        if (isset($question->answer) && !isset($question->answers)) {
-            $question->answers = $question->answer;
-        }
-        foreach ($question->answers as $key => $answerdata) {
-            if (is_array($answerdata)) {
-                $answerdata = $answerdata['text'];
-            }
+        foreach ($question->answer as $key => $answerdata) {
             if (trim($answerdata) == '') {
                 continue;
             }
@@ -471,6 +466,25 @@ protected function validate_answer($answer) {
         }
     }
 
+    /**
+     * Validate data before save.
+     * @param stdClass $question data from the form / import file.
+     */
+    protected function validate_question_data($question) {
+        $this->validate_text($question->questiontext); // Yes, really no ['text'].
+
+        if (isset($question->generalfeedback['text'])) {
+            $this->validate_text($question->generalfeedback['text']);
+        } else if (isset($question->generalfeedback)) {
+            $this->validate_text($question->generalfeedback); // Because question import is weird.
+        }
+
+        foreach ($question->answer as $key => $answer) {
+            $this->validate_answer($answer);
+            $this->validate_text($question->feedback[$key]['text']);
+        }
+    }
+
     /**
      * This method prepare the $datasets in a format similar to dadatesetdefinitions_form.php
      * so that they can be saved
@@ -483,13 +497,13 @@ protected function validate_answer($answer) {
      * @param object $form
      * @param int $questionfromid default = '0'
      */
-    public function preparedatasets($form , $questionfromid = '0') {
+    public function preparedatasets($form, $questionfromid = '0') {
 
         // The dataset names present in the edit_question_form and edit_calculated_form
         // are retrieved.
         $possibledatasets = $this->find_dataset_names($form->questiontext);
         $mandatorydatasets = array();
-        foreach ($form->answers as $key => $answer) {
+        foreach ($form->answer as $key => $answer) {
             $mandatorydatasets += $this->find_dataset_names($answer);
         }
         // If there are identical datasetdefs already saved in the original question
@@ -579,12 +593,6 @@ public function addnamecategory(&$question) {
     public function save_question($question, $form) {
         global $DB;
 
-        if (isset($form->correctfeedback)) {
-            $this->validate_text($form->correctfeedback['text']);
-            $this->validate_text($form->partiallycorrectfeedback['text']);
-            $this->validate_text($form->incorrectfeedback['text']);
-        }
-
         if ($this->wizardpagesnumber() == 1 || $question->qtype == 'calculatedsimple') {
             $question = parent::save_question($question, $form);
             return $question;
@@ -605,14 +613,6 @@ public function save_question($question, $form) {
             case '' :
             case 'question': // Coming from the first page, creating the second.
                 if (empty($form->id)) { // or a new question $form->id is empty.
-                    // Make it impossible to save bad formulas anywhere.
-                    $this->validate_text($form->questiontext['text']);
-                    $this->validate_text($form->generalfeedback['text']);
-                    foreach ($form->answer as $key => $answer) {
-                        $this->validate_answer($answer);
-                        $this->validate_text($form->feedback[$key]['text']);
-                    }
-
                     $question = parent::save_question($question, $form);
                     // Prepare the datasets using default $questionfromid.
                     $this->preparedatasets($form);

@@ -43,11 +43,9 @@ class qtype_calculatedsimple extends qtype_calculated {
     public function save_question_options($question) {
         global $CFG, $DB;
         $context = $question->context;
-        // Get old answers.
 
-        if (isset($question->answer) && !isset($question->answers)) {
-            $question->answers = $question->answer;
-        }
+        // Make it impossible to save bad formulas anywhere.
+        $this->validate_question_data($question);
 
         // Get old versions of the objects.
         if (!$oldanswers = $DB->get_records('question_answers',
@@ -69,10 +67,7 @@ public function save_question_options($question) {
             $units = &$result->units;
         }
         // Insert all the new answers.
-        if (isset($question->answer) && !isset($question->answers)) {
-            $question->answers = $question->answer;
-        }
-        foreach ($question->answers as $key => $answerdata) {
+        foreach ($question->answer as $key => $answerdata) {
             if (is_array($answerdata)) {
                 $answerdata = $answerdata['text'];
             }

@@ -346,49 +346,6 @@ protected function initialise_question_instance(question_definition $question, $
         $question->datasetloader = new qtype_calculated_dataset_loader($questiondata->id);
     }
 
-    public function validate_form($form) {
-        switch($form->wizardpage) {
-            case 'question':
-                $calculatedmessages = array();
-                if (empty($form->name)) {
-                    $calculatedmessages[] = get_string('missingname', 'qtype_calculated');
-                }
-                if (empty($form->questiontext)) {
-                    $calculatedmessages[] = get_string('missingquestiontext', 'qtype_calculated');
-                }
-                // Verify formulas.
-                foreach ($form->answers as $key => $answer) {
-                    if ('' === trim($answer)) {
-                        $calculatedmessages[] = get_string(
-                                'missingformula', 'qtype_calculated');
-                    }
-                    if ($formulaerrors = qtype_calculated_find_formula_errors($answer)) {
-                        $calculatedmessages[] = $formulaerrors;
-                    }
-                    if (! isset($form->tolerance[$key])) {
-                        $form->tolerance[$key] = 0.0;
-                    }
-                    if (! is_numeric($form->tolerance[$key])) {
-                        $calculatedmessages[] = get_string('xmustbenumeric', 'qtype_numerical',
-                                get_string('tolerance', 'qtype_calculated'));
-                    }
-                }
-
-                if (!empty($calculatedmessages)) {
-                    $errorstring = "The following errors were found:<br />";
-                    foreach ($calculatedmessages as $msg) {
-                        $errorstring .= $msg . '<br />';
-                    }
-                    print_error($errorstring);
-                }
-
-                break;
-            default:
-                return parent::validate_form($form);
-                break;
-        }
-        return true;
-    }
     public function finished_edit_wizard($form) {
         return isset($form->savechanges);
     }

@@ -216,36 +216,39 @@ public function qtype() {
         return 'calculated';
     }
 
-    public function validation($data, $files) {
-
-        // Verifying for errors in {=...} in question text.
-        $qtext = "";
-        $qtextremaining = $data['questiontext']['text'];
-        $possibledatasets = $this->qtypeobj->find_dataset_names($data['questiontext']['text']);
-        foreach ($possibledatasets as $name => $value) {
-            $qtextremaining = str_replace('{'.$name.'}', '1', $qtextremaining);
-        }
-        while (preg_match('~\{=([^[:space:]}]*)}~', $qtextremaining, $regs1)) {
-            $qtextsplits = explode($regs1[0], $qtextremaining, 2);
-            $qtext = $qtext.$qtextsplits[0];
-            $qtextremaining = $qtextsplits[1];
-            if (!empty($regs1[1]) && $formulaerrors =
-                    qtype_calculated_find_formula_errors($regs1[1])) {
-                if (!isset($errors['questiontext'])) {
-                    $errors['questiontext'] = $formulaerrors.':'.$regs1[1];
-                } else {
-                    $errors['questiontext'] .= '<br/>'.$formulaerrors.':'.$regs1[1];
-                }
-            }
+    /**
+     * Validate the equations in the some question content.
+     * @param array $errors where errors are being accumulated.
+     * @param string $field the field being validated.
+     * @param string $text the content of that field.
+     * @return array the updated $errors array.
+     */
+    protected function validate_text($errors, $field, $text) {
+        $problems = qtype_calculated_find_formula_errors_in_text($text);
+        if ($problems) {
+            $errors[$field] = $problems;
         }
+        return $errors;
+    }
 
+    public function validation($data, $files) {
         $errors = parent::validation($data, $files);
 
+        // Verifying for errors in {=...} in question text.
+        $errors = $this->validate_text($errors, 'questiontext', $data['questiontext']['text']);
+        $errors = $this->validate_text($errors, 'generalfeedback', $data['generalfeedback']['text']);
+
         // Check that the answers use datasets.
         $answers = $data['answer'];
         $mandatorydatasets = array();
         foreach ($answers as $key => $answer) {
+            $problems = qtype_calculated_find_formula_errors($answer);
+            if ($problems) {
+                $errors['answeroptions['.$key.']'] = $problems;
+            }
             $mandatorydatasets += $this->qtypeobj->find_dataset_names($answer);
+            $errors = $this->validate_text($errors, 'feedback[' . $key . ']',
+                    $data['feedback'][$key]['text']);
         }
         if (empty($mandatorydatasets)) {
             foreach ($answers as $key => $answer) {

@@ -232,30 +232,31 @@ protected function data_preprocessing_answers($question, $withanswerfiles = fals
         return $question;
     }
 
+    /**
+     * Validate the equations in the some question content.
+     * @param array $errors where errors are being accumulated.
+     * @param string $field the field being validated.
+     * @param string $text the content of that field.
+     * @return array the updated $errors array.
+     */
+    protected function validate_text($errors, $field, $text) {
+        $problems = qtype_calculated_find_formula_errors_in_text($text);
+        if ($problems) {
+            $errors[$field] = $problems;
+        }
+        return $errors;
+    }
+
     public function validation($data, $files) {
         $errors = parent::validation($data, $files);
 
         // Verifying for errors in {=...} in question text.
-        $qtext = '';
-        $qtextremaining = $data['questiontext']['text'];
-        $possibledatasets = $this->qtypeobj->find_dataset_names($data['questiontext']['text']);
-        foreach ($possibledatasets as $name => $value) {
-            $qtextremaining = str_replace('{'.$name.'}', '1', $qtextremaining);
-        }
+        $errors = $this->validate_text($errors, 'questiontext', $data['questiontext']['text']);
+        $errors = $this->validate_text($errors, 'generalfeedback', $data['generalfeedback']['text']);
+        $errors = $this->validate_text($errors, 'correctfeedback', $data['correctfeedback']['text']);
+        $errors = $this->validate_text($errors, 'partiallycorrectfeedback', $data['partiallycorrectfeedback']['text']);
+        $errors = $this->validate_text($errors, 'incorrectfeedback', $data['incorrectfeedback']['text']);
 
-        while (preg_match('~\{=([^[:space:]}]*)}~', $qtextremaining, $regs1)) {
-            $qtextsplits = explode($regs1[0], $qtextremaining, 2);
-            $qtext = $qtext.$qtextsplits[0];
-            $qtextremaining = $qtextsplits[1];
-            if (!empty($regs1[1]) && $formulaerrors =
-                    qtype_calculated_find_formula_errors($regs1[1])) {
-                if (!isset($errors['questiontext'])) {
-                    $errors['questiontext'] = $formulaerrors.':'.$regs1[1];
-                } else {
-                    $errors['questiontext'] .= '<br/>'.$formulaerrors.':'.$regs1[1];
-                }
-            }
-        }
         $answers = $data['answer'];
         $answercount = 0;
         $maxgrade = false;
@@ -270,6 +271,7 @@ public function validation($data, $files) {
                         get_string('atleastonewildcard', 'qtype_calculated');
             }
         }
+
         $totalfraction = 0;
         $maxfraction = -1;
         foreach ($answers as $key => $answer) {
@@ -283,27 +285,11 @@ public function validation($data, $files) {
             }
             if ($trimmedanswer != '' || $answercount == 0) {
                 // Verifying for errors in {=...} in answer text.
-                $qanswer = '';
-                $qanswerremaining =  $trimmedanswer;
-                $possibledatasets = $this->qtypeobj->find_dataset_names($trimmedanswer);
-                foreach ($possibledatasets as $name => $value) {
-                    $qanswerremaining = str_replace('{'.$name.'}', '1', $qanswerremaining);
-                }
-
-                while (preg_match('~\{=([^[:space:]}]*)}~', $qanswerremaining, $regs1)) {
-                    $qanswersplits = explode($regs1[0], $qanswerremaining, 2);
-                    $qanswer = $qanswer . $qanswersplits[0];
-                    $qanswerremaining = $qanswersplits[1];
-                    if (!empty($regs1[1]) && $formulaerrors =
-                            qtype_calculated_find_formula_errors($regs1[1])) {
-                        if (!isset($errors['answeroptions['.$key.']'])) {
-                            $errors['answeroptions['.$key.']'] = $formulaerrors.':'.$regs1[1];
-                        } else {
-                            $errors['answeroptions['.$key.']'] .= '<br/>'.$formulaerrors.':'.$regs1[1];
-                        }
-                    }
-                }
+                $errors = $this->validate_text($errors, 'answeroptions[' . $key . ']', $answer);
+                $errors = $this->validate_text($errors, 'feedback[' . $key . ']',
+                        $data['feedback'][$key]['text']);
             }
+
             if ($trimmedanswer != '') {
                 if ('2' == $data['correctanswerformat'][$key] &&
                         '0' == $data['correctanswerlength'][$key]) {

@@ -156,6 +156,13 @@ public function save_question_options($question) {
         return true;
     }
 
+    protected function validate_answer($answer) {
+        $error = qtype_calculated_find_formula_errors_in_text($answer);
+        if ($error) {
+            throw new coding_exception($error);
+        }
+    }
+
     protected function make_question_instance($questiondata) {
         question_bank::load_question_definition_classes($this->name());
         if ($questiondata->options->single) {

@@ -484,6 +484,36 @@ public function display_question_editing_page($mform, $question, $wizardnow) {
         $mform->display();
     }
 
+    /**
+     * Verify that the equations in part of the question are OK.
+     * We throw an exception here because this should have already been validated
+     * by the form. This is just a last line of defence to prevent a question
+     * being stored in the database if it has bad formulas. This saves us from,
+     * for example, malicious imports.
+     * @param string $text containing equations.
+     */
+    protected function validate_text($text) {
+        $error = qtype_calculated_find_formula_errors_in_text($text);
+        if ($error) {
+            throw new coding_exception($error);
+        }
+    }
+
+    /**
+     * Verify that an answer is OK.
+     * We throw an exception here because this should have already been validated
+     * by the form. This is just a last line of defence to prevent a question
+     * being stored in the database if it has bad formulas. This saves us from,
+     * for example, malicious imports.
+     * @param string $text containing equations.
+     */
+    protected function validate_answer($answer) {
+        $error = qtype_calculated_find_formula_errors($answer);
+        if ($error) {
+            throw new coding_exception($error);
+        }
+    }
+
     /**
      * This method prepare the $datasets in a format similar to dadatesetdefinitions_form.php
      * so that they can be saved
@@ -497,11 +527,12 @@ public function display_question_editing_page($mform, $question, $wizardnow) {
      * @param int $questionfromid default = '0'
      */
     public function preparedatasets($form , $questionfromid = '0') {
+
         // The dataset names present in the edit_question_form and edit_calculated_form
         // are retrieved.
         $possibledatasets = $this->find_dataset_names($form->questiontext);
         $mandatorydatasets = array();
-        foreach ($form->answers as $answer) {
+        foreach ($form->answers as $key => $answer) {
             $mandatorydatasets += $this->find_dataset_names($answer);
         }
         // If there are identical datasetdefs already saved in the original question
@@ -590,8 +621,15 @@ public function addnamecategory(&$question) {
      */
     public function save_question($question, $form) {
         global $DB;
+
+        if (isset($form->correctfeedback)) {
+            $this->validate_text($form->correctfeedback['text']);
+            $this->validate_text($form->partiallycorrectfeedback['text']);
+            $this->validate_text($form->incorrectfeedback['text']);
+        }
+
         if ($this->wizardpagesnumber() == 1 || $question->qtype == 'calculatedsimple') {
-                $question = parent::save_question($question, $form);
+            $question = parent::save_question($question, $form);
             return $question;
         }
 
@@ -610,6 +648,14 @@ public function save_question($question, $form) {
             case '' :
             case 'question': // Coming from the first page, creating the second.
                 if (empty($form->id)) { // or a new question $form->id is empty.
+                    // Make it impossible to save bad formulas anywhere.
+                    $this->validate_text($form->questiontext['text']);
+                    $this->validate_text($form->generalfeedback['text']);
+                    foreach ($form->answer as $key => $answer) {
+                        $this->validate_answer($answer);
+                        $this->validate_text($form->feedback[$key]['text']);
+                    }
+
                     $question = parent::save_question($question, $form);
                     // Prepare the datasets using default $questionfromid.
                     $this->preparedatasets($form);

@@ -278,6 +278,7 @@ public function datasets_are_synchronised($category) {
  * @license    http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
  */
 class qtype_calculated_variable_substituter {
+
     /** @var array variable name => value */
     protected $values;
 
@@ -466,7 +467,7 @@ protected function substitute_values_pretty($text) {
      */
     public function replace_expressions_in_text($text, $length = null, $format = null) {
         $vs = $this; // Can't use $this in a PHP closure.
-        $text = preg_replace_callback('~\{=([^{}]*(?:\{[^{}]+}[^{}]*)*)}~',
+        $text = preg_replace_callback(qtype_calculated::FORMULAS_IN_TEXT_REGEX,
                 function ($matches) use ($vs, $format, $length) {
                     return $vs->format_float($vs->calculate($matches[1]), $length, $format);
                 }, $text);

@@ -37,6 +37,9 @@
  * @license    http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
  */
 class qtype_calculated extends question_type {
+    /** Regular expression that finds the formulas in content. */
+    const FORMULAS_IN_TEXT_REGEX = '~\{=([^{}]*(?:\{[^{}]+}[^{}]*)*)\}~';
+
     const MAX_DATASET_ITEMS = 100;
 
     public $wizardpagesnumber = 3;
@@ -1984,3 +1987,26 @@ function qtype_calculated_find_formula_errors($formula) {
         return false;
     }
 }
+
+/**
+ * Validate all the forumulas in a bit of text.
+ * @param string $text the text in which to validate the formulas.
+ * @return string|boolean false if there are no problems. Otherwise a string error message.
+ */
+function qtype_calculated_find_formula_errors_in_text($text) {
+    preg_match_all(qtype_calculated::FORMULAS_IN_TEXT_REGEX, $text, $matches);
+
+    $errors = array();
+    foreach ($matches[1] as $match) {
+        $error = qtype_calculated_find_formula_errors($match);
+        if ($error) {
+            $errors[] = $error;
+        }
+    }
+
+    if ($errors) {
+        return implode(' ', $errors);
+    }
+
+    return false;
+}

@@ -83,4 +83,23 @@ public function test_functions_with_wrong_num_args_caught() {
         $this->assert_nonempty_string(qtype_calculated_find_formula_errors('atan2(1.0, 1.0, 2.0)'));
         $this->assert_nonempty_string(qtype_calculated_find_formula_errors('max(1.0)'));
     }
+
+    public function test_validation_of_formulas_in_text_ok() {
+        $this->assertFalse(qtype_calculated_find_formula_errors_in_text(
+                '<p>Look no equations.</p>'));
+        $this->assertFalse(qtype_calculated_find_formula_errors_in_text(
+                '<p>Simple variable: {x}.</p>'));
+        $this->assertFalse(qtype_calculated_find_formula_errors_in_text(
+                '<p>This is an equation: {=1+1}, as is this: {={x}+{y}}.</p>' .
+                '<p>Here is a more complex one: {=sin(2*pi()*{theta})}.</p>'));
+    }
+
+    public function test_validation_of_formulas_in_text_bad_function() {
+        $this->assert_nonempty_string(qtype_calculated_find_formula_errors_in_text(
+                '<p>This is an equation: {=eval(1)}.</p>'));
+        $this->assert_nonempty_string(qtype_calculated_find_formula_errors_in_text(
+                '<p>Good: {=1+1}, bad: {=eval(1)}, good: {={x}+{y}}.</p>'));
+        $this->assert_nonempty_string(qtype_calculated_find_formula_errors_in_text(
+                '<p>Bad: {=eval(1)}, bad: {=system(1)}.</p>'));
+    }
 }

@@ -465,108 +465,11 @@ protected function substitute_values_pretty($text) {
      * @return string the text with values substituted.
      */
     public function replace_expressions_in_text($text, $length = null, $format = null) {
-        $vs = $this; // Can't see to use $this in a PHP closure.
+        $vs = $this; // Can't use $this in a PHP closure.
         $text = preg_replace_callback('~\{=([^{}]*(?:\{[^{}]+}[^{}]*)*)}~',
                 function ($matches) use ($vs, $format, $length) {
                     return $vs->format_float($vs->calculate($matches[1]), $length, $format);
                 }, $text);
         return $this->substitute_values_pretty($text);
     }
-
-    /**
-     * Return an array describing any problems there are with an expression.
-     * Returns false if the expression is fine.
-     * @param string $formula an expression.
-     * @return array|false list of problems, or false if the exression is OK.
-     */
-    public function get_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 (preg_match('~\\{[[:alpha:]][^>} <{"\']*\\}~', $formula, $regs)) {
-            $formula = str_replace($regs[0], '1', $formula);
-        }
-
-        // Strip away empty space and lowercase it.
-        $formula = strtolower(str_replace(' ', '', $formula));
-
-        $safeoperatorchar = '-+/*%>:^\~<?=&|!'; /* */
-        $operatorornumber = "[$safeoperatorchar.0-9eE]";
-
-        while (preg_match("~(^|[$safeoperatorchar,(])([a-z0-9_]*)" .
-                "\\(($operatorornumber+(,$operatorornumber+((,$operatorornumber+)+)?)?)?\\)~",
-            $formula, $regs)) {
-            switch ($regs[2]) {
-                // Simple parenthesis.
-                case '':
-                    if ((isset($regs[4]) && $regs[4]) || strlen($regs[3]) == 0) {
-                        return get_string('illegalformulasyntax', 'qtype_calculated', $regs[0]);
-                    }
-                    break;
-
-                    // Zero argument functions.
-                case 'pi':
-                    if ($regs[3]) {
-                        return get_string('functiontakesnoargs', 'qtype_calculated', $regs[2]);
-                    }
-                    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 (!empty($regs[4]) || empty($regs[3])) {
-                        return get_string('functiontakesonearg', 'qtype_calculated', $regs[2]);
-                    }
-                    break;
-
-                    // Functions that take one or two arguments.
-                case 'log': case 'round':
-                    if (!empty($regs[5]) || empty($regs[3])) {
-                        return get_string('functiontakesoneortwoargs', 'qtype_calculated',
-                                $regs[2]);
-                    }
-                    break;
-
-                    // Functions that must have two arguments.
-                case 'atan2': case 'fmod': case 'pow':
-                    if (!empty($regs[5]) || empty($regs[4])) {
-                        return get_string('functiontakestwoargs', 'qtype_calculated', $regs[2]);
-                    }
-                    break;
-
-                    // Functions that take two or more arguments.
-                case 'min': case 'max':
-                    if (empty($regs[4])) {
-                        return get_string('functiontakesatleasttwo', 'qtype_calculated', $regs[2]);
-                    }
-                    break;
-
-                default:
-                    return get_string('unsupportedformulafunction', 'qtype_calculated', $regs[2]);
-            }
-
-            // Exchange the function call with '1' and then check for another function call.
-
-            if ($regs[1]) {
-                // The function call is proceeded by an operator.
-                $formula = str_replace($regs[0], $regs[1] . '1', $formula);
-            } else {
-                // The function call starts the formula.
-                $formula = preg_replace("~^$regs[2]\\([^)]*\\)~", '1', $formula);
-            }
-        }
-
-        if (preg_match("~[^$safeoperatorchar.0-9eE]+~", $formula, $regs)) {
-            return get_string('illegalformulasyntax', 'qtype_calculated', $regs[0]);
-        } else {
-            // Formula just might be valid.
-            return false;
-        }
-    }
-}
\ No newline at end of file
+}

@@ -1890,6 +1890,11 @@ function qtype_calculated_calculate_answer($formula, $individualdata,
 }
 
 
+/**
+ * Validate a forumula.
+ * @param string $formula the formula to validate.
+ * @return string|boolean false if there are no problems. Otherwise a string error message.
+ */
 function qtype_calculated_find_formula_errors($formula) {
     // Validates the formula submitted from the question edit page.
     // Returns false if everything is alright
@@ -1918,7 +1923,7 @@ function qtype_calculated_find_formula_errors($formula) {
 
                 // Zero argument functions.
             case 'pi':
-                if ($regs[3]) {
+                if (array_key_exists(3, $regs)) {
                     return get_string('functiontakesnoargs', 'qtype_calculated', $regs[2]);
                 }
                 break;

@@ -0,0 +1,86 @@
+<?php
+// This file is part of Moodle - http://moodle.org/
+//
+// Moodle is free software: you can redistribute it and/or modify
+// it under the terms of the GNU General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+//
+// Moodle is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+//
+// You should have received a copy of the GNU General Public License
+// along with Moodle.  If not, see <http://www.gnu.org/licenses/>.
+
+/**
+ * Unit tests for formula validation code.
+ *
+ * @package    qtype_calculated
+ * @copyright  2014 The Open University
+ * @license    http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
+ */
+
+
+defined('MOODLE_INTERNAL') || die();
+
+global $CFG;
+require_once($CFG->dirroot . '/question/type/calculated/questiontype.php');
+
+
+/**
+ * Unit tests for formula validation code.
+ *
+ * @copyright  2014 The Open University
+ * @license    http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
+ */
+class qtype_calculated_formula_validation_testcase extends basic_testcase {
+    protected function assert_nonempty_string($actual) {
+        $this->assertInternalType('string', $actual);
+        $this->assertNotEquals('', $actual);
+    }
+
+    public function test_simple_equations_ok() {
+        $this->assertFalse(qtype_calculated_find_formula_errors(1));
+        $this->assertFalse(qtype_calculated_find_formula_errors('1 + 1'));
+        $this->assertFalse(qtype_calculated_find_formula_errors('{x} + {y}'));
+        $this->assertFalse(qtype_calculated_find_formula_errors('{x}*{y}'));
+    }
+
+    public function test_safe_functions_ok() {
+        $this->assertFalse(qtype_calculated_find_formula_errors('abs(-1)'));
+        $this->assertFalse(qtype_calculated_find_formula_errors('tan(pi())'));
+        $this->assertFalse(qtype_calculated_find_formula_errors('log(10)'));
+        $this->assertFalse(qtype_calculated_find_formula_errors('log(64, 2)'));
+        $this->assertFalse(qtype_calculated_find_formula_errors('atan2(1.0, 1.0)'));
+        $this->assertFalse(qtype_calculated_find_formula_errors('max(1.0, 1.0)'));
+        $this->assertFalse(qtype_calculated_find_formula_errors('max(1.0, 1.0, 2.0)'));
+        $this->assertFalse(qtype_calculated_find_formula_errors('max(1.0, 1.0, 2, 3)'));
+    }
+
+    public function test_dangerous_functions_blocked() {
+        $this->assert_nonempty_string(qtype_calculated_find_formula_errors('eval(1)'));
+        $this->assert_nonempty_string(qtype_calculated_find_formula_errors('system(1)'));
+        $this->assert_nonempty_string(qtype_calculated_find_formula_errors('base64_decode(1)'));
+        $this->assert_nonempty_string(qtype_calculated_find_formula_errors('unserialize(1)'));
+
+        $this->assert_nonempty_string(qtype_calculated_find_formula_errors('cos(tan(1) + abs(cos(eval)) * pi())'));
+        $this->assert_nonempty_string(qtype_calculated_find_formula_errors('eval (CONSTANTREADASSTRING)'));
+        $this->assert_nonempty_string(qtype_calculated_find_formula_errors("eval \t ()"));
+        $this->assert_nonempty_string(qtype_calculated_find_formula_errors('"eval"()'));
+        $this->assert_nonempty_string(qtype_calculated_find_formula_errors('?><?php()'));
+        $this->assert_nonempty_string(qtype_calculated_find_formula_errors('?><?php+1'));
+    }
+
+    public function test_functions_with_wrong_num_args_caught() {
+        $this->assert_nonempty_string(qtype_calculated_find_formula_errors('abs(-1, 1)'));
+        $this->assert_nonempty_string(qtype_calculated_find_formula_errors('abs()'));
+        $this->assert_nonempty_string(qtype_calculated_find_formula_errors('pi(1)'));
+        $this->assert_nonempty_string(qtype_calculated_find_formula_errors('log()'));
+        $this->assert_nonempty_string(qtype_calculated_find_formula_errors('log(64, 2, 3)'));
+        $this->assert_nonempty_string(qtype_calculated_find_formula_errors('atan2(1.0)'));
+        $this->assert_nonempty_string(qtype_calculated_find_formula_errors('atan2(1.0, 1.0, 2.0)'));
+        $this->assert_nonempty_string(qtype_calculated_find_formula_errors('max(1.0)'));
+    }
+}