PHP Memcached with Binary Protocol — Junk data returned after `increment()`

4
votes

I've started using the increment() method of the PHP Memcached client, and with that switched to the binary protocol. Apparently, increment() is only supported on the binary protocol. Occasionally, I'm seeing garbage results come back from incremented keys. For example:

$memcached = new \Memcached();
$memcached->setOption(\Memcached::OPT_BINARY_PROTOCOL, TRUE);

…

$this->cache->increment($key,1,1);

…

$this->cache->get($key);

Output:

"1\u0000ants1 0 1\r\n1\r\n1\r\n25\r"

Given that the key did not exist before it was incremented at first, and an initial value of 1 was given to the increment() call, I'd expect the value returned to be an integer. Instead, the strings returned look like left-over junk, e.g. the ants part of that string has no relevance.

Other (possibly) pertinent info:

  • I'm seeing this on a range of different keys
  • Our Memcached server is an AWS Elasticache instance
  • Other clients using the same cache node are not using the binary protocol.
  • All clients are running the same OS (CentOS), PHP and Memcached versions.
1
phpmemcachedamazon-elasticache
Would you happen to be using EC2s of both 32-bit and 64-bit builds both writing to this memcached instance? Also which version of memcached PHP bindings are you using? - Sherif

1 Answers

6
votes

tl;dr;

This is a bug in the PHP extension code...

I dug into the PHP extension code that wraps libmemcached and the libmemcached API code itself, but I think I've found the possible underlying cause of your problem...

If you take a look at the PHP Memcached::increment() implementation you'll see on line 1858 of php_memcached.c

status = memcached_increment_with_initial(m_obj->memc, key, key_len, (unsigned int)offset, initial, expiry, &value);

The problem here is that offset may or may not be 64 bits wide. The libmemcached API tells us that the memcached_increment_with_initial function signature expects a uint64_t for offset whereas here offset is declared long and then cast to unsigned int.

So if we were do something like this...

$memcached = new memcached;
$memcached->addServer('127.0.0.1','11211');
$memcached->setOption(\Memcached::OPT_BINARY_PROTOCOL, TRUE);

$memcached->delete('foo'); // remove the key if it already exists
$memcached->increment('foo',1,1);

var_dump($memcached->get('foo'));

You'd see something like...

string(22) "8589934592
"

as the output from that script. Note this only works if the key foo does not already exist on that memcached server. Also note the length of that string at 22 characters, when clearly it's not supposed to be anywhere near that.

If you look at the hex representation of that string....

 var_dump(bin2hex($memcached->get('foo')));

The result is clear garbage at the end...

 string(44) "38353839393334353932000d0a000000000000000000"

The object that was being stored was clearly corrupted between the casts. So you may end up getting the same result as me or you may end up getting completely broken data as you have demonstrated above. It depends on how the cast effected the chunk of memory being stored at the time (which is falling into undefined behavior here). Also the only seemingly root cause for this is using an initial value with increment (using increment subsequently after this does not demonstrate that problem or if the key already exists).

I guess the problem of this stems from the fact that the libmemcached API has two different size requirements for offset parameter between memcached_increment and memcached_increment_with_initial

memcached_increment(memcached_st *ptr, const char *key, size_t key_length, uint32_t offset, uint64_t *value)

The former takes uint32_t whereas the later takes uint64_t and PHP's extension code casts both to unsigned int, which would be equivalent to uint32_t pretty much.

This discrepency in the width of the offset parameter is likely what causes the key to be corrupted somehow between the calling PHP extension code and the API code.