Moodle
  1. Moodle
  2. MDL-31028

invalid use of file objects in event triggers for plagiarism plugins.

    Details

    • Testing Instructions:
      Hide

      No core code uses these events or functions so testing options are limited. - it should be sufficient to check to make sure that uploading a file to an assignment occurs without any warnings. (old assignment upload/uploadsingle and new assign)

      Show
      No core code uses these events or functions so testing options are limited. - it should be sufficient to check to make sure that uploading a file to an assignment occurs without any warnings. (old assignment upload/uploadsingle and new assign)
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull Master Branch:
      master-MDL-31028_2
    • Rank:
      37435

      Description

      When an 'assessable_file_uploaded' event is to be processed in the Moodle cron job, depending on what operations have been performed earlier in the job, the 'stored_file' class may not yet be declared before the event data is unserialised. PHP will unserialise the objects but it substitutes an incomplete class type and the cron job will fail if any attempt is made to call methods of the file object contained in the event data.

      To demonstrate this, I have attached a simple local plugin (eventtest.tar.gz) that implements an event handler that defers an 'assessable_file_uploaded' event to be processed in the cron job. Create a course in a test instance, add a file upload assignment (single or advanced, it doesn't matter), and submit a file. Then, run the Moodle cron job from the CLI repeatedly. Some executions will succeed if other earlier operations have called get_file_storage() or required lib/filelib.php, but if the events queue processing is the first time those classes are needed, they won't exist before the event handler is called, by which time the event data has been unserialised in events_process_queued_handler() and is an incomplete class. (See example output below.)

      A simple solution for this specific problem is to add a hack in events_process_queued_handler() (patch attached) to pull in lib/filelib.php if the event being processed is 'assessable_file_uploaded', but the root of this issue is likely to come up for other classes if the same sort of circumstances happen there. Implementing autoloading may be one route around this, or avoid storing objects of any type but stdClass in event data.

      fowlerj@q08-0967 ~/src/moodle-org.git $ sudo -u www-data php admin/cli/cron.php 
      Server Time: Thu, 05 Jan 2012 11:55:53 +1000
      
      
      Running clean-up tasks...
       Deleted old cache_text records
       Executed tag cron
       Cleaned up context instances
       Built context paths
       Cleaned cache flags
       Cleaned up read notifications
      ...finished clean-up tasks
       Created missing context instances
      Cleaned up stale user sessions
      Running auth crons if required...
      Running enrol crons if required...
      Running cron for enrol_cohort...
      Starting activity modules
      Processing module function assignment_cron ...... used 2 dbqueries
      ... used 0.052798986434937 seconds
      done.
      Processing module function forum_cron ...Starting digest processing...
      Cleaned old digest records
      ... used 4 dbqueries
      ... used 0.048096179962158 seconds
      done.
      Finished activity modules
      Starting blocks
      Finished blocks
      Starting quiz reports
      Finished quiz reports
      Starting admin reports
      Finished admin reports
      Starting main gradebook job...
      done.
      Starting processing the event queue...
       *** in local_eventtest_handler::assessable_file_uploaded
       *** class_exists(stored_file) = true
       *** get_class($eventdata->files[e4a5b27283c568ae7e57014c3c2caf1b5c92d91e]) = stored_file
       *** get_class($eventdata->files[807b87cb6e01ead5107e37722a29ff9b26552363]) = stored_file
       *** get_class($eventdata->files[50afe513f4fef6e0d31952f79aec6b51e2ad7bb3]) = stored_file
      done.
      Starting course reports
      Finished course reports
      Starting gradebook plugins
      Finished gradebook plugins
      Fetching external blog entries...done.
      Deleting blog associations linked to non-existent contexts...done.
      Starting registration update on hubs...
      Finished registration update on hubs.
      Deleting session linked tokens more than one day old...done.
      Starting admin tools
      Processing cron function for tool_qeupgradehelper...
      done. (1 dbqueries, 0 seconds)
      Finished admin tools
      Processing customized cron scripts ...done.
      Checking automated backup status...INACTIVE
      Deleting old draft files... done.
      Cron script completed correctly
      Execution took 0.573476 seconds
      
      
      
      fowlerj@q08-0967 ~/src/moodle-org.git $ sudo -u www-data php admin/cli/cron.php 
      Server Time: Thu, 05 Jan 2012 11:55:55 +1000
      
      
       Created missing context instances
      Cleaned up stale user sessions
      Running auth crons if required...
      Running enrol crons if required...
      Starting activity modules
      Finished activity modules
      Starting blocks
      Finished blocks
      Starting quiz reports
      Finished quiz reports
      Starting admin reports
      Finished admin reports
      Starting main gradebook job...
      done.
      Starting processing the event queue...
       *** in local_eventtest_handler::assessable_file_uploaded
       *** class_exists(stored_file) = false
       *** get_class($eventdata->files[e4a5b27283c568ae7e57014c3c2caf1b5c92d91e]) = __PHP_Incomplete_Class
       *** get_class($eventdata->files[807b87cb6e01ead5107e37722a29ff9b26552363]) = __PHP_Incomplete_Class
       *** get_class($eventdata->files[50afe513f4fef6e0d31952f79aec6b51e2ad7bb3]) = __PHP_Incomplete_Class
      PHP Fatal error:  local_eventtest_handler::assessable_file_uploaded(): The script tried to execute a \
      method or access a property of an incomplete object. Please ensure that the class definition \
      "stored_file" of the object you are trying to operate on was loaded _before_ unserialize() gets \
      called or provide a __autoload() function to load the class definition \
        in /home/fowlerj/src/moodle-org.git/local/eventtest/lib.php on line 20
      PHP Stack trace:
      PHP   1. {main}() /home/fowlerj/src/moodle-org.git/admin/cli/cron.php:0
      PHP   2. cron_run() /home/fowlerj/src/moodle-org.git/admin/cli/cron.php:61
      PHP   3. events_cron() /home/fowlerj/src/moodle-org.git/lib/cronlib.php:342
      PHP   4. events_process_queued_handler() /home/fowlerj/src/moodle-org.git/lib/eventslib.php:448
      PHP   5. events_dispatch() /home/fowlerj/src/moodle-org.git/lib/eventslib.php:340
      PHP   6. call_user_func() /home/fowlerj/src/moodle-org.git/lib/eventslib.php:296
      PHP   7. local_eventtest_handler::assessable_file_uploaded() /home/fowlerj/src/moodle-org.git/lib/eventslib.php:0
      
      1. assessable-file-uploaded.patch
        1 kB
        Jonathon Fowler
      2. eventtest.tar.gz
        0.8 kB
        Jonathon Fowler

        Issue Links

          Activity

          Hide
          Michael de Raadt added a comment -

          Thanks for spotting that and providing a solution.

          Show
          Michael de Raadt added a comment - Thanks for spotting that and providing a solution.
          Hide
          Petr Škoda added a comment -

          Hello, the problem here is that it is not possible to send full objects to events api - $eventdata->files is very wrong, instead is must create new stdClass instance and copy information from the stored_file instances. The biggest danger is that the stored_file record may become stale before it is processed in cron.

          The file object was introduced to the assignment code in MDL-21861, I am proposing to remove it because full objects are not allowed in events api.

          Any other ideas?

          Show
          Petr Škoda added a comment - Hello, the problem here is that it is not possible to send full objects to events api - $eventdata->files is very wrong, instead is must create new stdClass instance and copy information from the stored_file instances. The biggest danger is that the stored_file record may become stale before it is processed in cron. The file object was introduced to the assignment code in MDL-21861 , I am proposing to remove it because full objects are not allowed in events api. Any other ideas?
          Hide
          Petr Škoda added a comment - - edited

          The eventdata->files could be a simple array with filepaths relative to the assignment file area, unfortunately there does not seem to be any way to make this fully backwards compatible.

          Show
          Petr Škoda added a comment - - edited The eventdata->files could be a simple array with filepaths relative to the assignment file area, unfortunately there does not seem to be any way to make this fully backwards compatible.
          Hide
          Jonathon Fowler added a comment -

          The eventdata object could store pathname hashes in a replacement array for 'files' named, say, 'pathnamehashes', and as a deprecated backward-compatibility measure do file_storage::get_file_by_hash() calls in events_dispatch(). Plagiarism and other plugins that handle this event should then migrate themselves to doing the file_storage::get_file_by_hash() calls themselves and ignore the 'files'/'file' properties in the eventdata.

          Also, eventdata currently may contain a single stored_file object in a 'file' property if the event is generated by mod/assignment/type/uploadsingle, or an array of them as 'files' if it came from mod/assignment/type/upload. I can't imagine any reason why a single array with one object in it would be any less suitable than having these two variations since both need to be handled by the event handler, and a typical implementation of this in the handler may be:

          if (!empty($eventdata->file)) {
              $eventdata->files = array($eventdata->file);
          }
          foreach ($eventdata->files as $file) {
              ...
          }
          

          I've implemented my thinkings on a branch at https://github.com/jonof/moodle/commits/MDL-31028-assessable-file-uploaded. It includes code for handling existing events in the queue that were generated under the 'old' way, as well as events generated in the 'new' way to be handled by plugins coded for the 'old' way. I suppose conversion of existing queued events' eventdata could be done in upgrade code which would remove the need to taint lib/eventslib.php quite so much. I'll see if I can implement this.

          Show
          Jonathon Fowler added a comment - The eventdata object could store pathname hashes in a replacement array for 'files' named, say, 'pathnamehashes', and as a deprecated backward-compatibility measure do file_storage::get_file_by_hash() calls in events_dispatch(). Plagiarism and other plugins that handle this event should then migrate themselves to doing the file_storage::get_file_by_hash() calls themselves and ignore the 'files'/'file' properties in the eventdata. Also, eventdata currently may contain a single stored_file object in a 'file' property if the event is generated by mod/assignment/type/uploadsingle, or an array of them as 'files' if it came from mod/assignment/type/upload. I can't imagine any reason why a single array with one object in it would be any less suitable than having these two variations since both need to be handled by the event handler, and a typical implementation of this in the handler may be: if (!empty($eventdata->file)) { $eventdata->files = array($eventdata->file); } foreach ($eventdata->files as $file) { ... } I've implemented my thinkings on a branch at https://github.com/jonof/moodle/commits/MDL-31028-assessable-file-uploaded . It includes code for handling existing events in the queue that were generated under the 'old' way, as well as events generated in the 'new' way to be handled by plugins coded for the 'old' way. I suppose conversion of existing queued events' eventdata could be done in upgrade code which would remove the need to taint lib/eventslib.php quite so much. I'll see if I can implement this.
          Hide
          Dan Marsden added a comment -

          I'm wondering if we need to handle the deprec method at all here...

          people using the older plugins will find that the files will never get submitted, getting the error in the cron log as shown - and because each event will return false it won't clear the event - it will keep trying each cron and fail.

          We could patch Moodle to use the pathname hashes as shown in your patch and then write an upgrade script within the plagiarism plugin to "convert" all old events still in the queue during upgrade of the plagiarism plugin - that way the core code is kept very clean. - how does that sound?

          Show
          Dan Marsden added a comment - I'm wondering if we need to handle the deprec method at all here... people using the older plugins will find that the files will never get submitted, getting the error in the cron log as shown - and because each event will return false it won't clear the event - it will keep trying each cron and fail. We could patch Moodle to use the pathname hashes as shown in your patch and then write an upgrade script within the plagiarism plugin to "convert" all old events still in the queue during upgrade of the plagiarism plugin - that way the core code is kept very clean. - how does that sound?
          Hide
          Jonathon Fowler added a comment -

          Well, I just figured out some upgrade code for mod/assignment/db/upgrade.php which handles conversion of any queued 'assessable_file_uploaded' events: https://github.com/jonof/moodle/commit/d84c8ba5f6eaf663b23fa5e05a456d55eab52782

          It removes the backwards/forwards-compatibility additions in lib/eventlib.php that the previous commit https://github.com/jonof/moodle/commit/b7edec20d3619d52d36a473618e2679421497f7a introduced.

          I'll let you guys decide which bits you feel are worth using.

          Show
          Jonathon Fowler added a comment - Well, I just figured out some upgrade code for mod/assignment/db/upgrade.php which handles conversion of any queued 'assessable_file_uploaded' events: https://github.com/jonof/moodle/commit/d84c8ba5f6eaf663b23fa5e05a456d55eab52782 It removes the backwards/forwards-compatibility additions in lib/eventlib.php that the previous commit https://github.com/jonof/moodle/commit/b7edec20d3619d52d36a473618e2679421497f7a introduced. I'll let you guys decide which bits you feel are worth using.
          Hide
          chester folming added a comment -

          As I understand it you resolved this in principle, and are deciding if it should be a "forward in time" solution only.
          While this is a minor issue I agree, could you maybe give some estimate as to how long before it goes into a new build..?

          Show
          chester folming added a comment - As I understand it you resolved this in principle, and are deciding if it should be a "forward in time" solution only. While this is a minor issue I agree, could you maybe give some estimate as to how long before it goes into a new build..?
          Hide
          Petr Škoda added a comment -

          Workaround could be to use instant event handler instead of cron handler.

          Show
          Petr Škoda added a comment - Workaround could be to use instant event handler instead of cron handler.
          Hide
          Christopher Tombleson added a comment - - edited

          I have implemented a fix that passes a reference to the file by passing the files pathname hash.

          https://github.com/chtombleson/moodle/tree/master-MDL-31028_2

          Show
          Christopher Tombleson added a comment - - edited I have implemented a fix that passes a reference to the file by passing the files pathname hash. https://github.com/chtombleson/moodle/tree/master-MDL-31028_2
          Hide
          Dan Marsden added a comment -

          This patch from Chris also includes a fix for a strict standards error and comments to state that using $eventdata->files is deprec - we should leave this in 2.3 but I'll add a new tracker issue when this is accepted to remove it from 2.4 as per Petrs comments above.

          Note to integrator - this is for master only!

          thanks to Chris for doing the work.

          Show
          Dan Marsden added a comment - This patch from Chris also includes a fix for a strict standards error and comments to state that using $eventdata->files is deprec - we should leave this in 2.3 but I'll add a new tracker issue when this is accepted to remove it from 2.4 as per Petrs comments above. Note to integrator - this is for master only! thanks to Chris for doing the work.
          Hide
          Dan Marsden added a comment -

          (and thanks to Jonathan for the original patches/tracker issue!!!!)

          Show
          Dan Marsden added a comment - (and thanks to Jonathan for the original patches/tracker issue!!!!)
          Hide
          Petr Škoda added a comment -

          thanks!

          Show
          Petr Škoda added a comment - thanks!
          Hide
          Sam Hemelryk added a comment -

          Thanks guys, this has been integrated now.

          Show
          Sam Hemelryk added a comment - Thanks guys, this has been integrated now.
          Hide
          Sam Hemelryk added a comment -

          Tested and passed

          Show
          Sam Hemelryk added a comment - Tested and passed
          Hide
          Eloy Lafuente (stronk7) added a comment -

          This is now part of Moodle and a few millions people around the globe will be using it soon. Isn't that awesome?

          Many, many thanks and don't forget http://youtu.be/4N7dPaP5Z8U

          Closing, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - This is now part of Moodle and a few millions people around the globe will be using it soon. Isn't that awesome? Many, many thanks and don't forget http://youtu.be/4N7dPaP5Z8U Closing, ciao

            People

            • Votes:
              1 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: