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

Course copy controllers can be instantiated in an invalid state

    XMLWordPrintable

Details

    • MOODLE_311_STABLE, MOODLE_400_STABLE
    • MOODLE_311_STABLE, MOODLE_400_STABLE
    • MDL-74548-master
    • Hide

      Regression testing

      Repeat the testing instructions from MDL-64843

      Test the race condition is gone

      1. Create a course
      2. Login as admin in two separate sessions (e.g., a regular browser session and an incognito/private session)
      3. Edit backup/util/helper/copy_helper.class.php:
        1. Find the create_copy method
        2. Add sleep(100); on the line before the comment // Create the initial restore controller.
      4. In one of the sessions navigate to the course copy page, fill it out and submit
      5. While it's sleeping, in the other session access the course copy form
      6. Verify no errors are displayed on the screen
      7. Verify that after 100 seconds have passed you can see the course copy on the copy progress screen

      Test the cleanup task

      1. Remove the code edit from the previous test
      2. Browse to "Site administration" > "Courses" > "Asynchronous backup/restore" and tick the "Enable asynchronous backups"checkbox
      3. Create two courses (we'll call them A and B)
      4. In course A and B create a backup and restore it (you will need to run cron to get the backup and restore to process)
      5. In course A and course B initiate a copy and run cron to process them
      6. In course A and B create a backup but do not run cron to process them
      7. In course A and B initiate a copy but do not run cron to process them
      8. In initiate a copy in only course B but do not run cron to process it
      9. View the copy progress page (refresh it if needed) for both of the courses ([YOUR_MOODLE]/backup/copyprogress.php?id=[COURSE_ID]) and verify that:
        • Course A shows one copy
        • Course B shows two copies
        • There are no errors
      10. Manually edit the DB record for the most recent backup controller in the mdl_backup_controllers table (sort them by ID and find the restore controller with the largest ID) setting the status to 900 to simulate a failure

        select max(id) from mdl_backup_controllers where operation = 'backup';
        update mdl_backup_controllers set status = 900 where id = YOURBACKUPID;
        

      11. Take note of the statuses of all the other controllers (best to take a screenshot or similar)

        select id, operation, status from mdl_backup_controllers order by id;
        

      12. Wait for one minute and run the cleanup task with:

        php admin/cli/scheduled_task.php --execute=\\core\\task\\backup_cleanup_task
        

      13. Check the mdl_backup_controllers table and verify that the most recent restore now has status 900

        select max(id) from mdl_backup_controllers where operation = 'restore';
        select id, operation, status from mdl_backup_controllers where id = YOURRESTOREID;
        

      14. Compare the current state of the table with the screenshot and verify that no other controllers have changed
      15. Check the copy progress page (refresh it if needed) for each course and verify:
        • Course A shows one copy
        • Course B shows one copy
        • There are no errors
      16. Manually edit the DB record for the most recent backup controller again (the same backup record from earlier), setting the status back to 700

        update mdl_backup_controllers set status = 700 where id = YOURBACKUPID;
        

      17. Delete the most recent restore controller

        select max(id) from mdl_backup_controllers where operation = 'restore';
        delete from mdl_backup_controllers where id = YOURRESTOREID;
        

      18. Check the copy progress page (refresh it if needed) for each course and verify:
        • Course A shows one copy
        • Course B shows one copy
        • There are no errors
      19. Run the cleanup task with:

        php admin/cli/scheduled_task.php --execute=\\core\\task\\backup_cleanup_task
        

      20. Check the DB and verify that the most recent backup now has its status set to 900

        select max(id) from mdl_backup_controllers where operation = 'backup';
        select id, operation, status from mdl_backup_controllers where id = YOURRESTOREID;
        

      21. Compare with the screenshot from earlier and verify no other controllers have changed
      22. Check the copy progress page for each course and verify:
        • Course A shows one copy
        • Course B shows one copy
        • There are no errors
      23. Run cron to process the remaining backups/restores/copies and verify it completes successfully
      24. Check the copy progress page for each course (refresh it if needed) and verify:
        • Course A shows no copies
        • Course B shows no copies
        • There are no errors
      Show
      Regression testing Repeat the testing instructions from MDL-64843 Test the race condition is gone Create a course Login as admin in two separate sessions (e.g., a regular browser session and an incognito/private session) Edit backup/util/helper/copy_helper.class.php: Find the create_copy method Add sleep(100); on the line before the comment // Create the initial restore controller. In one of the sessions navigate to the course copy page, fill it out and submit While it's sleeping, in the other session access the course copy form Verify no errors are displayed on the screen Verify that after 100 seconds have passed you can see the course copy on the copy progress screen Test the cleanup task Remove the code edit from the previous test Browse to "Site administration" > "Courses" > "Asynchronous backup/restore" and tick the "Enable asynchronous backups"checkbox Create two courses (we'll call them A and B) In course A and B create a backup and restore it (you will need to run cron to get the backup and restore to process) In course A and course B initiate a copy and run cron to process them In course A and B create a backup but do not run cron to process them In course A and B initiate a copy but do not run cron to process them In initiate a copy in only course B but do not run cron to process it View the copy progress page (refresh it if needed) for both of the courses ( [YOUR_MOODLE] /backup/copyprogress.php?id= [COURSE_ID] ) and verify that: Course A shows one copy Course B shows two copies There are no errors Manually edit the DB record for the most recent backup controller in the mdl_backup_controllers table (sort them by ID and find the restore controller with the largest ID) setting the status to 900 to simulate a failure select max(id) from mdl_backup_controllers where operation = 'backup' ; update mdl_backup_controllers set status = 900 where id = YOURBACKUPID; Take note of the statuses of all the other controllers (best to take a screenshot or similar) select id, operation, status from mdl_backup_controllers order by id; Wait for one minute and run the cleanup task with: php admin/cli/scheduled_task.php --execute=\\core\\task\\backup_cleanup_task Check the mdl_backup_controllers table and verify that the most recent restore now has status 900 select max(id) from mdl_backup_controllers where operation = 'restore' ; select id, operation, status from mdl_backup_controllers where id = YOURRESTOREID; Compare the current state of the table with the screenshot and verify that no other controllers have changed Check the copy progress page (refresh it if needed) for each course and verify : Course A shows one copy Course B shows one copy There are no errors Manually edit the DB record for the most recent backup controller again (the same backup record from earlier), setting the status back to 700 update mdl_backup_controllers set status = 700 where id = YOURBACKUPID; Delete the most recent restore controller select max(id) from mdl_backup_controllers where operation = 'restore' ; delete from mdl_backup_controllers where id = YOURRESTOREID; Check the copy progress page (refresh it if needed) for each course and verify : Course A shows one copy Course B shows one copy There are no errors Run the cleanup task with: php admin/cli/scheduled_task.php --execute=\\core\\task\\backup_cleanup_task Check the DB and verify that the most recent backup now has its status set to 900 select max(id) from mdl_backup_controllers where operation = 'backup' ; select id, operation, status from mdl_backup_controllers where id = YOURRESTOREID; Compare with the screenshot from earlier and verify no other controllers have changed Check the copy progress page for each course and verify : Course A shows one copy Course B shows one copy There are no errors Run cron to process the remaining backups/restores/copies and verify it completes successfully Check the copy progress page for each course (refresh it if needed) and verify : Course A shows no copies Course B shows no copies There are no errors

    Description

      tl;dr

      When a backup/restore controller is instantiated with MODE_COPY (e.g., in create_copy) there needs to be a guarantee that the copy data will be set from the point of instantiation, otherwise the class is in an invalid state.

      Long version

      Classes extending base_controller can be instantiated without providing data for the copy member. This creates a situation where the get_copy method can attempt to return null (which will cause an exception to be thrown).

      When a controller has MODE_COPY set, there's an expectation that the copy member is also set (and therefore it should be safe to call get_copy). However, there is no guarantee that copy will be set. See https://github.com/moodle/moodle/blob/master/backup/util/ui/classes/copy/copy.php - it makes many calls to get_copy because it expects it should be safe to do so.

      There's at least one place in moodle where this can cause problems:

      backup/util/ui/classes/copy/copy.php

      137
      public function create_copy(): array {
      138
          global $USER;
      139
          $copyids = array();
      140
       
      141
          // Create the initial backupcontoller.
      142
          $bc = new \backup_controller(\backup::TYPE_1COURSE, $this->copydata->courseid, \backup::FORMAT_MOODLE,
      143
                  \backup::INTERACTIVE_NO, \backup::MODE_COPY, $USER->id, \backup::RELEASESESSION_YES);
      144
       
      145
          // a bunch of stuff here ...
      146
       
      147
          $bc->set_copy($copydata);
      148
          $bc->set_status(\backup::STATUS_AWAITING);
      

      When the backup controller is instantiated, it is serialised and saved (see backup_controller's constructor which conditionally calls save_controller, ultimately serialising the class and saving it in the DB) without data for the copy member being set.

      This creates a window where it's possible to call get_copy while the copy member is not set, and an exception will be thrown. This can potentially happen in core_backup\copy\copy::get_copies. To produce the error:

      1. Create a course
      2. Login as admin in two separate sessions (e.g., a regular browser session and an incognito/private session)
      3. Edit backup/util/ui/classes/copy/copy.php and add sleep(100); on the line before $bc->set_copy($copydata);
      4. In one of the sessions navigate to the course copy page, fill it out and submit
      5. While it's sleeping, in the other session access the course copy form
      6. You should see the error

      A potential solution is to refactor the base_controller class such that the data for the copy member is optionally provided as a constructor parameter, this way the data will exist before the controller is ever serialised and saved. Hence it should always be present in the serialised data. Another (probably better) option could be to extend the controller class, creating copy_controller, which explicitly requires copy data be passed in through the constructor.

      Attachments

        1. MDL-74548_cleanup_1.png
          MDL-74548_cleanup_1.png
          67 kB
        2. MDL-74548_cleanup_2.png
          MDL-74548_cleanup_2.png
          62 kB
        3. MDL-74548_cleanup_3.png
          MDL-74548_cleanup_3.png
          113 kB
        4. MDL-74548_cleanup_4.png
          MDL-74548_cleanup_4.png
          56 kB
        5. MDL-74548_cleanup_5.png
          MDL-74548_cleanup_5.png
          54 kB
        6. MDL-74548_cleanup_6.png
          MDL-74548_cleanup_6.png
          61 kB
        7. MDL-74548_cleanup_7.png
          MDL-74548_cleanup_7.png
          89 kB
        8. MDL-74548_cleanup_8.png
          MDL-74548_cleanup_8.png
          55 kB
        9. MDL-74548_cleanup_9.png
          MDL-74548_cleanup_9.png
          81 kB
        10. MDL-74548_race condition_1.png
          MDL-74548_race condition_1.png
          104 kB
        11. MDL-74548_race condition_2.png
          MDL-74548_race condition_2.png
          63 kB
        12. MDL-74548_regression_course management_1.png
          MDL-74548_regression_course management_1.png
          155 kB
        13. MDL-74548_regression_course management_2.png
          MDL-74548_regression_course management_2.png
          162 kB
        14. MDL-74548_regression_course management_3.png
          MDL-74548_regression_course management_3.png
          118 kB
        15. MDL-74548_regression_source_1.png
          MDL-74548_regression_source_1.png
          142 kB
        16. MDL-74548_regression_source_2.png
          MDL-74548_regression_source_2.png
          152 kB
        17. MDL-74548_regression_source_3.png
          MDL-74548_regression_source_3.png
          121 kB

        Issue Links

          Activity

            People

              cameron1729 cameron1729
              cameron1729 cameron1729
              Peter Burnett Peter Burnett
              Ilya Tregubov Ilya Tregubov
              Angelia Dela Cruz Angelia Dela Cruz
              Votes:
              1 Vote for this issue
              Watchers:
              8 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved:

                Time Tracking

                  Estimated:
                  Original Estimate - 0 minutes
                  0m
                  Remaining:
                  Remaining Estimate - 0 minutes
                  0m
                  Logged:
                  Time Spent - 1 day, 4 hours, 30 minutes
                  1d 4h 30m

                  Clockify

                    Error rendering 'clockify-timesheets-time-tracking-reports:timer-sidebar'. Please contact your Jira administrators.