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

Race condition in forum delete code has potential to delete entire moodledata directory

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Blocker
    • Resolution: Fixed
    • Affects Version/s: 1.9.5
    • Fix Version/s: 1.6.9+, 1.7.7+, 1.8.10, 1.9.6
    • Component/s: Forum
    • Labels:
      None
    • Affected Branches:
      MOODLE_19_STABLE
    • Fixed Branches:
      MOODLE_16_STABLE, MOODLE_17_STABLE, MOODLE_18_STABLE, MOODLE_19_STABLE

      Description

      This bug is very difficult to reproduce under "real world" conditions, but we had this bug bite us on a production system and delete course data for over 100 courses. We had to recover the deleted course data from tape.

      In mod/forum/lib.php, there is a sequence of calls when deleting a forum activity, which cleans up attachments:

      forum_delete_instance() -> forum_delete_discussion() -> forum_delete_post() -> forum_delete_old_attachments()

      The forum_delete_old_attachments function calls forum_file_area to get the path the the attachment directory, and then procedes to do an effective 'rm -rf <dir>'.

      The bug is in the forum_file_area() function, this line:

      return make_upload_directory( forum_file_area_name($post) );

      There is no error checking on the return value of forum_file_area_name, which returns false if the parent discussion no longer exists in the database.

      make_upload_directory(false) returns the moodledata directory, so the attachment cleanup code will remove all moodledata!

      This happened on a production instance that we host, because two simultaneous deletes of the same course were happening at the same time. It has since been very difficult to reproduce that situation, however here are some steps to demonstrate the bug by manually changing some values in the database.

      • Make a new forum
      • Post to it
      • Reply to that post with another post containing an attachment
      • Now in the database, update the record for the 2nd post and set discussion = 0 (this gets the forum_file_area_name function to return false).
      • Delete the forum or whole course, and watch as your moodledata directory gets entirely erased.

      In the real world, the bug occurs only if the parent discussion record gets deleted before the forum_file_area_name() function executes (eg. multiple simultaneous deletes are happening).

      I've attached a patch which does some error checking in the forum_file_area() function to fix the problem.

      Although this bug is an extremely rare occurence due to the intricate timing involved, it is definately unsafe to assume forum_file_area_name() will not return false, considering it has two "return false" statements in it.

        Gliffy Diagrams

          Activity

          Hide
          skodak Petr Skoda added a comment -

          scary! thanks a lot for the report!

          Show
          skodak Petr Skoda added a comment - scary! thanks a lot for the report!
          Hide
          skodak Petr Skoda added a comment -

          oops, committed into cvs, thanks again

          Show
          skodak Petr Skoda added a comment - oops, committed into cvs, thanks again
          Hide
          samhemelryk Sam Hemelryk added a comment -

          Thanks Petr and Ashley for the fix there, all appears fixed

          Show
          samhemelryk Sam Hemelryk added a comment - Thanks Petr and Ashley for the fix there, all appears fixed

            People

            • Votes:
              8 Vote for this issue
              Watchers:
              9 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:
                Fix Release Date:
                31/Jul/09