From a8f00e5c9738447063f87f5c704e7a99becd9219 Mon Sep 17 00:00:00 2001
From: Matthias Mullie <git@mullie.eu>
Date: Mon, 4 Feb 2013 17:54:53 +0100
Subject: [PATCH 1/1] Read full memcached response before manipulating data

Memcached response when fetching data typically looks like this:
VALUE <the stored value for whatever key you requested>
END

What the code used to do is read the first line (the VALUE) and re-
assemble the data being fetched there (like unserializing serialized
data). After that, it will read the next line (END).

The value could be a serialized object, which could have a __wakeup.
This __wakeup could have code which in turn executes Memcached-
related stuff. The problem is that, while that object is being
unserialized already, it's wakeup code is attempting to read new
stuff from Memcached, but we have yet to read the END of the data
we're attempting to unserialize (when we'll read a new value from
Memcached, the first thing we'd get is the END we have not yet read..)

The correct way to go about this would be to first read the full
Memcached response, and only unserialize the read data after that.
This is exactly what this patchset does.

Change-Id: I902809c6dde657091c8161a09df823170bd41f7a
---
 includes/objectcache/MemcachedClient.php      | 47 ++++++++++++-------
 .../includes/objectcache/BagOStuffTest.php    | 31 ++++++++++++
 2 files changed, 61 insertions(+), 17 deletions(-)

diff --git a/includes/objectcache/MemcachedClient.php b/includes/objectcache/MemcachedClient.php
index 2342d63370..54f6c59108 100644
--- a/includes/objectcache/MemcachedClient.php
+++ b/includes/objectcache/MemcachedClient.php
@@ -908,34 +908,47 @@ class MWMemcached {
 	 * @access private
 	 */
 	function _load_items( $sock, &$ret, &$casToken = null ) {
+		$results = array();
+
 		while ( 1 ) {
 			$decl = $this->_fgets( $sock );
 			if( $decl === false ) {
 				return false;
-			} elseif ( $decl == "END" ) {
-				return true;
 			} elseif ( preg_match( '/^VALUE (\S+) (\d+) (\d+) (\d+)$/', $decl, $match ) ) {
-				list( $rkey, $flags, $len, $casToken ) = array( $match[1], $match[2], $match[3], $match[4] );
-				$data = $this->_fread( $sock, $len + 2 );
-				if ( $data === false ) {
-					return false;
-				}
-				if ( substr( $data, -2 ) !== "\r\n" ) {
-					$this->_handle_error( $sock,
-						'line ending missing from data block from $1' );
+
+				$results[] = array(
+					$match[1], // rkey
+					$match[2], // flags
+					$match[3], // len
+					$match[4], // casToken
+					$this->_fread( $sock, $match[3] + 2 ), // data
+				);
+			} elseif ( $decl == "END" ) {
+				if ( count( $results ) == 0 ) {
 					return false;
 				}
-				$data = substr( $data, 0, -2 );
-				$ret[$rkey] = $data;
 
-				if ( $this->_have_zlib && $flags & self::COMPRESSED ) {
-					$ret[$rkey] = gzuncompress( $ret[$rkey] );
-				}
+				foreach ( $results as $vars ) {
+					list( $rkey, $flags, $len, $casToken, $data ) = $vars;
+
+					if ( $data === false || substr( $data, -2 ) !== "\r\n" ) {
+						$this->_handle_error( $sock,
+							'line ending missing from data block from $1' );
+						return false;
+					}
+					$data = substr( $data, 0, -2 );
+					$ret[$rkey] = $data;
 
-				if ( $flags & self::SERIALIZED ) {
-					$ret[$rkey] = unserialize( $ret[$rkey] );
+					if ( $this->_have_zlib && $flags & self::COMPRESSED ) {
+						$ret[$rkey] = gzuncompress( $ret[$rkey] );
+					}
+
+					if ( $flags & self::SERIALIZED ) {
+						$ret[$rkey] = unserialize( $ret[$rkey] );
+					}
 				}
 
+				return true;
 			} else {
 				$this->_handle_error( $sock, 'Error parsing response from $1' );
 				return false;
diff --git a/tests/phpunit/includes/objectcache/BagOStuffTest.php b/tests/phpunit/includes/objectcache/BagOStuffTest.php
index f3dd0a0ef4..88b07f0ae5 100644
--- a/tests/phpunit/includes/objectcache/BagOStuffTest.php
+++ b/tests/phpunit/includes/objectcache/BagOStuffTest.php
@@ -15,6 +15,7 @@ class BagOStuffTest extends MediaWikiTestCase {
 			$name = $this->getCliArg( 'use-bagostuff=' );
 
 			$this->cache = ObjectCache::newFromId( $name );
+
 		} else {
 			// no type defined - use simple hash
 			$this->cache = new HashBagOStuff;
@@ -104,4 +105,34 @@ class BagOStuffTest extends MediaWikiTestCase {
 			}
 		}
 	}
+
+	public function testAdd() {
+		$key = wfMemcKey( 'test' );
+		$this->assertTrue( $this->cache->add( $key, 'test' ) );
+	}
+
+	public function testGet() {
+		$value = array( 'this' => 'is', 'a' => 'test' );
+
+		$key = wfMemcKey( 'test' );
+		$this->cache->add( $key, $value );
+		$this->assertEquals( $this->cache->get( $key ), $value );
+	}
+
+	public function testGetMulti() {
+		$value1 = array( 'this' => 'is', 'a' => 'test' );
+		$value2 = array( 'this' => 'is', 'another' => 'test' );
+
+		$key1 = wfMemcKey( 'test1' );
+		$key2 = wfMemcKey( 'test2' );
+
+		$this->cache->add( $key1, $value1 );
+		$this->cache->add( $key2, $value2 );
+
+		$this->assertEquals( $this->cache->getMulti( array( $key1, $key2 ) ), array( $key1 => $value1, $key2 => $value2 ) );
+
+		// cleanup
+		$this->cache->delete( $key1 );
+		$this->cache->delete( $key2 );
+	}
 }
-- 
2.20.1