Skip to content
This repository

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse code

MDL-34251 question engine: possible infinite loop loading usages

In the case where either a question_attempt had not steps, or a
question_usage had not question_attempts, the load_from_records methods
could get stuck in an infinite loop.

This fix ensures that does not happen, with unit tests to verify it. At
the same time, I noticed an error in the existing tests, which this
patch fixes.
  • Loading branch information...
commit 2d7cbf59234c2d8d1976bd87d619e33405d1d600 1 parent 9592f5d
Tim Hunt timhunt authored
9 question/engine/questionattempt.php
@@ -1213,6 +1213,15 @@ public static function load_from_records($records, $questionattemptid,
1213 1213 $qa->behaviour = question_engine::make_behaviour(
1214 1214 $record->behaviour, $qa, $preferredbehaviour);
1215 1215
  1216 + // If attemptstepid is null (which should not happen, but has happened
  1217 + // due to corrupt data, see MDL-34251) then the current pointer in $records
  1218 + // will not be advanced in the while loop below, and we get stuck in an
  1219 + // infinite loop, since this method is supposed to always consume at
  1220 + // least one record. Therefore, in this case, advance the record here.
  1221 + if (is_null($record->attemptstepid)) {
  1222 + $records->next();
  1223 + }
  1224 +
1216 1225 $i = 0;
1217 1226 while ($record && $record->questionattemptid == $questionattemptid && !is_null($record->attemptstepid)) {
1218 1227 $qa->steps[$i] = question_attempt_step::load_from_records($records, $record->attemptstepid);
8 question/engine/questionusage.php
@@ -714,6 +714,14 @@ public static function load_from_records($records, $qubaid) {
714 714
715 715 $quba->observer = new question_engine_unit_of_work($quba);
716 716
  717 + // If slot is null then the current pointer in $records will not be
  718 + // advanced in the while loop below, and we get stuck in an infinite loop,
  719 + // since this method is supposed to always consume at least one record.
  720 + // Therefore, in this case, advance the record here.
  721 + if (is_null($record->slot)) {
  722 + $records->next();
  723 + }
  724 +
717 725 while ($record && $record->qubaid == $qubaid && !is_null($record->slot)) {
718 726 $quba->questionattempts[$record->slot] =
719 727 question_attempt::load_from_records($records,
68 question/engine/tests/questionusagebyactivity_test.php
@@ -169,16 +169,17 @@ public function test_access_out_of_sequence_throws_exception() {
169 169 */
170 170 class question_usage_db_test extends data_loading_method_test_base {
171 171 public function test_load() {
  172 + $scid = context_system::instance()->id;
172 173 $records = new question_test_recordset(array(
173 174 array('qubaid', 'contextid', 'component', 'preferredbehaviour',
174   - 'questionattemptid', 'contextid', 'questionusageid', 'slot',
  175 + 'questionattemptid', 'questionusageid', 'slot',
175 176 'behaviour', 'questionid', 'variant', 'maxmark', 'minfraction', 'flagged',
176 177 'questionsummary', 'rightanswer', 'responsesummary', 'timemodified',
177 178 'attemptstepid', 'sequencenumber', 'state', 'fraction',
178 179 'timecreated', 'userid', 'name', 'value'),
179   - array(1, 1, 'unit_test', 'interactive', 1, 123, 1, 1, 'interactive', -1, 1, 2.0000000, 0.0000000, 0, '', '', '', 1256233790, 1, 0, 'todo', null, 1256233700, 1, null, null),
180   - array(1, 1, 'unit_test', 'interactive', 1, 123, 1, 1, 'interactive', -1, 1, 2.0000000, 0.0000000, 0, '', '', '', 1256233790, 2, 1, 'todo', null, 1256233705, 1, 'answer', '1'),
181   - array(1, 1, 'unit_test', 'interactive', 1, 123, 1, 1, 'interactive', -1, 1, 2.0000000, 0.0000000, 0, '', '', '', 1256233790, 5, 2, 'gradedright', 1.0000000, 1256233720, 1, '-finish', '1'),
  180 + array(1, $scid, 'unit_test', 'interactive', 1, 1, 1, 'interactive', -1, 1, 2.0000000, 0.0000000, 0, '', '', '', 1256233790, 1, 0, 'todo', null, 1256233700, 1, null, null),
  181 + array(1, $scid, 'unit_test', 'interactive', 1, 1, 1, 'interactive', -1, 1, 2.0000000, 0.0000000, 0, '', '', '', 1256233790, 2, 1, 'todo', null, 1256233705, 1, 'answer', '1'),
  182 + array(1, $scid, 'unit_test', 'interactive', 1, 1, 1, 'interactive', -1, 1, 2.0000000, 0.0000000, 0, '', '', '', 1256233790, 5, 2, 'gradedright', 1.0000000, 1256233720, 1, '-finish', '1'),
182 183 ));
183 184
184 185 $question = test_question_maker::make_question('truefalse', 'true');
@@ -221,4 +222,63 @@ public function test_load() {
221 222 $this->assertEquals(1, $step->get_user_id());
222 223 $this->assertEquals(array('-finish' => '1'), $step->get_all_data());
223 224 }
  225 +
  226 + public function test_load_data_no_steps() {
  227 + // The code had a bug where if one question_attempt had no steps,
  228 + // load_from_records got stuck in an infinite loop. This test is to
  229 + // verify that no longer happens.
  230 + $scid = context_system::instance()->id;
  231 + $records = new question_test_recordset(array(
  232 + array('qubaid', 'contextid', 'component', 'preferredbehaviour',
  233 + 'questionattemptid', 'questionusageid', 'slot',
  234 + 'behaviour', 'questionid', 'variant', 'maxmark', 'minfraction', 'flagged',
  235 + 'questionsummary', 'rightanswer', 'responsesummary', 'timemodified',
  236 + 'attemptstepid', 'sequencenumber', 'state', 'fraction',
  237 + 'timecreated', 'userid', 'name', 'value'),
  238 + array(1, $scid, 'unit_test', 'interactive', 1, 1, 1, 'interactive', 0, 1, 1.0000000, 0.0000000, 0, 'This question is missing. Unable to display anything.', '', '', 0, null, null, null, null, null, null, null, null),
  239 + array(1, $scid, 'unit_test', 'interactive', 2, 1, 2, 'interactive', 0, 1, 1.0000000, 0.0000000, 0, 'This question is missing. Unable to display anything.', '', '', 0, null, null, null, null, null, null, null, null),
  240 + array(1, $scid, 'unit_test', 'interactive', 3, 1, 3, 'interactive', 0, 1, 1.0000000, 0.0000000, 0, 'This question is missing. Unable to display anything.', '', '', 0, null, null, null, null, null, null, null, null),
  241 + ));
  242 +
  243 + question_bank::start_unit_test();
  244 + $quba = question_usage_by_activity::load_from_records($records, 1);
  245 + question_bank::end_unit_test();
  246 +
  247 + $this->assertEquals('unit_test', $quba->get_owning_component());
  248 + $this->assertEquals(1, $quba->get_id());
  249 + $this->assertInstanceOf('question_engine_unit_of_work', $quba->get_observer());
  250 + $this->assertEquals('interactive', $quba->get_preferred_behaviour());
  251 +
  252 + $this->assertEquals(array(1, 2, 3), $quba->get_slots());
  253 +
  254 + $qa = $quba->get_question_attempt(1);
  255 + $this->assertEquals(0, $qa->get_num_steps());
  256 + }
  257 +
  258 + public function test_load_data_no_qas() {
  259 + // The code had a bug where if a question_usage had no question_attempts,
  260 + // load_from_records got stuck in an infinite loop. This test is to
  261 + // verify that no longer happens.
  262 + $scid = context_system::instance()->id;
  263 + $records = new question_test_recordset(array(
  264 + array('qubaid', 'contextid', 'component', 'preferredbehaviour',
  265 + 'questionattemptid', 'questionusageid', 'slot',
  266 + 'behaviour', 'questionid', 'variant', 'maxmark', 'minfraction', 'flagged',
  267 + 'questionsummary', 'rightanswer', 'responsesummary', 'timemodified',
  268 + 'attemptstepid', 'sequencenumber', 'state', 'fraction',
  269 + 'timecreated', 'userid', 'name', 'value'),
  270 + array(1, $scid, 'unit_test', 'interactive', null, null, null, null, null, null, null, null, null, null, null, null, null, null, null, null, null, null, null, null, null),
  271 + ));
  272 +
  273 + question_bank::start_unit_test();
  274 + $quba = question_usage_by_activity::load_from_records($records, 1);
  275 + question_bank::end_unit_test();
  276 +
  277 + $this->assertEquals('unit_test', $quba->get_owning_component());
  278 + $this->assertEquals(1, $quba->get_id());
  279 + $this->assertInstanceOf('question_engine_unit_of_work', $quba->get_observer());
  280 + $this->assertEquals('interactive', $quba->get_preferred_behaviour());
  281 +
  282 + $this->assertEquals(array(), $quba->get_slots());
  283 + }
224 284 }

0 comments on commit 2d7cbf5

Please sign in to comment.
Something went wrong with that request. Please try again.