Ensure all key transformations are applied by BagOStuff::makeKeyInternal()

Currently we have to undo any transformations we apply to keys in getMulti() by
iterating over the response array keys reversing any changes. This is hairy and
complicated. So --

* Replace calls to MemcachedBagOStuff::encodeKey() with calls to a new method,
  MemcachedBagOStuff::validateKeyEncoding(). The new method only validates that
  the key is compatible with memcached. If it is not, it throws an exception.
  If it is, it returns the key unmodified.

* Remove MemcachedBagOStuff::{encode,decode}Key(), since they no longer serve a
  purpose, and have no callers.

Change-Id: If3e20c6a1a1b42fc1f2823aa660328e37c26eccb

includes/objectcache/MemcachedBagOStuff.php

@@ -65,25 +65,25 @@ class MemcachedBagOStuff extends BagOStuff {
 	}
 
 	protected function getWithToken( $key, &$casToken, $flags = 0 ) {
-		return $this->client->get( $this->encodeKey( $key ), $casToken );
+		return $this->client->get( $this->validateKeyEncoding( $key ), $casToken );
 	}
 
 	public function set( $key, $value, $exptime = 0, $flags = 0 ) {
-		return $this->client->set( $this->encodeKey( $key ), $value,
+		return $this->client->set( $this->validateKeyEncoding( $key ), $value,
 			$this->fixExpiry( $exptime ) );
 	}
 
 	protected function cas( $casToken, $key, $value, $exptime = 0 ) {
-		return $this->client->cas( $casToken, $this->encodeKey( $key ),
+		return $this->client->cas( $casToken, $this->validateKeyEncoding( $key ),
 			$value, $this->fixExpiry( $exptime ) );
 	}
 
 	public function delete( $key ) {
-		return $this->client->delete( $this->encodeKey( $key ) );
+		return $this->client->delete( $this->validateKeyEncoding( $key ) );
 	}
 
 	public function add( $key, $value, $exptime = 0 ) {
-		return $this->client->add( $this->encodeKey( $key ), $value,
+		return $this->client->add( $this->validateKeyEncoding( $key ), $value,
 			$this->fixExpiry( $exptime ) );
 	}
 
@@ -121,10 +121,14 @@ class MemcachedBagOStuff extends BagOStuff {
 		$that = $this;
 		$args = array_map(
 			function ( $arg ) use ( $that, &$charsLeft ) {
-				// Because MemcachedBagOStuff::encodeKey() will be called again
-				// with this input once the key is actually used, we have to
-				// encode pound signs here rather than in encodeKey().
-				$arg = $that->encodeKey( str_replace( '#', '%23', $arg ) );
+				// Make sure %, #, and non-ASCII chars are escaped
+				$arg = preg_replace_callback(
+					'/[^\x21-\x22\x24\x26-\x7e]+/',
+					function ( $m ) {
+						return rawurlencode( $m[0] );
+					},
+					$arg
+				);
 
 				// 33 = 32 characters for the MD5 + 1 for the '#' prefix.
 				if ( $charsLeft > 33 && strlen( $arg ) > $charsLeft ) {
@@ -145,26 +149,18 @@ class MemcachedBagOStuff extends BagOStuff {
 	}
 
 	/**
-	 * Encode a key for use on the wire inside the memcached protocol.
+	 * Ensure that a key is safe to use (contains no control characters and no
+	 * characters above the ASCII range.)
 	 *
-	 * We encode spaces and line breaks to avoid protocol errors. We encode
-	 * the other control characters for compatibility with libmemcached
-	 * verify_key. We leave other punctuation alone, to maximise backwards
-	 * compatibility.
 	 * @param string $key
 	 * @return string
+	 * @throws Exception
 	 */
-	public function encodeKey( $key ) {
-		return preg_replace_callback( '/[^\x21-\x7e]+/',
-			array( $this, 'encodeKeyCallback' ), $key );
-	}
-
-	/**
-	 * @param array $m
-	 * @return string
-	 */
-	protected function encodeKeyCallback( $m ) {
-		return rawurlencode( $m[0] );
+	public function validateKeyEncoding( $key ) {
+		if ( preg_match( '/[^\x21-\x7e]+/', $key ) ) {
+			throw new Exception( "Key contains invalid characters: $key" );
+		}
+		return $key;
 	}
 
 	/**
@@ -183,21 +179,6 @@ class MemcachedBagOStuff extends BagOStuff {
 		return (int)$expiry;
 	}
 
-	/**
-	 * Decode a key encoded with encodeKey(). This is provided as a convenience
-	 * function for debugging.
-	 *
-	 * @param string $key
-	 *
-	 * @return string
-	 */
-	public function decodeKey( $key ) {
-		// matches %00-%20, %25, %7F (=decoded alternatives for those encoded in encodeKey)
-		return preg_replace_callback( '/%([0-1][0-9]|20|25|7F)/i', function ( $match ) {
-			return urldecode( $match[0] );
-		}, $key );
-	}
-
 	/**
 	 * Send a debug message to the log
 	 * @param string $text

includes/objectcache/MemcachedPeclBagOStuff.php

@@ -117,7 +117,7 @@ class MemcachedPeclBagOStuff extends MemcachedBagOStuff {
 
 	protected function getWithToken( $key, &$casToken, $flags = 0 ) {
 		$this->debugLog( "get($key)" );
-		$result = $this->client->get( $this->encodeKey( $key ), null, $casToken );
+		$result = $this->client->get( $this->validateKeyEncoding( $key ), null, $casToken );
 		$result = $this->checkResult( $key, $result );
 		return $result;
 	}
@@ -202,14 +202,10 @@ class MemcachedPeclBagOStuff extends MemcachedBagOStuff {
 
 	public function getMulti( array $keys, $flags = 0 ) {
 		$this->debugLog( 'getMulti(' . implode( ', ', $keys ) . ')' );
-		$callback = array( $this, 'encodeKey' );
-		$encodedResult = $this->client->getMulti( array_map( $callback, $keys ) );
-		$encodedResult = $encodedResult ?: array(); // must be an array
-		$result = array();
-		foreach ( $encodedResult as $key => $value ) {
-			$key = $this->decodeKey( $key );
-			$result[$key] = $value;
+		foreach ( $keys as $key ) {
+			$this->validateKeyEncoding( $key );
 		}
+		$result = $this->client->getMulti( $keys ) ?: array();
 		return $this->checkResult( false, $result );
 	}
 
@@ -219,14 +215,10 @@ class MemcachedPeclBagOStuff extends MemcachedBagOStuff {
 	 * @return bool
 	 */
 	public function setMulti( array $data, $exptime = 0 ) {
-		foreach ( $data as $key => $value ) {
-			$encKey = $this->encodeKey( $key );
-			if ( $encKey !== $key ) {
-				$data[$encKey] = $value;
-				unset( $data[$key] );
-			}
-		}
 		$this->debugLog( 'setMulti(' . implode( ', ', array_keys( $data ) ) . ')' );
+		foreach ( array_keys( $data ) as $key ) {
+			$this->validateKeyEncoding( $key );
+		}
 		$result = $this->client->setMulti( $data, $this->fixExpiry( $exptime ) );
 		return $this->checkResult( false, $result );
 	}

tests/phpunit/includes/objectcache/MemcachedBagOStuffTest.php

@@ -21,8 +21,13 @@ class MemcachedBagOStuffTest extends MediaWikiTestCase {
 		);
 
 		$this->assertEquals(
-			$this->cache->makeKey( 'punctuation_marks_are_ok', '!@$%^&*()' ),
-			'test:punctuation_marks_are_ok:!@$%^&*()'
+			$this->cache->makeKey( 'punctuation_marks_are_ok', '!@$^&*()' ),
+			'test:punctuation_marks_are_ok:!@$^&*()'
+		);
+
+		$this->assertEquals(
+			$this->cache->makeKey( 'percent_is_escaped', '!@$%^&*()' ),
+			'test:percent_is_escaped:!@$%25^&*()'
 		);
 
 		$this->assertEquals(