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

diff --git a/includes/objectcache/MemcachedBagOStuff.php b/includes/objectcache/MemcachedBagOStuff.php
index 12ba83e..0489531 100644 (file)
--- a/includes/objectcache/MemcachedBagOStuff.php
+++ b/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

diff --git a/includes/objectcache/MemcachedPeclBagOStuff.php b/includes/objectcache/MemcachedPeclBagOStuff.php
index 365236d..e6df900 100644 (file)
--- a/includes/objectcache/MemcachedPeclBagOStuff.php
+++ b/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 );
        }

diff --git a/tests/phpunit/includes/objectcache/MemcachedBagOStuffTest.php b/tests/phpunit/includes/objectcache/MemcachedBagOStuffTest.php
index 64478a2..938f3b9 100644 (file)
--- a/tests/phpunit/includes/objectcache/MemcachedBagOStuffTest.php
+++ b/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(