From cdb543272883b18000c6a522c6526fa7d72f4c1c Mon Sep 17 00:00:00 2001 From: Ori Livneh Date: Fri, 23 Oct 2015 17:53:25 -0700 Subject: [PATCH] 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 | 61 +++++++------------ .../objectcache/MemcachedPeclBagOStuff.php | 22 +++---- .../objectcache/MemcachedBagOStuffTest.php | 9 ++- 3 files changed, 35 insertions(+), 57 deletions(-) diff --git a/includes/objectcache/MemcachedBagOStuff.php b/includes/objectcache/MemcachedBagOStuff.php index 12ba83e7c7..048953140c 100644 --- 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 365236d883..e6df9007bf 100644 --- 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 64478a2968..938f3b9ddf 100644 --- 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( -- 2.20.1