Uploaded image for project: '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

      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.

        Gliffy Diagrams

          Attachments

            Activity

            Hide
            poltawski Dan Poltawski added a comment -

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

            Show
            poltawski Dan Poltawski added a comment - Have just reproduced this on 1.9.. Adding some watchers and raising priority.
            Hide
            martinlanghoff 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
            martinlanghoff 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
            nicolasconnault Nicolas Connault added a comment -

            Reproduced on 1.9 also.

            Show
            nicolasconnault Nicolas Connault added a comment - Reproduced on 1.9 also.
            Hide
            nicolasconnault 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
            nicolasconnault 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
            martinlanghoff 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
            martinlanghoff 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
            martinlanghoff Martín Langhoff added a comment -

            Patched to

            • only use clone
            • do it on set and get
            Show
            martinlanghoff Martín Langhoff added a comment - Patched to only use clone do it on set and get
            Hide
            ashleyholman 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
            ashleyholman 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
            ashleyholman Ashley Holman added a comment -

            Causes syntax error in PHP4 causing new installation to fail

            Show
            ashleyholman Ashley Holman added a comment - Causes syntax error in PHP4 causing new installation to fail
            Hide
            martinlanghoff 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
            martinlanghoff 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
            ashleyholman Ashley Holman added a comment -

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

            Thanks.

            Show
            ashleyholman Ashley Holman added a comment - I just updated/tested and it's working fine for me now. Thanks.
            Hide
            bushido 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
            bushido 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
            martinlanghoff 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
            martinlanghoff 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
            nicolasconnault Nicolas Connault added a comment -

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

            Show
            nicolasconnault Nicolas Connault added a comment - Thanks Martin for your patch, and sorry about my botch-up
            Hide
            bushido 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
            bushido 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
            bushido Mark Nielsen added a comment -

            Attaching a diff with a proposed solution.

            Show
            bushido Mark Nielsen added a comment - Attaching a diff with a proposed solution.
            Hide
            martinlanghoff 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
            martinlanghoff 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
            bushido Mark Nielsen added a comment -

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

            Show
            bushido 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:
                  Fix Release Date:
                  3/Mar/08