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

GDPR: moodle_content_writer can cause endless loop

    XMLWordPrintable

    Details

    • Testing Instructions:
      Hide

      The error condition was not reproducible, so we are just going to confirm that GDPR export isn't broken by this change.

      1. Log in as a admin
      2. Navigate to site admin => users => privacy => data requests
      3. Create a new export request
      4. 35 only: Run cron:

        php admin/cli/cron.php
        

      5. Approve the request
      6. Run cron:

        php admin/cli/cron.php
        

        1. Confirm that the export was generated
      7. Download the file
        1. Confirm you can unzip it

      Note: I did an export of the same user with and without this change, and compared zip files - they were identical.

      Show
      The error condition was not reproducible, so we are just going to confirm that GDPR export isn't broken by this change. Log in as a admin Navigate to site admin => users => privacy => data requests Create a new export request 35 only: Run cron: php admin/cli/cron.php Approve the request Run cron: php admin/cli/cron.php Confirm that the export was generated Download the file Confirm you can unzip it Note: I did an export of the same user with and without this change, and compared zip files - they were identical.
    • Affected Branches:
      MOODLE_35_STABLE
    • Fixed Branches:
      MOODLE_35_STABLE, MOODLE_36_STABLE
    • Pull 3.5 Branch:
      MDL-64497-m35
    • Pull 3.6 Branch:
      MDL-64497-m36
    • Pull Master Branch:
      MDL-64497-master

      Description

      Here is some code from moodle_content_writer::get_file_content:

              $filepointer = fopen($filepath, 'r');
              while (!feof($filepointer)) {
                  // ...
              }
      

      The 'fopen' returns FALSE on error. Meanwhile, feof returns FALSE if its file pointer is not valid, such as when you pass in FALSE. (See second big red 'WARNING' box on the PHP manual page.)

      This error condition could occur for a variety of reasons (e.g. temporary file system failure) or possibly if there is a bug in a plugin's provider implementation. If it does occur, the consequences are likely to be terminal for the server unless an operator takes prompt action. In our case on a test server this endless loop, with PHP warnings occuring each time, caused the PHP error log to reach 56 GB and run out of space in the partition within an hour or so, so that the test server was not able to respond to requests.

      We would rather this doesn't happen on the production server.

      In addition I noticed another error, which is that this function never calls fclose($filepointer). It's possible that this may have been what caused it to fail in the first place (if it opened many thousands of files using this function in the same request).

      Finally, er, I can't see any reason this function is different from file_get_contents, other than having two critical bugs in it.

      I am submitting a fix as follows:

      1. Replace the entire contents of the buggy function with a call to file_get_contents.
      2. Check the return value and throw an exception if it failed.
      3. Also throw an exception if it fails writing the file, so that we detect the error closer to the point it occurs.

        Attachments

          Issue Links

            Activity

              People

              • Assignee:
                quen Sam Marshall
                Reporter:
                quen Sam Marshall
                Peer reviewer:
                Shamim Rezaie
                Integrator:
                Andrew Nicols
                Tester:
                Janelle Barcega
                Participants:
                Component watchers:
                Andrew Nicols, Mathew May, Michael Hawkins, Shamim Rezaie, Simey Lameze
              • Votes:
                0 Vote for this issue
                Watchers:
                6 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  14/Jan/19

                  Time Tracking

                  Estimated:
                  Original Estimate - 0 minutes
                  0m
                  Remaining:
                  Remaining Estimate - 0 minutes
                  0m
                  Logged:
                  Time Spent - 21 minutes
                  21m