Moodle
  1. Moodle
  2. MDL-10905

Backup misses files if a directory name evaluates to false (eg. "0")

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 1.8.2, 1.9, 2.0
    • Fix Version/s: 1.6.6, 1.7.4, 1.8.4, 1.9, 2.0
    • Component/s: Backup, Other
    • Labels:
      None
    • Affected Branches:
      MOODLE_18_STABLE, MOODLE_19_STABLE, MOODLE_20_STABLE
    • Fixed Branches:
      MOODLE_16_STABLE, MOODLE_17_STABLE, MOODLE_18_STABLE, MOODLE_19_STABLE, MOODLE_20_STABLE
    • Rank:
      28701

      Description

      The backup_copy_dir and delete_dir_contents functions in the backup/lib.php file do not work correctly if there is a directory (or file?) name that evaluates to false (eg. "0"). This will result in files not being copied for backup or deleted correctly. In those functions, there is a while loop that uses
      $file=readdir($dir)
      as its condition which can return valid files that evaluate to false. It is handled correctly in other files with something like
      false !== ($file = readdir($handle))

      There are other occurrences in the code of this bug including admin/delete.php: delete_subdirectories and backup/bb/restore_bb.php: get_subdirs. There may be others as well.

        Issue Links

          Activity

          Hide
          Eloy Lafuente (stronk7) added a comment -

          Hi Scott,

          but that only can happen if we are talking about relative paths (where a "0" dir name can have the explained effect).... but all those Moodle functions are running against absolute paths (where $CFG->dataroot is prepended).

          So, perhaps it's impossible in real life?

          Thanks and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Hi Scott, but that only can happen if we are talking about relative paths (where a "0" dir name can have the explained effect).... but all those Moodle functions are running against absolute paths (where $CFG->dataroot is prepended). So, perhaps it's impossible in real life? Thanks and ciao
          Hide
          Scott Turner added a comment - - edited

          This can be duplicated by going to the course files and adding a directory named "0". When the data is backed up, that directory and all files/directories added after it will not be in the backup. If the name of the directory is changed from "0" the backup will proceed correctly.

          Those functions may all run with absolute paths, but internally they do not use them. readdir returns a filename and not a path, so the code that uses:

          $file=readdir($dir)

          as a condition will fail incorrectly at times. The php manual makes an explicit note of this. (http://us3.php.net/readdir)

          Show
          Scott Turner added a comment - - edited This can be duplicated by going to the course files and adding a directory named "0". When the data is backed up, that directory and all files/directories added after it will not be in the backup. If the name of the directory is changed from "0" the backup will proceed correctly. Those functions may all run with absolute paths, but internally they do not use them. readdir returns a filename and not a path, so the code that uses: $file=readdir($dir) as a condition will fail incorrectly at times. The php manual makes an explicit note of this. ( http://us3.php.net/readdir )
          Hide
          Eloy Lafuente (stronk7) added a comment -

          100% agree. Going to propose this to be changed across Moodle code... thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - 100% agree. Going to propose this to be changed across Moodle code... thanks!
          Hide
          Martin Dougiamas added a comment -

          While you're looking at files, Nicolas, can you look into this?

          Show
          Martin Dougiamas added a comment - While you're looking at files, Nicolas, can you look into this?
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Raising this to blokcer because of MDL-8605. That upgrade can be breaking sites using the new "user/0" folder.

          Show
          Eloy Lafuente (stronk7) added a comment - Raising this to blokcer because of MDL-8605 . That upgrade can be breaking sites using the new "user/0" folder.
          Hide
          Nicolas Connault added a comment - - edited

          This actually affects all versions from 1.6 onwards: 13 files (the same in each version) have incorrect iterations through readdir(). I fixed them all by simply adding ...

          false !== ([code])

          ... as suggested in the php doc.

          Show
          Nicolas Connault added a comment - - edited This actually affects all versions from 1.6 onwards: 13 files (the same in each version) have incorrect iterations through readdir(). I fixed them all by simply adding ... false !== ( [code] ) ... as suggested in the php doc.
          Hide
          Nicolas Connault added a comment -

          All fixed

          Show
          Nicolas Connault added a comment - All fixed
          Hide
          Andrew Davis added a comment -

          Fixes look good and backup is correctly handling a dir named "0". Closing.

          Show
          Andrew Davis added a comment - Fixes look good and backup is correctly handling a dir named "0". Closing.

            People

            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: