Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3.4, 2.4.7
    • Fix Version/s: 2.4.7, 2.5.3
    • Component/s: Files API
    • Labels:
    • Testing Instructions:
      Hide

      NOTE: This test is a bit difficult as it requires direct access to the filesystem and database of your Moodle server. (Also you should obviously only do this on a test server as it can be dangerous if you get it wrong.)

      NOTE: This code change does NOT apply if you have the xsendfile config option turned on (in which case it probably works already without needing this change). So if you have that option on, please turn it off before testing.

      The hard part is that you need to create a large file (more than 2GB) in the backup area of a course. If you don't have a very large course for backup, or if backup is currently failing, this can be difficult to do within Moodle, so here are instructions to do it manually.

      If you already have a large file in the backup area of a course, you can skip steps 1-9.

      1. Locate a large file on your computer. Any type of file will do (could be a video file, a DVD-ROM image, a game installer, or whatever) but it should be around 3GB or more in size. Note the exact size of the file in bytes.

      2. Compute the SHA-1 hash of the large file. You can do this using the sha1sum utility (Windows users: download at http://lists.gnupg.org/pipermail/gnupg-announce/2004q4/000184.html )

      3. Go to any small course on your Moodle server. If there is not already a course backup (or if you want to keep the existing course backup), make a new backup using default options so that it eventually appears in the 'Course backups' section of the Restore page.

      4. Mouse over the backup download link for this small file. Note the contextid and filename.

      5. Using a database access tool on your server, and using your database prefix (e.g. mdl_) instead of myprefix_, run a query such as:

      SELECT * FROM myprefix_files WHERE contextid=? AND component='backup'
      

      6. Look through the results to find the line that corresponds to the backup file above (you're going to overwrite this file with the large file). You will need the id.

      7. Run this query with the values you now have. Replace 'aaa (etc)' with the SHA-1 hash, 3333333333 with the file size, and 1111 with the id from the mdl_files table.

      update myprefix_files set 
      contenthash='aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa', 
      filesize=3333333333
      where id=1111
      

      8. Now locate the correct position for the file within the Moodle data directory; it should be in filedir/aa/aa where aa/aa are the first four characters of the contenthash. Create this subdirectory if it doesn't already exist.

      9. Copy the file into this directory. Rename it so that its filename is the same as the contenthash (no extension).

      If you did this right, you have now inserted your large file into the Moodle filesystem. (Note: If you get it wrong, the rest of the test will fail.)

      10. Reload the restore page. You should see the file listed with a suitable size such as 3.1GB.

      11. Click the Download link next to the file.

      EXPECTED: The file should begin downloading. Your browser should show the correct size (e.g. 3.1GB) as it downloads.

      12. When the file downloads, run the sha1sum utility again on the downloaded file.

      EXPECTED: The SHA-1 hash should be identical to the value seen previously, confirming that the file is exactly identical.

      13. From the same restore page or a different one, locate a smaller file (as small as you like, but less than 2GB). Download this file too.

      EXPECTED: Download should still complete OK. (This is just checking that I didn't break 'normal' downloads.)

      Show
      NOTE: This test is a bit difficult as it requires direct access to the filesystem and database of your Moodle server. (Also you should obviously only do this on a test server as it can be dangerous if you get it wrong.) NOTE: This code change does NOT apply if you have the xsendfile config option turned on (in which case it probably works already without needing this change). So if you have that option on, please turn it off before testing. The hard part is that you need to create a large file (more than 2GB) in the backup area of a course. If you don't have a very large course for backup, or if backup is currently failing, this can be difficult to do within Moodle, so here are instructions to do it manually. If you already have a large file in the backup area of a course, you can skip steps 1-9. 1. Locate a large file on your computer. Any type of file will do (could be a video file, a DVD-ROM image, a game installer, or whatever) but it should be around 3GB or more in size. Note the exact size of the file in bytes. 2. Compute the SHA-1 hash of the large file. You can do this using the sha1sum utility (Windows users: download at http://lists.gnupg.org/pipermail/gnupg-announce/2004q4/000184.html ) 3. Go to any small course on your Moodle server. If there is not already a course backup (or if you want to keep the existing course backup), make a new backup using default options so that it eventually appears in the 'Course backups' section of the Restore page. 4. Mouse over the backup download link for this small file. Note the contextid and filename. 5. Using a database access tool on your server, and using your database prefix (e.g. mdl_) instead of myprefix_, run a query such as: SELECT * FROM myprefix_files WHERE contextid=? AND component='backup' 6. Look through the results to find the line that corresponds to the backup file above (you're going to overwrite this file with the large file). You will need the id. 7. Run this query with the values you now have. Replace 'aaa (etc)' with the SHA-1 hash, 3333333333 with the file size, and 1111 with the id from the mdl_files table. update myprefix_files set contenthash='aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa', filesize=3333333333 where id=1111 8. Now locate the correct position for the file within the Moodle data directory; it should be in filedir/aa/aa where aa/aa are the first four characters of the contenthash. Create this subdirectory if it doesn't already exist. 9. Copy the file into this directory. Rename it so that its filename is the same as the contenthash (no extension). If you did this right, you have now inserted your large file into the Moodle filesystem. (Note: If you get it wrong, the rest of the test will fail.) 10. Reload the restore page. You should see the file listed with a suitable size such as 3.1GB. 11. Click the Download link next to the file. EXPECTED: The file should begin downloading. Your browser should show the correct size (e.g. 3.1GB) as it downloads. 12. When the file downloads, run the sha1sum utility again on the downloaded file. EXPECTED: The SHA-1 hash should be identical to the value seen previously, confirming that the file is exactly identical. 13. From the same restore page or a different one, locate a smaller file (as small as you like, but less than 2GB). Download this file too. EXPECTED: Download should still complete OK. (This is just checking that I didn't break 'normal' downloads.)
    • Affected Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE
    • Pull Master Branch:
      MDL-38194-master

      Description

      File downloads using the file API should operate correctly with large files (over 4GB), using Moodle out-of-the-box.

      • This currently does not work, so you cannot download large backup files.
      • It might work if you turn on the xsendfile option but not all webservers support this option and it is unlikely to be enabled without complex configuration; Moodle should work out-of-the-box.
      • Partially discussed in Moodle tracker MDL-34388.
      • May only be possible to make this work on 64-bit platforms but this should be OK (users requiring large files can be advised to use a 64-bit platform).

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Sam Marshall added a comment -

            Updating test instructions

            Show
            Sam Marshall added a comment - Updating test instructions
            Hide
            Sam Marshall added a comment -

            I think this code is ready for peer review, please. Some notes:

            1. This change applies to all use of pluginfile.php but in practice, there are unlikely to be large (more than 2GB) files anywhere except in the backup restore list page.

            2. Since this is a fairly serious bug (you can't download large backup files, which prevents transferring data to other servers, etc.) I have submitted branches for all supported versions. The change cherry-picked cleanly.

            3. I changed upgrade.txt only for master, although the new function is actually added in all three versions. Should I have actually changed upgrade.txt in all branches? Not sure we really want people using this function in public API particularly... Let me know if I should make that change in other branches though?

            4. There is no unit test for the new function; I did consider this but it would be too difficult/slow to test it. Manual test script is included in this issue.

            Show
            Sam Marshall added a comment - I think this code is ready for peer review, please. Some notes: 1. This change applies to all use of pluginfile.php but in practice, there are unlikely to be large (more than 2GB) files anywhere except in the backup restore list page. 2. Since this is a fairly serious bug (you can't download large backup files, which prevents transferring data to other servers, etc.) I have submitted branches for all supported versions. The change cherry-picked cleanly. 3. I changed upgrade.txt only for master, although the new function is actually added in all three versions. Should I have actually changed upgrade.txt in all branches? Not sure we really want people using this function in public API particularly... Let me know if I should make that change in other branches though? 4. There is no unit test for the new function; I did consider this but it would be too difficult/slow to test it. Manual test script is included in this issue.
            Hide
            Dan Poltawski added a comment -

            Hi Sam,

            Thanks for the patch! It looks great to me.

            About the filesize param - is it right to have this argument? (Would it be safer to stat() the file to make sure you are serving the complete file rather than allow the caller to send the wrong file-size?). I suppose its a minor optimisation to avoid that stat so I see why you've done it, but perhaps it could be an optional param. Though to be honest I don't feel strongly either way, just for your consideration.

            I think it should be backported. Regarding upgrade.txt, even in master I don't think its strictly required, so I think its fine not to mention it in the stables.

            Show
            Dan Poltawski added a comment - Hi Sam, Thanks for the patch! It looks great to me. About the filesize param - is it right to have this argument? (Would it be safer to stat() the file to make sure you are serving the complete file rather than allow the caller to send the wrong file-size?). I suppose its a minor optimisation to avoid that stat so I see why you've done it, but perhaps it could be an optional param. Though to be honest I don't feel strongly either way, just for your consideration. I think it should be backported. Regarding upgrade.txt, even in master I don't think its strictly required, so I think its fine not to mention it in the stables.
            Hide
            Sam Marshall added a comment -

            Thanks for review and explanation!

            As per your comment, I've made the filesize argument optional. If left out, it will call filesize() on the path.

            Submitting for integration review.

            Show
            Sam Marshall added a comment - Thanks for review and explanation! As per your comment, I've made the filesize argument optional. If left out, it will call filesize() on the path. Submitting for integration review.
            Hide
            Sam Hemelryk added a comment -

            Thanks Sam + Dan this has been integrated now.

            Show
            Sam Hemelryk added a comment - Thanks Sam + Dan this has been integrated now.
            Hide
            Damyon Wiese added a comment -

            Well, after killing my machine because my moodledata was mounted on a ramdisk, I have reinstalled my moodle and finished this test. It all passes fine.

            (Tested on 24, 25 and master).

            Show
            Damyon Wiese added a comment - Well, after killing my machine because my moodledata was mounted on a ramdisk, I have reinstalled my moodle and finished this test. It all passes fine. (Tested on 24, 25 and master).
            Hide
            Sam Marshall added a comment -

            Oh dear! Sorry for killing your machine. And thanks for testing.

            Show
            Sam Marshall added a comment - Oh dear! Sorry for killing your machine. And thanks for testing.
            Hide
            Sam Hemelryk added a comment -

            Yarrr me 'arties, good job done. Yer code 'as landed and the weeklies ave been released with your contributions in tow.
            The brethren thank ya for yer 'ard work and if there'd been treasure to ave ya would ave got yer cut.

            Thanks for the effort everyone, another successful weekly release has been rolled.
            Please keep in mind code freeze is just around the corner now, get your new features and improvements in ASAP.

            Many thanks
            Sam

            Show
            Sam Hemelryk added a comment - Yarrr me 'arties, good job done. Yer code 'as landed and the weeklies ave been released with your contributions in tow. The brethren thank ya for yer 'ard work and if there'd been treasure to ave ya would ave got yer cut. Thanks for the effort everyone, another successful weekly release has been rolled. Please keep in mind code freeze is just around the corner now, get your new features and improvements in ASAP. Many thanks Sam

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved: