Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-55720

Potentially undefined exception object in course/modlib.php

    XMLWordPrintable

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 3.1.1, 3.2, 3.3
    • Fix Version/s: 3.1.4, 3.2.1
    • Component/s: Course
    • Labels:
    • Testing Instructions:
      Hide
      1. Edit mod/assign.php#assign_add_instance such that it immediately returns the string "asdf"
      2. Create a course and attempt to add an assignment
      3. After submission of the assignment form you'll get some errors (assuming dev mode is enabled)
      4. Verify the there is no mention of undefined variables
      Show
      Edit mod/assign.php#assign_add_instance such that it immediately returns the string "asdf" Create a course and attempt to add an assignment After submission of the assignment form you'll get some errors (assuming dev mode is enabled) Verify the there is no mention of undefined variables
    • Affected Branches:
      MOODLE_31_STABLE, MOODLE_32_STABLE, MOODLE_33_STABLE
    • Fixed Branches:
      MOODLE_31_STABLE, MOODLE_32_STABLE
    • Pull from Repository:
    • Pull Master Branch:

      Description

      Consider this code snippet from course/modlib.php

          try {
              $returnfromfunc = $addinstancefunction($moduleinfo, $mform);
          } catch (moodle_exception $e) {
              $returnfromfunc = $e;
          }
          if (!$returnfromfunc or !is_number($returnfromfunc)) {
              // Undo everything we can. This is not necessary for databases which
              // support transactions, but improves consistency for other databases.
              $modcontext = context_module::instance($moduleinfo->coursemodule);
              context_helper::delete_instance(CONTEXT_MODULE, $moduleinfo->coursemodule);
              $DB->delete_records('course_modules', array('id'=>$moduleinfo->coursemodule));
       
              if ($e instanceof moodle_exception) {
                  throw $e;
              } else if (!is_number($returnfromfunc)) {
                  print_error('invalidfunction', '', course_get_url($course, $moduleinfo->section));
              } else {
                  print_error('cannotaddnewmodule', '', course_get_url($course, $moduleinfo->section), $moduleinfo->modulename);
              }
          }
      

      It's entirely possible to reach the test for variable $e (if ($e instanceof...) without $e being defined thus throwing an error. The 'try' at the start of the snippet could quite easily return $returnfromfunc as a 'false' value without throwing an exception... in which case $e would not exist.

      The test for $e needs an additional isset() or similar check to be safe.

      Turned up in a help request in the forums.

      Just my $00.02 but (unless you are dealing with some external system over which you have no control) try...catch is almost always a very bad idea and should be removed.

        Attachments

          Activity

            People

            Assignee:
            ak4t0sh Arnaud Trouvé
            Reporter:
            howardsmiller Howard Miller
            Peer reviewer:
            cameron1729
            Integrator:
            Eloy Lafuente (stronk7)
            Tester:
            Damyon Wiese
            Participants:
            Component watchers:
            Amaia Anabitarte, Carlos Escobedo, Ferran Recio, Sara Arjona (@sarjona), Víctor Déniz Falcón
            Votes:
            0 Vote for this issue
            Watchers:
            6 Start watching this issue

              Dates

              Created:
              Updated:
              Resolved:
              Fix Release Date:
              9/Jan/17