[#MDL-12827] Internal record cache can be modified

Issue Details (XML | Word | Printable)

Key:	MDL-12827
Type:	Bug
Status:	Closed
Resolution:	Fixed
Priority:	Blocker
Assignee:	Nicolas Connault
Reporter:	Mark Nielsen
Votes:	0
Watchers:	5

Operations

Add/Edit UI Mockup to this issue

If you were logged in you would be able to see more operations.

Moodle

Internal record cache can be modified

Created: 04/Jan/08 07:49 AM Updated: 22/Jan/08 02:53 AM

Return to search

Issue 81 of 2207 issue(s)

<< Previous | MDL-12827 | Next >>

Component/s:

Database activity module, Lib

Affects Version/s:

1.8, 1.9

Fix Version/s:

1.8.5, 1.9, 2.0

File Attachments:	1. dmlib.diff (1.0 kB)
Environment:	PHP5

Participants:	Ashley Holman, Dan Poltawski, Mark Nielsen, Martín Langhoff and Nicolas Connault
Security Level:	None
Resolved date:	08/Jan/08
Affected Branches:	MOODLE_18_STABLE, MOODLE_19_STABLE
Fixed Branches:	MOODLE_18_STABLE, MOODLE_19_STABLE, MOODLE_20_STABLE

Description

« Hide

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.

All

Comments

Change History

Version Control

Sort Order:

[ Permalink | « Hide ]

Dan Poltawski added a comment - 04/Jan/08 08:02 AM

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

[ Permalink | « Hide ]

Martín Langhoff added a comment - 04/Jan/08 09:23 AM

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

[ Permalink | « Hide ]

Nicolas Connault added a comment - 04/Jan/08 06:00 PM

Reproduced on 1.9 also.

[ Permalink | « Hide ]

Nicolas Connault added a comment - 04/Jan/08 06:25 PM

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

[ Permalink | « Hide ]

Martín Langhoff added a comment - 07/Jan/08 07:12 AM

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'.

[ Permalink | « Hide ]

Martín Langhoff added a comment - 07/Jan/08 08:40 AM

Patched to

only use clone
do it on set and get

[ Permalink | « Hide ]

Ashley Holman added a comment - 07/Jan/08 01:57 PM

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

[ Permalink | « Hide ]

Ashley Holman added a comment - 07/Jan/08 02:06 PM

Causes syntax error in PHP4 causing new installation to fail

[ Permalink | « Hide ]

Martín Langhoff added a comment - 07/Jan/08 02:07 PM

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.

[ Permalink | « Hide ]

Ashley Holman added a comment - 08/Jan/08 10:47 AM

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

Thanks.

[ Permalink | « Hide ]

Mark Nielsen added a comment - 08/Jan/08 01:03 PM

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.

[ Permalink | « Hide ]

Martín Langhoff added a comment - 08/Jan/08 01:17 PM

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

[ Permalink | « Hide ]

Nicolas Connault added a comment - 08/Jan/08 07:15 PM

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

[ Permalink | « Hide ]

Mark Nielsen added a comment - 19/Jan/08 10:52 AM

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.

[ Permalink | « Hide ]

Mark Nielsen added a comment - 19/Jan/08 10:53 AM

Attaching a diff with a proposed solution.

[ Permalink | « Hide ]

Martín Langhoff added a comment - 19/Jan/08 03:15 PM

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.

[ Permalink | « Hide ]

Mark Nielsen added a comment - 22/Jan/08 02:53 AM

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

All content on this web site is made available under the GNU General Public License, unless otherwise stated.