From edf69e62d851ba02b0c44db0aab05b4da8144c4e Mon Sep 17 00:00:00 2001 From: Brad Jorsch Date: Fri, 7 Jul 2017 09:51:00 -0400 Subject: [PATCH] MultiWriteBagOStuff: Fix async writes of mutable objects If someone writes an object into a BagOStuff, they typically expect that later changes to the object will not affect the value stored. MultiWriteBagOStuff's async write handling was violating this expectation, which is potentially causing T168040. Bug: T168040 Change-Id: Ie897b900befdc8998614af06f9339cd07665703e --- includes/libs/objectcache/MultiWriteBagOStuff.php | 6 ++++++ .../libs/objectcache/MultiWriteBagOStuffTest.php | 10 +++++++--- 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/includes/libs/objectcache/MultiWriteBagOStuff.php b/includes/libs/objectcache/MultiWriteBagOStuff.php index 687c67c356..d94578dff1 100644 --- a/includes/libs/objectcache/MultiWriteBagOStuff.php +++ b/includes/libs/objectcache/MultiWriteBagOStuff.php @@ -181,6 +181,12 @@ class MultiWriteBagOStuff extends BagOStuff { $ret = true; $args = array_slice( func_get_args(), 3 ); + if ( $count > 1 && $asyncWrites ) { + // Deep-clone $args to prevent misbehavior when something writes an + // object to the BagOStuff then modifies it afterwards, e.g. T168040. + $args = unserialize( serialize( $args ) ); + } + foreach ( $this->caches as $i => $cache ) { if ( $i >= $count ) { break; // ignore the lower tiers diff --git a/tests/phpunit/includes/libs/objectcache/MultiWriteBagOStuffTest.php b/tests/phpunit/includes/libs/objectcache/MultiWriteBagOStuffTest.php index 775709f241..4a9f6cc9ac 100644 --- a/tests/phpunit/includes/libs/objectcache/MultiWriteBagOStuffTest.php +++ b/tests/phpunit/includes/libs/objectcache/MultiWriteBagOStuffTest.php @@ -81,22 +81,26 @@ class MultiWriteBagOStuffTest extends MediaWikiTestCase { */ public function testSetDelayed() { $key = wfRandomString(); - $value = wfRandomString(); + $value = (object)[ 'v' => wfRandomString() ]; + $expectValue = clone $value; // XXX: DeferredUpdates bound to transactions in CLI mode $dbw = wfGetDB( DB_MASTER ); $dbw->begin(); $this->cache->set( $key, $value ); + // Test that later changes to $value don't affect the saved value (e.g. T168040) + $value->v = 'bogus'; + // Set in tier 1 - $this->assertEquals( $value, $this->cache1->get( $key ), 'Written to tier 1' ); + $this->assertEquals( $expectValue, $this->cache1->get( $key ), 'Written to tier 1' ); // Not yet set in tier 2 $this->assertEquals( false, $this->cache2->get( $key ), 'Not written to tier 2' ); $dbw->commit(); // Set in tier 2 - $this->assertEquals( $value, $this->cache2->get( $key ), 'Written to tier 2' ); + $this->assertEquals( $expectValue, $this->cache2->get( $key ), 'Written to tier 2' ); } /** -- 2.20.1