Moodle

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

Details

  • Type: Bug Bug
  • Status: Closed 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

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

This issue will help resolve:
MDL-8605 Reduce the number of dirs under the users file area Blocker Closed

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
Nicolas Connault added a comment -

All fixed

Show
Nicolas Connault added a comment - All fixed
Hide
Permalink
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

Vote (0)
Watch (2)

Dates

  • Created:
    Updated:
    Resolved:
Atlassian JIRA (v4.4.3#663-r165197) | Report a problem
Powered by a free Atlassian JIRA open source license for Moodle. Try JIRA - bug tracking software for your team.