Moodle
  1. Moodle
  2. MDL-12827

Internal record cache can be modified

    Details

    • Affected Branches:
      MOODLE_18_STABLE, MOODLE_19_STABLE
    • Fixed Branches:
      MOODLE_18_STABLE, MOODLE_19_STABLE, MOODLE_20_STABLE
    • Rank:
      30786

      Description

      Moodle config settings:

      cachetype: internal
      rcache: true

      The test (I added this block to the top of index.php):

      // Get course, print name, change name
      $course = get_record('course', 'id', SITEID);
      notify($course->fullname); // Prints: TESTING SITE
      $course->fullname = 'newname';

      // Get course again, print name
      $course = get_record('course', 'id', SITEID);
      notify($course->fullname); // Prints: newname
      die;

      From the above, the first time we get the course, we modify it, which in turn modifies the object in the cache. The cache should return a clone of the object stored in the cache to prevent this from happening.

      This doesn't seem to be a problem with the other caching types, but I wasn't able to test eaccelerator.

      1. dmlib.diff
        1.0 kB
        Mark Nielsen

        Activity

        Hide
        Dan Poltawski added a comment -

        Have just reproduced this on 1.9.. Adding some watchers and raising priority.

        Show
        Dan Poltawski added a comment - Have just reproduced this on 1.9.. Adding some watchers and raising priority.
        Hide
        Martín Langhoff added a comment -

        I agree - it should return a cloned obj. All the other caching schemes I'm aware of (memcached, eaccelerator, apc, turck) will return a different object, not a reference.

        It is fairly constrained...

        • The impact is short-lived as the cache is discarded at the end of the request
        • If you grab a record from the cache and change it, it's usually to update the DB...

        and probably easy to fix

        Show
        Martín Langhoff added a comment - I agree - it should return a cloned obj. All the other caching schemes I'm aware of (memcached, eaccelerator, apc, turck) will return a different object, not a reference. It is fairly constrained... The impact is short-lived as the cache is discarded at the end of the request If you grab a record from the cache and change it, it's usually to update the DB... and probably easy to fix
        Hide
        Nicolas Connault added a comment -

        Reproduced on 1.9 also.

        Show
        Nicolas Connault added a comment - Reproduced on 1.9 also.
        Hide
        Nicolas Connault added a comment -

        Modified the set_rcache method so that it saves a clone of the record object. This solves this issue.

        Show
        Nicolas Connault added a comment - Modified the set_rcache method so that it saves a clone of the record object. This solves this issue.
        Hide
        Martín Langhoff added a comment -

        Reopening. This patch is wrong on 2 counts

        • It does not fix the problem! it only clones when saving into the cache, not when retrieving. 2 independent retrievals will get the same reference.
        • It uses fullclone() – the record cache is for performance, the cost of serialize/unserialize is high, and it is not needed, as we know records from the DB won't carry references. They are 'final'.
        Show
        Martín Langhoff added a comment - Reopening. This patch is wrong on 2 counts It does not fix the problem! it only clones when saving into the cache, not when retrieving. 2 independent retrievals will get the same reference. It uses fullclone() – the record cache is for performance, the cost of serialize/unserialize is high, and it is not needed, as we know records from the DB won't carry references. They are 'final'.
        Hide
        Martín Langhoff added a comment -

        Patched to

        • only use clone
        • do it on set and get
        Show
        Martín Langhoff added a comment - Patched to only use clone do it on set and get
        Hide
        Ashley Holman added a comment -

        Under PHP4 I'm getting the following error:

        Parse error: parse error, unexpected T_VARIABLE in /var/www/cvs/19/moodle/lib/dmllib.php on line 2586

        This happens after a fresh install of MOODLE_19_STABLE

        Show
        Ashley Holman added a comment - Under PHP4 I'm getting the following error: Parse error: parse error, unexpected T_VARIABLE in /var/www/cvs/19/moodle/lib/dmllib.php on line 2586 This happens after a fresh install of MOODLE_19_STABLE
        Hide
        Ashley Holman added a comment -

        Causes syntax error in PHP4 causing new installation to fail

        Show
        Ashley Holman added a comment - Causes syntax error in PHP4 causing new installation to fail
        Hide
        Martín Langhoff added a comment -

        Thanks for spotting this - eagle eyes, finding the right bug to comment this. Just fixed it so if you update in the next hour or so you should get the fix.

        Else, just search for clone $bla and replace it with clone($bla) - we need the parenthesis for it to work correctly in PHP4. I'm off home for the day, so it'd be great to hear feedback on the fix I just committed to cvs.

        Show
        Martín Langhoff added a comment - Thanks for spotting this - eagle eyes, finding the right bug to comment this. Just fixed it so if you update in the next hour or so you should get the fix. Else, just search for clone $bla and replace it with clone($bla) - we need the parenthesis for it to work correctly in PHP4. I'm off home for the day, so it'd be great to hear feedback on the fix I just committed to cvs.
        Hide
        Ashley Holman added a comment -

        I just updated/tested and it's working fine for me now.

        Thanks.

        Show
        Ashley Holman added a comment - I just updated/tested and it's working fine for me now. Thanks.
        Hide
        Mark Nielsen added a comment -

        Thanks for all of the great work on this. Checked out the code and it looks like Martin's changes have not been merged to HEAD or to MOODLE_18_STABLE. MOODLE_18_STABLE is still using fullclone and HEAD is using clone without the open/close parenthesis.

        Show
        Mark Nielsen added a comment - Thanks for all of the great work on this. Checked out the code and it looks like Martin's changes have not been merged to HEAD or to MOODLE_18_STABLE. MOODLE_18_STABLE is still using fullclone and HEAD is using clone without the open/close parenthesis.
        Hide
        Martín Langhoff added a comment -

        Good spotting. The commit to HEAD had errored out and I didn't realise. Fixed in HEAD and 18_STABLE now.

        Show
        Martín Langhoff added a comment - Good spotting. The commit to HEAD had errored out and I didn't realise. Fixed in HEAD and 18_STABLE now.
        Hide
        Nicolas Connault added a comment -

        Thanks Martin for your patch, and sorry about my botch-up

        Show
        Nicolas Connault added a comment - Thanks Martin for your patch, and sorry about my botch-up
        Hide
        Mark Nielsen added a comment -

        Looks like we are not done. Get the following error:

        Fatal error: __clone method called on non-object in path/to/lib/dmllib.php on line 2327

        Looks like we are caching boolean false as well (least in this case). Need to check to see if it is an object before cloning.

        Show
        Mark Nielsen added a comment - Looks like we are not done. Get the following error: Fatal error: __clone method called on non-object in path/to/lib/dmllib.php on line 2327 Looks like we are caching boolean false as well (least in this case). Need to check to see if it is an object before cloning.
        Hide
        Mark Nielsen added a comment -

        Attaching a diff with a proposed solution.

        Show
        Mark Nielsen added a comment - Attaching a diff with a proposed solution.
        Hide
        Martín Langhoff added a comment -

        Mark, Dan Poltawski already spotted and fixed it as MDL-12882, perhaps you are testing with an older codebase? Have a look at his patch, anyway.

        Show
        Martín Langhoff added a comment - Mark, Dan Poltawski already spotted and fixed it as MDL-12882 , perhaps you are testing with an older codebase? Have a look at his patch, anyway.
        Hide
        Mark Nielsen added a comment -

        Closing again since the reason why it was opened is now in MDL-12882

        Show
        Mark Nielsen added a comment - Closing again since the reason why it was opened is now in MDL-12882

          People

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

            Dates

            • Created:
              Updated:
              Resolved: